Merge pull request #20746 from poettering/sysctl-rework

various sysctl-util.c cleanups

src/basic/fileio.c

@@ -30,14 +30,16 @@
 /* The maximum size of the file we'll read in one go in read_full_file() (64M). */
 #define READ_FULL_BYTES_MAX (64U*1024U*1024U - 1U)
 
-/* The maximum size of virtual files we'll read in one go in read_virtual_file() (4M). Note that this limit
+/* The maximum size of virtual files (i.e. procfs, sysfs, and other virtual "API" files) we'll read in one go
- * is different (and much lower) than the READ_FULL_BYTES_MAX limit. This reflects the fact that we use
+ * in read_virtual_file(). Note that this limit is different (and much lower) than the READ_FULL_BYTES_MAX
- * different strategies for reading virtual and regular files: virtual files are generally size constrained:
+ * limit. This reflects the fact that we use different strategies for reading virtual and regular files:
- * there we allocate the full buffer size in advance. Regular files OTOH can be much larger, and here we grow
+ * virtual files we generally have to read in a single read() syscall since the kernel doesn't support
- * the allocations exponentially in a loop. In glibc large allocations are immediately backed by mmap()
+ * continuation read()s for them. Thankfully they are somewhat size constrained. Thus we can allocate the
- * making them relatively slow (measurably so). Thus, when allocating the full buffer in advance the large
+ * full potential buffer in advance. Regular files OTOH can be much larger, and there we grow the allocations
- * limit is a problem. When allocating piecemeal it's not. Hence pick two distinct limits. */
+ * exponentially in a loop. We use a size limit of 4M-2 because 4M-1 is the maximum buffer that /proc/sys/
-#define READ_VIRTUAL_BYTES_MAX (4U*1024U*1024U - 1U)
+ * allows us to read() (larger reads will fail with ENOMEM), and we want to read one extra byte so that we
+ * can detect EOFs. */
+#define READ_VIRTUAL_BYTES_MAX (4U*1024U*1024U - 2U)
 
 int fopen_unlocked(const char *path, const char *options, FILE **ret) {
         assert(ret);
@@ -393,7 +395,7 @@ int read_virtual_file(const char *filename, size_t max_size, char **ret_contents
          * contents* may be returned. (Though the read is still done using one syscall.) Returns 0 on
          * partial success, 1 if untruncated contents were read. */
 
-        fd = open(filename, O_RDONLY|O_CLOEXEC);
+        fd = open(filename, O_RDONLY|O_NOCTTY|O_CLOEXEC);
         if (fd < 0)
                 return -errno;
 

src/basic/socket-util.c

@@ -793,7 +793,7 @@ bool ifname_valid_full(const char *p, IfnameValidFlags flags) {
 
         /* Let's refuse "all" and "default" as interface name, to avoid collisions with the special sysctl
          * directories /proc/sys/net/{ipv4,ipv6}/conf/{all,default} */
-        if (STR_IN_SET(p, "all", "default"))
+        if (!FLAGS_SET(flags, IFNAME_VALID_SPECIAL) && STR_IN_SET(p, "all", "default"))
                 return false;
 
         for (const char *t = p; *t; t++) {

src/basic/socket-util.h

@@ -135,9 +135,10 @@ int ip_tos_to_string_alloc(int i, char **s);
 int ip_tos_from_string(const char *s);
 
 typedef enum {
-        IFNAME_VALID_ALTERNATIVE = 1 << 0,
+        IFNAME_VALID_ALTERNATIVE = 1 << 0, /* Allow "altnames" too */
-        IFNAME_VALID_NUMERIC     = 1 << 1,
+        IFNAME_VALID_NUMERIC     = 1 << 1, /* Allow decimal formatted ifindexes too */
-        _IFNAME_VALID_ALL        = IFNAME_VALID_ALTERNATIVE | IFNAME_VALID_NUMERIC,
+        IFNAME_VALID_SPECIAL     = 1 << 2, /* Allow the special names "all" and "default" */
+        _IFNAME_VALID_ALL        = IFNAME_VALID_ALTERNATIVE | IFNAME_VALID_NUMERIC | IFNAME_VALID_SPECIAL,
 } IfnameValidFlags;
 bool ifname_valid_char(char a);
 bool ifname_valid_full(const char *p, IfnameValidFlags flags);

src/basic/sysctl-util.c

@@ -5,11 +5,13 @@
 #include <stdio.h>
 #include <unistd.h>
 
+#include "af-list.h"
 #include "fd-util.h"
 #include "fileio.h"
 #include "log.h"
 #include "macro.h"
 #include "path-util.h"
+#include "socket-util.h"
 #include "string-util.h"
 #include "sysctl-util.h"
 
@@ -36,7 +38,7 @@ char *sysctl_normalize(char *s) {
         path_simplify(s);
 
         /* Kill the leading slash, but keep the first character of the string in the same place. */
-        if (*s == '/' && *(s+1))
+        if (s[0] == '/' && s[1] != 0)
                 memmove(s, s+1, strlen(s));
 
         return s;
@@ -44,25 +46,19 @@ char *sysctl_normalize(char *s) {
 
 int sysctl_write(const char *property, const char *value) {
         char *p;
-        _cleanup_close_ int fd = -1;
 
         assert(property);
         assert(value);
 
-        log_debug("Setting '%s' to '%.*s'.", property, (int) strcspn(value, NEWLINE), value);
-
         p = strjoina("/proc/sys/", property);
-        fd = open(p, O_WRONLY|O_CLOEXEC);
-        if (fd < 0)
-                return -errno;
 
-        if (!endswith(value, "\n"))
+        path_simplify(p);
-                value = strjoina(value, "\n");
+        if (!path_is_normalized(p))
+                return -EINVAL;
 
-        if (write(fd, value, strlen(value)) < 0)
+        log_debug("Setting '%s' to '%s'", p, value);
-                return -errno;
 
-        return 0;
+        return write_string_file(p, value, WRITE_STRING_FILE_VERIFY_ON_FAILURE | WRITE_STRING_FILE_DISABLE_BUFFER);
 }
 
 int sysctl_writef(const char *property, const char *format, ...) {
@@ -83,48 +79,59 @@ int sysctl_writef(const char *property, const char *format, ...) {
 int sysctl_write_ip_property(int af, const char *ifname, const char *property, const char *value) {
         const char *p;
 
-        assert(IN_SET(af, AF_INET, AF_INET6));
         assert(property);
         assert(value);
 
-        p = strjoina("/proc/sys/net/ipv", af == AF_INET ? "4" : "6",
+        if (!IN_SET(af, AF_INET, AF_INET6))
-                     ifname ? "/conf/" : "", strempty(ifname),
+                return -EAFNOSUPPORT;
-                     property[0] == '/' ? "" : "/", property);
 
-        log_debug("Setting '%s' to '%s'", p, value);
+        if (ifname) {
+                if (!ifname_valid_full(ifname, IFNAME_VALID_SPECIAL))
+                        return -EINVAL;
 
-        return write_string_file(p, value, WRITE_STRING_FILE_VERIFY_ON_FAILURE | WRITE_STRING_FILE_DISABLE_BUFFER);
+                p = strjoina("net/", af_to_ipv4_ipv6(af), "/conf/", ifname, "/", property);
+        } else
+                p = strjoina("net/", af_to_ipv4_ipv6(af), "/", property);
+
+        return sysctl_write(p, value);
 }
 
 int sysctl_read(const char *property, char **ret) {
         char *p;
-
-        assert(property);
-        assert(ret);
-
-        p = strjoina("/proc/sys/", property);
-        return read_full_virtual_file(p, ret, NULL);
-}
-
-int sysctl_read_ip_property(int af, const char *ifname, const char *property, char **ret) {
-        _cleanup_free_ char *value = NULL;
-        const char *p;
         int r;
 
-        assert(IN_SET(af, AF_INET, AF_INET6));
         assert(property);
 
-        p = strjoina("/proc/sys/net/ipv", af == AF_INET ? "4" : "6",
+        p = strjoina("/proc/sys/", property);
-                     ifname ? "/conf/" : "", strempty(ifname),
-                     property[0] == '/' ? "" : "/", property);
 
-        r = read_full_virtual_file(p, &value, NULL);
+        path_simplify(p);
+        if (!path_is_normalized(p)) /* Filter out attempts to write to /proc/sys/../../…, just in case */
+                return -EINVAL;
+
+        r = read_full_virtual_file(p, ret, NULL);
         if (r < 0)
                 return r;
-
-        truncate_nl(value);
         if (ret)
-                *ret = TAKE_PTR(value);
+                delete_trailing_chars(*ret, NEWLINE);
 
         return r;
 }
+
+int sysctl_read_ip_property(int af, const char *ifname, const char *property, char **ret) {
+        const char *p;
+
+        assert(property);
+
+        if (!IN_SET(af, AF_INET, AF_INET6))
+                return -EAFNOSUPPORT;
+
+        if (ifname) {
+                if (!ifname_valid_full(ifname, IFNAME_VALID_SPECIAL))
+                        return -EINVAL;
+
+                p = strjoina("net/", af_to_ipv4_ipv6(af), "/conf/", ifname, "/", property);
+        } else
+                p = strjoina("net/", af_to_ipv4_ipv6(af), "/", property);
+
+        return sysctl_read(p, ret);
+}

src/test/test-sysctl-util.c

@@ -1,10 +1,14 @@
 /* SPDX-License-Identifier: LGPL-2.1-or-later */
 
+#include "sd-id128.h"
+
+#include "errno-util.h"
+#include "hostname-util.h"
 #include "strv.h"
 #include "sysctl-util.h"
 #include "tests.h"
 
-static const char* cases[] = {
+static const char* const cases[] = {
         "a.b.c", "a/b/c",
         "a/b/c", "a/b/c",
         "a/b.c/d", "a/b.c/d",
@@ -24,7 +28,7 @@ static void test_sysctl_normalize(void) {
         log_info("/* %s */", __func__);
 
         const char **s, **expected;
-        STRV_FOREACH_PAIR(s, expected, cases) {
+        STRV_FOREACH_PAIR(s, expected, (const char**) cases) {
                 _cleanup_free_ char *t;
 
                 assert_se(t = strdup(*s));
@@ -35,10 +39,44 @@ static void test_sysctl_normalize(void) {
         }
 }
 
+static void test_sysctl_read(void) {
+        _cleanup_free_ char *s = NULL, *h = NULL;
+        sd_id128_t a, b;
+        int r;
+
+        assert_se(sysctl_read("kernel/random/boot_id", &s) >= 0);
+        assert_se(sd_id128_from_string(s, &a) >= 0);
+        assert_se(sd_id128_get_boot(&b) >= 0);
+        assert_se(sd_id128_equal(a, b));
+        s = mfree(s);
+
+        assert_se(sysctl_read_ip_property(AF_INET, "lo", "forwarding", &s));
+        assert_se(STR_IN_SET(s, "0", "1"));
+
+        r = sysctl_write_ip_property(AF_INET, "lo", "forwarding", s);
+        assert_se(r >= 0 || ERRNO_IS_PRIVILEGE(r) || r == -EROFS);
+        s = mfree(s);
+
+        assert_se(sysctl_read_ip_property(AF_INET, NULL, "ip_forward", &s));
+        assert_se(STR_IN_SET(s, "0", "1"));
+
+        r = sysctl_write_ip_property(AF_INET, NULL, "ip_forward", s);
+        assert_se(r >= 0 || ERRNO_IS_PRIVILEGE(r) || r == -EROFS);
+        s = mfree(s);
+
+        assert_se(sysctl_read("kernel/hostname", &s) >= 0);
+        assert_se(gethostname_full(GET_HOSTNAME_ALLOW_NONE|GET_HOSTNAME_ALLOW_LOCALHOST, &h) >= 0);
+        assert_se(streq(s, h));
+
+        r = sysctl_write("kernel/hostname", s);
+        assert_se(r >= 0 || ERRNO_IS_PRIVILEGE(r) || r == -EROFS);
+}
+
 int main(int argc, char *argv[]) {
         test_setup_logging(LOG_INFO);
 
         test_sysctl_normalize();
+        test_sysctl_read();
 
         return 0;
 }