From 00d4b1e684a84cf87f5694f30ad69afc19ada166 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 25 Feb 2018 17:26:22 +0100 Subject: [PATCH 1/5] basic: shorten the code a bit in two places gcc complains that len might be used unitialized, but afaict, this is not true. --- src/basic/cgroup-util.c | 4 +--- src/basic/env-util.c | 22 +++++++--------------- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 7c0ba921104..52ae37e6b42 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -1062,13 +1062,11 @@ int cg_pid_get_path(const char *controller, pid_t pid, char **path) { continue; *e = 0; - FOREACH_WORD_SEPARATOR(word, k, l, ",", state) { + FOREACH_WORD_SEPARATOR(word, k, l, ",", state) if (k == cs && memcmp(word, controller_str, cs) == 0) { found = true; break; } - } - if (!found) continue; } diff --git a/src/basic/env-util.c b/src/basic/env-util.c index e77f9d6d3fd..ab3fc71cd2b 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -539,8 +539,7 @@ char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { assert(format); - for (e = format, i = 0; *e && i < n; e ++, i ++) { - + for (e = format, i = 0; *e && i < n; e ++, i ++) switch (state) { case WORD: @@ -554,8 +553,7 @@ char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { if (!k) return NULL; - free(r); - r = k; + free_and_replace(r, k); word = e-1; state = VARIABLE; @@ -565,8 +563,7 @@ char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { if (!k) return NULL; - free(r); - r = k; + free_and_replace(r, k); word = e+1; state = WORD; @@ -576,8 +573,7 @@ char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { if (!k) return NULL; - free(r); - r = k; + free_and_replace(r, k); word = e-1; state = VARIABLE_RAW; @@ -596,8 +592,7 @@ char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { if (!k) return NULL; - free(r); - r = k; + free_and_replace(r, k); word = e+1; state = WORD; @@ -653,8 +648,7 @@ char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { if (!k) return NULL; - free(r); - r = k; + free_and_replace(r, k); word = e+1; state = WORD; @@ -673,8 +667,7 @@ char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { if (!k) return NULL; - free(r); - r = k; + free_and_replace(r, k); word = e--; i--; @@ -682,7 +675,6 @@ char *replace_env_n(const char *format, size_t n, char **env, unsigned flags) { } break; } - } if (state == VARIABLE_RAW) { const char *t; From 3554ef51771b472145e4f7376943e7209e8a2864 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 25 Feb 2018 21:07:18 +0100 Subject: [PATCH 2/5] basic/exec-util: use _exit() to return from child --- src/basic/exec-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/exec-util.c b/src/basic/exec-util.c index 0829b3d836c..d20e09dc540 100644 --- a/src/basic/exec-util.c +++ b/src/basic/exec-util.c @@ -65,7 +65,7 @@ static int do_spawn(const char *path, char *argv[], int stdout_fd, pid_t *pid) { /* If the fd happens to be in the right place, go along with that */ if (stdout_fd != STDOUT_FILENO && dup2(stdout_fd, STDOUT_FILENO) < 0) - return -errno; + _exit(EXIT_FAILURE); (void) fd_cloexec(STDOUT_FILENO, false); } From e4de62591b41564acb3106d6cefd91934d524a9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 25 Feb 2018 21:25:33 +0100 Subject: [PATCH 3/5] basic/xattr-util: do not cast ssize_t to int MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gcc warns about unitialized memory access because it notices that ssize_t which is < 0 could be cast to positive int value. We know that this can't really happen because only -1 can be returned, but OTOH, in principle a large *positive* value cannot be cast properly. This is unlikely too, since xattrs cannot be too large, but it seems cleaner to just use a size_t to return the value and avoid the cast altoghter. This makes the code simpler and gcc is happy too. The following warning goes away: [113/1502] Compiling C object 'src/basic/basic@sta/xattr-util.c.o'. In file included from ../src/basic/alloc-util.h:28:0, from ../src/basic/xattr-util.c:30: ../src/basic/xattr-util.c: In function ‘fd_getcrtime_at’: ../src/basic/macro.h:207:60: warning: ‘b’ may be used uninitialized in this function [-Wmaybe-uninitialized] UNIQ_T(A,aq) < UNIQ_T(B,bq) ? UNIQ_T(A,aq) : UNIQ_T(B,bq); \ ^ ../src/basic/xattr-util.c:155:19: note: ‘b’ was declared here usec_t a, b; ^ --- src/basic/xattr-util.c | 28 ++++++++++++++++++---------- src/basic/xattr-util.h | 8 +++++++- src/test/test-xattr-util.c | 8 +++++--- 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/src/basic/xattr-util.c b/src/basic/xattr-util.c index 4dc5a7de5b1..a37ee4b7ef0 100644 --- a/src/basic/xattr-util.c +++ b/src/basic/xattr-util.c @@ -107,7 +107,14 @@ int fgetxattr_malloc(int fd, const char *name, char **value) { } } -ssize_t fgetxattrat_fake(int dirfd, const char *filename, const char *attribute, void *value, size_t size, int flags) { +int fgetxattrat_fake( + int dirfd, + const char *filename, + const char *attribute, + void *value, size_t size, + int flags, + size_t *ret_size) { + char fn[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int) + 1]; _cleanup_close_ int fd = -1; ssize_t l; @@ -134,7 +141,8 @@ ssize_t fgetxattrat_fake(int dirfd, const char *filename, const char *attribute, if (l < 0) return -errno; - return l; + *ret_size = l; + return 0; } static int parse_crtime(le64_t le, usec_t *usec) { @@ -154,7 +162,7 @@ int fd_getcrtime_at(int dirfd, const char *name, usec_t *ret, int flags) { struct_statx sx; usec_t a, b; le64_t le; - ssize_t n; + size_t n; int r; assert(ret); @@ -180,13 +188,13 @@ int fd_getcrtime_at(int dirfd, const char *name, usec_t *ret, int flags) { else a = USEC_INFINITY; - n = fgetxattrat_fake(dirfd, name, "user.crtime_usec", &le, sizeof(le), flags); - if (n < 0) - r = -errno; - else if (n != sizeof(le)) - r = -EIO; - else - r = parse_crtime(le, &b); + r = fgetxattrat_fake(dirfd, name, "user.crtime_usec", &le, sizeof(le), flags, &n); + if (r >= 0) { + if (n != sizeof(le)) + r = -EIO; + else + r = parse_crtime(le, &b); + } if (r < 0) { if (a != USEC_INFINITY) { *ret = a; diff --git a/src/basic/xattr-util.h b/src/basic/xattr-util.h index 1a78027aaac..e25970ee6d2 100644 --- a/src/basic/xattr-util.h +++ b/src/basic/xattr-util.h @@ -29,7 +29,13 @@ int getxattr_malloc(const char *path, const char *name, char **value, bool allow_symlink); int fgetxattr_malloc(int fd, const char *name, char **value); -ssize_t fgetxattrat_fake(int dirfd, const char *filename, const char *attribute, void *value, size_t size, int flags); +int fgetxattrat_fake( + int dirfd, + const char *filename, + const char *attribute, + void *value, size_t size, + int flags, + size_t *ret_size); int fd_setcrtime(int fd, usec_t usec); diff --git a/src/test/test-xattr-util.c b/src/test/test-xattr-util.c index 17087a282db..36b64802196 100644 --- a/src/test/test-xattr-util.c +++ b/src/test/test-xattr-util.c @@ -36,8 +36,9 @@ static void test_fgetxattrat_fake(void) { char t[] = "/var/tmp/xattrtestXXXXXX"; _cleanup_close_ int fd = -1; const char *x; - char v[3] = {}; + char v[3]; int r; + size_t size; assert_se(mkdtemp(t)); x = strjoina(t, "/test"); @@ -51,13 +52,14 @@ static void test_fgetxattrat_fake(void) { fd = open(t, O_RDONLY|O_DIRECTORY|O_CLOEXEC|O_NOCTTY); assert_se(fd >= 0); - assert_se(fgetxattrat_fake(fd, "test", "user.foo", v, 3, 0) >= 0); + assert_se(fgetxattrat_fake(fd, "test", "user.foo", v, 3, 0, &size) >= 0); + assert_se(size == 3); assert_se(memcmp(v, "bar", 3) == 0); safe_close(fd); fd = open("/", O_RDONLY|O_DIRECTORY|O_CLOEXEC|O_NOCTTY); assert_se(fd >= 0); - assert_se(fgetxattrat_fake(fd, "usr", "user.idontexist", v, 3, 0) == -ENODATA); + assert_se(fgetxattrat_fake(fd, "usr", "user.idontexist", v, 3, 0, &size) == -ENODATA); cleanup: assert_se(unlink(x) >= 0); From 80127127914b1e511d93263621b6e468953c75b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 25 Feb 2018 21:59:04 +0100 Subject: [PATCH 4/5] core/path: add one more assert --- src/core/path.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/path.c b/src/core/path.c index 8a5ec0a72f6..1893d8de458 100644 --- a/src/core/path.c +++ b/src/core/path.c @@ -80,7 +80,8 @@ int path_spec_watch(PathSpec *s, sd_event_io_handler_t handler) { (void) sd_event_source_set_description(s->event_source, "path"); - /* This assumes the path was passed through path_kill_slashes()! */ + /* This function assumes the path was passed through path_kill_slashes()! */ + assert(!strstr(s->path, "//")); for (slash = strchr(s->path, '/'); ; slash = strchr(slash+1, '/')) { char *cut = NULL; From bea28c5adb4eaeab2231f4f56b699925e408a423 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 26 Feb 2018 15:47:54 +0100 Subject: [PATCH 5/5] core/unit: voidify one snprintf statement One more follow-up for f810b631cd. --- src/core/unit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/unit.c b/src/core/unit.c index cbb30155909..c3056624ef2 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1699,7 +1699,7 @@ static void unit_status_log_starting_stopping_reloading(Unit *u, JobType t) { format = unit_get_status_message_format(u, t); DISABLE_WARNING_FORMAT_NONLITERAL; - snprintf(buf, sizeof buf, format, unit_description(u)); + (void) snprintf(buf, sizeof buf, format, unit_description(u)); REENABLE_WARNING; mid = t == JOB_START ? "MESSAGE_ID=" SD_MESSAGE_UNIT_STARTING_STR :