diff options
| -rw-r--r-- | TODO | 27 | ||||
| -rw-r--r-- | src/basic/fileio.c | 108 | ||||
| -rw-r--r-- | src/basic/fileio.h | 16 | ||||
| -rw-r--r-- | src/basic/process-util.c | 62 | ||||
| -rw-r--r-- | src/basic/terminal-util.c | 13 | ||||
| -rw-r--r-- | src/machine/machine.c | 12 | ||||
| -rw-r--r-- | src/machine/machined-dbus.c | 8 | ||||
| -rw-r--r-- | src/test/test-fileio.c | 67 |
8 files changed, 177 insertions, 136 deletions
@@ -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; } |