Merge pull request #11182 from poettering/fileio-more-paranoia

More safety checks for fileio.c

8 files changed, 177 insertions, 136 deletions

diff --git a/TODO b/TODO
index b7038ec7c..1fbffc740 100644
--- a/TODO
+++ b/TODO
@@ -34,8 +34,6 @@ Features:
 
 * when importing an fs tree with machined, complain if image is not an OS
 
-* when we fork off generators and such, lower LIMIT_NOFILE soft limit to 1K
-
 * Maybe introduce a helper safe_exec() or so, which is to execve() which
   safe_fork() is to fork(). And then make revert the RLIMIT_NOFILE soft limit
   to 1K implicitly, unless explicitly opted-out.
@@ -63,16 +61,11 @@ Features:
 * when a socket unit is spawned with an AF_UNIX path in /var/run, complain and
   patch it to use /run instead
 
-* consider splitting out all temporary file creation APIs (we have so many in
-  fileio.h and elsewhere!) into a new util file of its own.
-
 * set memory.oom.group in cgroupsv2 for all leaf cgroups (kernel v4.19+)
 
 * add a new syscall group "@esoteric" for more esoteric stuff such as bpf() and
   usefaultd() and make systemd-analyze check for it.
 
-* drop umask() calls and suchlike from our generators, pid1 should set things up correctly anyway
-
 * paranoia: whenever we process passwords, call mlock() on the memory
   first. i.e. look for all places we use string_erase()/string_free_erase() and
   augment them with mlock()
@@ -165,13 +158,6 @@ Features:
 
 * nspawn: greater control over selinux label?
 
-* cgroups: figure out if we can somehow communicate in a cleaner way whether a
-  systemd instance not running in the cgroup root shall or shall not manage the
-  attributes of its top-level cgroup. Currently it assumes it manages all, but
-  then might get EPERM due to permission porblems/userns, which is OK, but this
-  should be revisited to make clearer and also work if the payload systemd runs
-  with full privs and without userns.
-
 * hibernate/s2h: make this robust and safe to enable in Fedora by default.
   Specifically:
 
@@ -320,11 +306,6 @@ Features:
   StateDirectory=, LogsDirectory=, CacheDirectory=, as well as RootDirectory= if it
   is set, plus the whole disk space any image configured with RootImage=.
 
-* Introduce "exit" as an EmergencyAction value, and allow to configure a
-  per-unit success/failure exit code to configure. This would be useful for
-  running commands inside of services inside of containers, which could then
-  propagate their failure state all the way up.
-
 * In DynamicUser= mode: before selecting a UID, use disk quota APIs on relevant
   disks to see if the UID is already in use.
 
@@ -341,7 +322,7 @@ Features:
 * show whether a service has out-of-date configuration in "systemctl status" by
   using mtime data of ConfigurationDirectory=.
 
-* replace all uses of fgets() + LINE_MAX by read_line()
+* replace all remaining uses of fgets() + LINE_MAX by read_line()
 
 * Add AddUser= setting to unit files, similar to DynamicUser=1 which however
   creates a static, persistent user rather than a dynamic, transient user. We
@@ -507,9 +488,6 @@ Features:
   state.
   http://lists.freedesktop.org/archives/systemd-devel/2015-April/030229.html
 
-* Maybe add support for the equivalent of "ethtool advertise" to .link files?
-  http://lists.freedesktop.org/archives/systemd-devel/2015-April/030112.html
-
 * The udev blkid built-in should expose a property that reflects
   whether media was sensed in USB CF/SD card readers. This should then
   be used to control SYSTEMD_READY=1/0 so that USB card readers aren't
@@ -635,7 +613,6 @@ Features:
 
 * cgroups:
   - implement per-slice CPUFairScheduling=1 switch
-  - handle jointly mounted controllers correctly
   - introduce high-level settings for RT budget, swappiness
   - how to reset dynamically changed unit cgroup attributes sanely?
   - when reloading configuration, apply new cgroup configuration
@@ -904,8 +881,6 @@ Features:
     PID 1...
   - optionally automatically add FORWARD rules to iptables whenever nspawn is
     running, remove them when shut down.
-  - maybe make copying of /etc/resolv.conf optional, and skip it if --read-only
-    is used
 
 * dissect
   - refuse mounting over a mount point
diff --git a/src/basic/fileio.c b/src/basic/fileio.c
index b22fcd030..66e127345 100644
--- a/src/basic/fileio.c
+++ b/src/basic/fileio.c
@@ -667,46 +667,6 @@ int fputs_with_space(FILE *f, const char *s, const char *separator, bool *space)
         return fputs(s, f);
 }
 
-int read_nul_string(FILE *f, char **ret) {
-        _cleanup_free_ char *x = NULL;
-        size_t allocated = 0, n = 0;
-
-        assert(f);
-        assert(ret);
-
-        /* Reads a NUL-terminated string from the specified file. */
-
-        for (;;) {
-                int c;
-
-                if (!GREEDY_REALLOC(x, allocated, n+2))
-                        return -ENOMEM;
-
-                c = fgetc(f);
-                if (c == 0) /* Terminate at NUL byte */
-                        break;
-                if (c == EOF) {
-                        if (ferror(f))
-                                return -errno;
-                        break; /* Terminate at EOF */
-                }
-
-                x[n++] = (char) c;
-        }
-
-        if (x)
-                x[n] = 0;
-        else {
-                x = new0(char, 1);
-                if (!x)
-                        return -ENOMEM;
-        }
-
-        *ret = TAKE_PTR(x);
-
-        return 0;
-}
-
 /* A bitmask of the EOL markers we know */
 typedef enum EndOfLineMarker {
         EOL_NONE     = 0,
@@ -715,11 +675,15 @@ typedef enum EndOfLineMarker {
         EOL_THIRTEEN = 1 << 2,  /* \r (aka CR)  */
 } EndOfLineMarker;
 
-static EndOfLineMarker categorize_eol(char c) {
-        if (c == '\n')
-                return EOL_TEN;
-        if (c == '\r')
-                return EOL_THIRTEEN;
+static EndOfLineMarker categorize_eol(char c, ReadLineFlags flags) {
+
+        if (!IN_SET(flags, READ_LINE_ONLY_NUL)) {
+                if (c == '\n')
+                        return EOL_TEN;
+                if (c == '\r')
+                        return EOL_THIRTEEN;
+        }
+
         if (c == '\0')
                 return EOL_ZERO;
 
@@ -728,9 +692,10 @@ static EndOfLineMarker categorize_eol(char c) {
 
 DEFINE_TRIVIAL_CLEANUP_FUNC(FILE*, funlockfile);
 
-int read_line(FILE *f, size_t limit, char **ret) {
+int read_line_full(FILE *f, size_t limit, ReadLineFlags flags, char **ret) {
         size_t n = 0, allocated = 0, count = 0;
         _cleanup_free_ char *buffer = NULL;
+        int r;
 
         assert(f);
 
@@ -770,7 +735,7 @@ int read_line(FILE *f, size_t limit, char **ret) {
 
                 for (;;) {
                         EndOfLineMarker eol;
-                        int c;
+                        char c;
 
                         if (n >= limit)
                                 return -ENOBUFS;
@@ -778,20 +743,13 @@ int read_line(FILE *f, size_t limit, char **ret) {
                         if (count >= INT_MAX) /* We couldn't return the counter anymore as "int", hence refuse this */
                                 return -ENOBUFS;
 
-                        errno = 0;
-                        c = fgetc_unlocked(f);
-                        if (c == EOF) {
-                                /* if we read an error, and have no data to return, then propagate the error */
-                                if (ferror_unlocked(f) && n == 0)
-                                        return errno > 0 ? -errno : -EIO;
-
-                                /* EOF is line ending too. */
+                        r = safe_fgetc(f, &c);
+                        if (r < 0)
+                                return r;
+                        if (r == 0) /* EOF is definitely EOL */
                                 break;
-                        }
-
-                        count++;
 
-                        eol = categorize_eol(c);
+                        eol = categorize_eol(c, flags);
 
                         if (FLAGS_SET(previous_eol, EOL_ZERO) ||
                             (eol == EOL_NONE && previous_eol != EOL_NONE) ||
@@ -800,10 +758,11 @@ int read_line(FILE *f, size_t limit, char **ret) {
                                  * EOL marker has been seen right before? In either of these three cases we are
                                  * done. But first, let's put this character back in the queue. */
                                 assert_se(ungetc(c, f) != EOF);
-                                count--;
                                 break;
                         }
 
+                        count++;
+
                         if (eol != EOL_NONE) {
                                 previous_eol |= eol;
                                 continue;
@@ -813,7 +772,7 @@ int read_line(FILE *f, size_t limit, char **ret) {
                                 if (!GREEDY_REALLOC(buffer, allocated, n + 2))
                                         return -ENOMEM;
 
-                                buffer[n] = (char) c;
+                                buffer[n] = c;
                         }
 
                         n++;
@@ -828,3 +787,30 @@ int read_line(FILE *f, size_t limit, char **ret) {
 
         return (int) count;
 }
+
+int safe_fgetc(FILE *f, char *ret) {
+        int k;
+
+        assert(f);
+
+        /* A safer version of plain fgetc(): let's propagate the error that happened while reading as such, and
+         * separate the EOF condition from the byte read, to avoid those confusion signed/unsigned issues fgetc()
+         * has. */
+
+        errno = 0;
+        k = fgetc(f);
+        if (k == EOF) {
+                if (ferror(f))
+                        return errno > 0 ? -errno : -EIO;
+
+                if (ret)
+                        *ret = 0;
+
+                return 0;
+        }
+
+        if (ret)
+                *ret = k;
+
+        return 1;
+}
diff --git a/src/basic/fileio.h b/src/basic/fileio.h
index 848024607..53e3f4ef5 100644
--- a/src/basic/fileio.h
+++ b/src/basic/fileio.h
@@ -61,6 +61,18 @@ int read_timestamp_file(const char *fn, usec_t *ret);
 
 int fputs_with_space(FILE *f, const char *s, const char *separator, bool *space);
 
-int read_nul_string(FILE *f, char **ret);
+typedef enum ReadLineFlags {
+        READ_LINE_ONLY_NUL = 1 << 0,
+} ReadLineFlags;
 
-int read_line(FILE *f, size_t limit, char **ret);
+int read_line_full(FILE *f, size_t limit, ReadLineFlags flags, char **ret);
+
+static inline int read_line(FILE *f, size_t limit, char **ret) {
+        return read_line_full(f, limit, 0, ret);
+}
+
+static inline int read_nul_string(FILE *f, size_t limit, char **ret) {
+        return read_line_full(f, limit, READ_LINE_ONLY_NUL, ret);
+}
+
+int safe_fgetc(FILE *f, char *ret);
diff --git a/src/basic/process-util.c b/src/basic/process-util.c
index 5b700df33..3cfaceea8 100644
--- a/src/basic/process-util.c
+++ b/src/basic/process-util.c
@@ -600,12 +600,14 @@ int get_process_root(pid_t pid, char **root) {
         return get_process_link_contents(p, root);
 }
 
+#define ENVIRONMENT_BLOCK_MAX (5U*1024U*1024U)
+
 int get_process_environ(pid_t pid, char **env) {
         _cleanup_fclose_ FILE *f = NULL;
         _cleanup_free_ char *outcome = NULL;
-        int c;
-        const char *p;
         size_t allocated = 0, sz = 0;
+        const char *p;
+        int r;
 
         assert(pid >= 0);
         assert(env);
@@ -621,23 +623,28 @@ int get_process_environ(pid_t pid, char **env) {
 
         (void) __fsetlocking(f, FSETLOCKING_BYCALLER);
 
-        while ((c = fgetc(f)) != EOF) {
+        for (;;) {
+                char c;
+
+                if (sz >= ENVIRONMENT_BLOCK_MAX)
+                        return -ENOBUFS;
+
                 if (!GREEDY_REALLOC(outcome, allocated, sz + 5))
                         return -ENOMEM;
 
+                r = safe_fgetc(f, &c);
+                if (r < 0)
+                        return r;
+                if (r == 0)
+                        break;
+
                 if (c == '\0')
                         outcome[sz++] = '\n';
                 else
                         sz += cescape_char(c, outcome + sz);
         }
 
-        if (!outcome) {
-                outcome = strdup("");
-                if (!outcome)
-                        return -ENOMEM;
-        } else
-                outcome[sz] = '\0';
-
+        outcome[sz] = '\0';
         *env = TAKE_PTR(outcome);
 
         return 0;
@@ -870,9 +877,9 @@ int kill_and_sigcont(pid_t pid, int sig) {
 int getenv_for_pid(pid_t pid, const char *field, char **ret) {
         _cleanup_fclose_ FILE *f = NULL;
         char *value = NULL;
-        bool done = false;
         const char *path;
-        size_t l;
+        size_t l, sum = 0;
+        int r;
 
         assert(pid >= 0);
         assert(field);
@@ -895,6 +902,9 @@ int getenv_for_pid(pid_t pid, const char *field, char **ret) {
                 return 1;
         }
 
+        if (!pid_is_valid(pid))
+                return -EINVAL;
+
         path = procfs_file_alloca(pid, "environ");
 
         f = fopen(path, "re");
@@ -908,24 +918,19 @@ int getenv_for_pid(pid_t pid, const char *field, char **ret) {
         (void) __fsetlocking(f, FSETLOCKING_BYCALLER);
 
         l = strlen(field);
+        for (;;) {
+                _cleanup_free_ char *line = NULL;
 
-        do {
-                char line[LINE_MAX];
-                size_t i;
-
-                for (i = 0; i < sizeof(line)-1; i++) {
-                        int c;
+                if (sum > ENVIRONMENT_BLOCK_MAX) /* Give up searching eventually */
+                        return -ENOBUFS;
 
-                        c = getc(f);
-                        if (_unlikely_(c == EOF)) {
-                                done = true;
-                                break;
-                        } else if (c == 0)
-                                break;
+                r = read_nul_string(f, LONG_LINE_MAX, &line);
+                if (r < 0)
+                        return r;
+                if (r == 0)  /* EOF */
+                        break;
 
-                        line[i] = c;
-                }
-                line[i] = 0;
+                sum += r;
 
                 if (strneq(line, field, l) && line[l] == '=') {
                         value = strdup(line + l + 1);
@@ -935,8 +940,7 @@ int getenv_for_pid(pid_t pid, const char *field, char **ret) {
                         *ret = value;
                         return 1;
                 }
-
-        } while (!done);
+        }
 
         *ret = NULL;
         return 0;
diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c
index ceba7d0ff..0f3812072 100644
--- a/src/basic/terminal-util.c
+++ b/src/basic/terminal-util.c
@@ -96,7 +96,7 @@ int read_one_char(FILE *f, char *ret, usec_t t, bool *need_nl) {
                 new_termios.c_cc[VTIME] = 0;
 
                 if (tcsetattr(fileno(f), TCSADRAIN, &new_termios) >= 0) {
-                        int c;
+                        char c;
 
                         if (t != USEC_INFINITY) {
                                 if (fd_wait_for_event(fileno(f), POLLIN, t) <= 0) {
@@ -105,17 +105,12 @@ int read_one_char(FILE *f, char *ret, usec_t t, bool *need_nl) {
                                 }
                         }
 
-                        errno = 0;
-                        c = fgetc(f);
-                        if (c == EOF)
-                                r = ferror(f) && errno > 0 ? -errno : -EIO;
-                        else
-                                r = 0;
-
+                        r = safe_fgetc(f, &c);
                         (void) tcsetattr(fileno(f), TCSADRAIN, &old_termios);
-
                         if (r < 0)
                                 return r;
+                        if (r == 0)
+                                return -EIO;
 
                         if (need_nl)
                                 *need_nl = c != '\n';
diff --git a/src/machine/machine.c b/src/machine/machine.c
index beb5b3566..4f89ac026 100644
--- a/src/machine/machine.c
+++ b/src/machine/machine.c
@@ -594,7 +594,7 @@ int machine_get_uid_shift(Machine *m, uid_t *ret) {
         uid_t uid_base, uid_shift, uid_range;
         gid_t gid_base, gid_shift, gid_range;
         _cleanup_fclose_ FILE *f = NULL;
-        int k;
+        int k, r;
 
         assert(m);
         assert(ret);
@@ -643,7 +643,10 @@ int machine_get_uid_shift(Machine *m, uid_t *ret) {
                 return -ENXIO;
 
         /* If there's more than one line, then we don't support this mapping. */
-        if (fgetc(f) != EOF)
+        r = safe_fgetc(f, NULL);
+        if (r < 0)
+                return r;
+        if (r != 0) /* Insist on EOF */
                 return -ENXIO;
 
         fclose(f);
@@ -664,7 +667,10 @@ int machine_get_uid_shift(Machine *m, uid_t *ret) {
         }
 
         /* If there's more than one line, then we don't support this file. */
-        if (fgetc(f) != EOF)
+        r = safe_fgetc(f, NULL);
+        if (r < 0)
+                return r;
+        if (r != 0) /* Insist on EOF */
                 return -ENXIO;
 
         /* If the UID and GID mapping doesn't match, we don't support this mapping. */
diff --git a/src/machine/machined-dbus.c b/src/machine/machined-dbus.c
index 9afae72ae..fea9cc263 100644
--- a/src/machine/machined-dbus.c
+++ b/src/machine/machined-dbus.c
@@ -637,8 +637,8 @@ static int clean_pool_done(Operation *operation, int ret, sd_bus_error *error) {
                 if (success) /* The resulting temporary file could not be updated, ignore it. */
                         return ret;
 
-                r = read_nul_string(f, &name);
-                if (r < 0 || isempty(name)) /* Same here... */
+                r = read_nul_string(f, LONG_LINE_MAX, &name);
+                if (r <= 0) /* Same here... */
                         return ret;
 
                 return sd_bus_error_set_errnof(error, ret, "Failed to remove image %s: %m", name);
@@ -660,10 +660,10 @@ static int clean_pool_done(Operation *operation, int ret, sd_bus_error *error) {
                 _cleanup_free_ char *name = NULL;
                 uint64_t size;
 
-                r = read_nul_string(f, &name);
+                r = read_nul_string(f, LONG_LINE_MAX, &name);
                 if (r < 0)
                         return r;
-                if (isempty(name)) /* reached the end */
+                if (r == 0) /* reached the end */
                         break;
 
                 errno = 0;
diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c
index fd0cc27de..f75b75757 100644
--- a/src/test/test-fileio.c
+++ b/src/test/test-fileio.c
@@ -610,9 +610,36 @@ static void test_tempfn(void) {
         free(ret);
 }
 
+static const char chars[] =
+        "Aąę„”\n루";
+
+static void test_fgetc(void) {
+        _cleanup_fclose_ FILE *f = NULL;
+        char c;
+
+        f = fmemopen((void*) chars, sizeof(chars), "re");
+        assert_se(f);
+
+        for (unsigned i = 0; i < sizeof(chars); i++) {
+                assert_se(safe_fgetc(f, &c) == 1);
+                assert_se(c == chars[i]);
+
+                assert_se(ungetc(c, f) != EOF);
+                assert_se(safe_fgetc(f, &c) == 1);
+                assert_se(c == chars[i]);
+
+                /* Check that ungetc doesn't care about unsigned char vs signed char */
+                assert_se(ungetc((unsigned char) c, f) != EOF);
+                assert_se(safe_fgetc(f, &c) == 1);
+                assert_se(c == chars[i]);
+        }
+
+        assert_se(safe_fgetc(f, &c) == 0);
+}
+
 static const char buffer[] =
         "Some test data\n"
-        "Some weird line\r"
+        "루Non-ascii chars: ąę„”\n"
         "terminators\r\n"
         "and even more\n\r"
         "now the same with a NUL\n\0"
@@ -631,7 +658,7 @@ static void test_read_line_one_file(FILE *f) {
         assert_se(read_line(f, (size_t) -1, &line) == 15 && streq(line, "Some test data"));
         line = mfree(line);
 
-        assert_se(read_line(f, (size_t) -1, &line) == 16 && streq(line, "Some weird line"));
+        assert_se(read_line(f, (size_t) -1, &line) > 0 && streq(line, "루Non-ascii chars: ąę„”"));
         line = mfree(line);
 
         assert_se(read_line(f, (size_t) -1, &line) == 13 && streq(line, "terminators"));
@@ -748,6 +775,40 @@ static void test_read_line4(void) {
         }
 }
 
+static void test_read_nul_string(void) {
+        static const char test[] = "string nr. 1\0"
+                "string nr. 2\n\0"
+                "empty string follows\0"
+                "\0"
+                "final string\n is empty\0"
+                "\0";
+
+        _cleanup_fclose_ FILE *f = NULL;
+        _cleanup_free_ char *s = NULL;
+
+        assert_se(f = fmemopen((void*) test, sizeof(test)-1, "r"));
+
+        assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 13 && streq_ptr(s, "string nr. 1"));
+        s = mfree(s);
+
+        assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 14 && streq_ptr(s, "string nr. 2\n"));
+        s = mfree(s);
+
+        assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 21 && streq_ptr(s, "empty string follows"));
+        s = mfree(s);
+
+        assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 1 && streq_ptr(s, ""));
+        s = mfree(s);
+
+        assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 23 && streq_ptr(s, "final string\n is empty"));
+        s = mfree(s);
+
+        assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 1 && streq_ptr(s, ""));
+        s = mfree(s);
+
+        assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 0 && streq_ptr(s, ""));
+}
+
 int main(int argc, char *argv[]) {
         test_setup_logging(LOG_DEBUG);
 
@@ -767,10 +828,12 @@ int main(int argc, char *argv[]) {
         test_search_and_fopen_nulstr();
         test_writing_tmpfile();
         test_tempfn();
+        test_fgetc();
         test_read_line();
         test_read_line2();
         test_read_line3();
         test_read_line4();
+        test_read_nul_string();
 
         return 0;
 }