From fe819f569a7ebcde0a0d0d7c091c742579b28b55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 9 Jul 2021 13:17:16 +0200 Subject: [PATCH 1/9] shared/format-table: fix invalid free Coverity CID#1458108. --- src/shared/format-table.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/format-table.c b/src/shared/format-table.c index 1c28fa73fa..b298019d7c 100644 --- a/src/shared/format-table.c +++ b/src/shared/format-table.c @@ -1763,7 +1763,7 @@ static const char *table_data_format(Table *t, TableData *d, bool avoid_uppercas } case TABLE_MODE: { - _cleanup_free_ char *p; + _cleanup_free_ char *p = NULL; if (d->mode == MODE_INVALID) return "n/a"; From ddedf7ca69fef93374645aa3146941711eb30688 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 9 Jul 2021 13:31:00 +0200 Subject: [PATCH 2/9] basic/escape: use _cleanup_ in one more place Also, let's not use 'r' for a char*. --- src/basic/escape.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/basic/escape.c b/src/basic/escape.c index 2a3a0e31a1..00ab4ab73f 100644 --- a/src/basic/escape.c +++ b/src/basic/escape.c @@ -289,9 +289,11 @@ int cunescape_one(const char *p, size_t length, char32_t *ret, bool *eight_bit, } int cunescape_length_with_prefix(const char *s, size_t length, const char *prefix, UnescapeFlags flags, char **ret) { - char *r, *t; + _cleanup_free_ char *ans = NULL; + char *t; const char *f; size_t pl; + int r; assert(s); assert(ret); @@ -300,18 +302,17 @@ int cunescape_length_with_prefix(const char *s, size_t length, const char *prefi pl = strlen_ptr(prefix); - r = new(char, pl+length+1); - if (!r) + ans = new(char, pl+length+1); + if (!ans) return -ENOMEM; if (prefix) - memcpy(r, prefix, pl); + memcpy(ans, prefix, pl); - for (f = s, t = r + pl; f < s + length; f++) { + for (f = s, t = ans + pl; f < s + length; f++) { size_t remaining; bool eight_bit = false; char32_t u; - int k; remaining = s + length - f; assert(remaining > 0); @@ -329,23 +330,21 @@ int cunescape_length_with_prefix(const char *s, size_t length, const char *prefi continue; } - free(r); return -EINVAL; } - k = cunescape_one(f + 1, remaining - 1, &u, &eight_bit, flags & UNESCAPE_ACCEPT_NUL); - if (k < 0) { + r = cunescape_one(f + 1, remaining - 1, &u, &eight_bit, flags & UNESCAPE_ACCEPT_NUL); + if (r < 0) { if (flags & UNESCAPE_RELAX) { /* Invalid escape code, let's take it literal then */ *(t++) = '\\'; continue; } - free(r); - return k; + return r; } - f += k; + f += r; if (eight_bit) /* One byte? Set directly as specified */ *(t++) = u; @@ -356,8 +355,8 @@ int cunescape_length_with_prefix(const char *s, size_t length, const char *prefi *t = 0; - *ret = r; - return t - r; + *ret = TAKE_PTR(ans); + return t - *ret; } char* xescape_full(const char *s, const char *bad, size_t console_width, XEscapeFlags flags) { From e437538f35e09f8caa3de6ca905da1f134aef5bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 9 Jul 2021 13:27:03 +0200 Subject: [PATCH 3/9] tree-wide: make cunescape*() functions return ssize_t Strictly speaking, we are returning the size of a memory chunk of arbitrary size, so ssize_t is more appropriate than int. --- src/basic/escape.c | 2 +- src/basic/escape.h | 9 +++--- src/core/load-fragment.c | 14 ++++----- src/core/path.c | 9 +++--- src/core/service.c | 7 +++-- src/core/swap.c | 5 ++-- src/cryptsetup/cryptsetup.c | 8 ++--- src/import/pull-common.c | 23 +++++++------- src/libsystemd/sd-login/sd-login.c | 10 +++---- src/login/logind-inhibit.c | 23 +++++--------- src/network/networkd-dhcp4.c | 8 ++--- src/network/networkd-dhcp6.c | 8 ++--- src/network/networkd-lldp-tx.c | 8 ++--- src/nspawn/nspawn.c | 2 +- src/shared/bpf-program.c | 7 +++-- src/shared/bus-unit-util.c | 15 +++++----- src/shared/devnode-acl.c | 6 ++-- src/shared/serialize.c | 7 +++-- src/shared/udev-util.c | 13 ++++---- src/tmpfiles/tmpfiles.c | 30 ++++++++++--------- .../xdg-autostart-service.c | 7 +++-- 21 files changed, 112 insertions(+), 109 deletions(-) diff --git a/src/basic/escape.c b/src/basic/escape.c index 00ab4ab73f..6095ac43b5 100644 --- a/src/basic/escape.c +++ b/src/basic/escape.c @@ -288,7 +288,7 @@ int cunescape_one(const char *p, size_t length, char32_t *ret, bool *eight_bit, return r; } -int cunescape_length_with_prefix(const char *s, size_t length, const char *prefix, UnescapeFlags flags, char **ret) { +ssize_t cunescape_length_with_prefix(const char *s, size_t length, const char *prefix, UnescapeFlags flags, char **ret) { _cleanup_free_ char *ans = NULL; char *t; const char *f; diff --git a/src/basic/escape.h b/src/basic/escape.h index 907b572bd4..98e61789c4 100644 --- a/src/basic/escape.h +++ b/src/basic/escape.h @@ -45,14 +45,15 @@ char* cescape(const char *s); char* cescape_length(const char *s, size_t n); int cescape_char(char c, char *buf); -int cunescape_length_with_prefix(const char *s, size_t length, const char *prefix, UnescapeFlags flags, char **ret); -static inline int cunescape_length(const char *s, size_t length, UnescapeFlags flags, char **ret) { +int cunescape_one(const char *p, size_t length, char32_t *ret, bool *eight_bit, bool accept_nul); + +ssize_t cunescape_length_with_prefix(const char *s, size_t length, const char *prefix, UnescapeFlags flags, char **ret); +static inline ssize_t cunescape_length(const char *s, size_t length, UnescapeFlags flags, char **ret) { return cunescape_length_with_prefix(s, length, NULL, flags, ret); } -static inline int cunescape(const char *s, UnescapeFlags flags, char **ret) { +static inline ssize_t cunescape(const char *s, UnescapeFlags flags, char **ret) { return cunescape_length(s, strlen(s), flags, ret); } -int cunescape_one(const char *p, size_t length, char32_t *ret, bool *eight_bit, bool accept_nul); typedef enum XEscapeFlags { XESCAPE_8_BIT = 1 << 0, diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index c72f5c22da..a3ac73f823 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -1010,8 +1010,6 @@ int config_parse_exec_input_text( _cleanup_free_ char *unescaped = NULL, *resolved = NULL; ExecContext *c = data; const Unit *u = userdata; - size_t sz; - void *p; int r; assert(data); @@ -1026,9 +1024,9 @@ int config_parse_exec_input_text( return 0; } - r = cunescape(rvalue, 0, &unescaped); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, + ssize_t l = cunescape(rvalue, 0, &unescaped); + if (l < 0) { + log_syntax(unit, LOG_WARNING, filename, line, l, "Failed to decode C escaped text '%s', ignoring: %m", rvalue); return 0; } @@ -1040,7 +1038,7 @@ int config_parse_exec_input_text( return 0; } - sz = strlen(resolved); + size_t sz = strlen(resolved); if (c->stdin_data_size + sz + 1 < c->stdin_data_size || /* check for overflow */ c->stdin_data_size + sz + 1 > EXEC_STDIN_DATA_MAX) { log_syntax(unit, LOG_WARNING, filename, line, 0, @@ -1049,7 +1047,7 @@ int config_parse_exec_input_text( return 0; } - p = realloc(c->stdin_data, c->stdin_data_size + sz + 1); + void *p = realloc(c->stdin_data, c->stdin_data_size + sz + 1); if (!p) return log_oom(); @@ -4516,7 +4514,7 @@ int config_parse_set_credential( } } else { char *unescaped = NULL; - int l; + ssize_t l; /* We support escape codes here, so that users can insert trailing \n if they like */ l = cunescape(p, UNESCAPE_ACCEPT_NUL, &unescaped); diff --git a/src/core/path.c b/src/core/path.c index e098e83a31..87930f637f 100644 --- a/src/core/path.c +++ b/src/core/path.c @@ -674,13 +674,14 @@ static int path_deserialize_item(Unit *u, const char *key, const char *value, FD p->result = f; } else if (streq(key, "path-spec")) { - int previous_exists, skip = 0, r; + int previous_exists, skip = 0; _cleanup_free_ char *type_str = NULL; if (sscanf(value, "%ms %i %n", &type_str, &previous_exists, &skip) < 2) log_unit_debug(u, "Failed to parse path-spec value: %s", value); else { _cleanup_free_ char *unescaped = NULL; + ssize_t l; PathType type; PathSpec *s; @@ -690,9 +691,9 @@ static int path_deserialize_item(Unit *u, const char *key, const char *value, FD return 0; } - r = cunescape(value+skip, 0, &unescaped); - if (r < 0) { - log_unit_warning_errno(u, r, "Failed to unescape serialize path: %m"); + l = cunescape(value+skip, 0, &unescaped); + if (l < 0) { + log_unit_warning_errno(u, l, "Failed to unescape serialize path: %m"); return 0; } diff --git a/src/core/service.c b/src/core/service.c index cb0a528f0d..34aac18dd8 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2890,10 +2890,11 @@ static int service_deserialize_item(Unit *u, const char *key, const char *value, log_unit_error_errno(u, r, "Unable to deserialize current bus owner %s: %m", value); } else if (streq(key, "status-text")) { char *t; + ssize_t l; - r = cunescape(value, 0, &t); - if (r < 0) - log_unit_debug_errno(u, r, "Failed to unescape status text '%s': %m", value); + l = cunescape(value, 0, &t); + if (l < 0) + log_unit_debug_errno(u, l, "Failed to unescape status text '%s': %m", value); else free_and_replace(s->status_text, t); diff --git a/src/core/swap.c b/src/core/swap.c index 3843b19500..fabee3c1f3 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -1202,8 +1202,9 @@ static int swap_load_proc_swaps(Manager *m, bool set_flags) { continue; } - if (cunescape(dev, UNESCAPE_RELAX, &d) < 0) - return log_oom(); + ssize_t l = cunescape(dev, UNESCAPE_RELAX, &d); + if (l < 0) + return log_error_errno(l, "Failed to unescape device path: %m"); device_found_node(m, d, DEVICE_FOUND_SWAP, DEVICE_FOUND_SWAP); diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c index 0fa41b8360..8243ea108f 100644 --- a/src/cryptsetup/cryptsetup.c +++ b/src/cryptsetup/cryptsetup.c @@ -453,7 +453,6 @@ static char* disk_description(const char *path) { _cleanup_(sd_device_unrefp) sd_device *device = NULL; const char *i, *name; struct stat st; - int r; assert(path); @@ -468,14 +467,15 @@ static char* disk_description(const char *path) { if (sd_device_get_property_value(device, "ID_PART_ENTRY_NAME", &name) >= 0) { _cleanup_free_ char *unescaped = NULL; + ssize_t l; /* ID_PART_ENTRY_NAME uses \x style escaping, using libblkid's blkid_encode_string(). Let's * reverse this here to make the string more human friendly in case people embed spaces or * other weird stuff. */ - r = cunescape(name, UNESCAPE_RELAX, &unescaped); - if (r < 0) { - log_debug_errno(r, "Failed to unescape ID_PART_ENTRY_NAME, skipping device: %m"); + l = cunescape(name, UNESCAPE_RELAX, &unescaped); + if (l < 0) { + log_debug_errno(l, "Failed to unescape ID_PART_ENTRY_NAME, skipping device: %m"); return NULL; } diff --git a/src/import/pull-common.c b/src/import/pull-common.c index 75c5c7493e..cb77454e0f 100644 --- a/src/import/pull-common.c +++ b/src/import/pull-common.c @@ -34,10 +34,6 @@ int pull_find_old_etags( const char *suffix, char ***etags) { - _cleanup_free_ char *escaped_url = NULL; - _cleanup_closedir_ DIR *d = NULL; - _cleanup_strv_free_ char **l = NULL; - struct dirent *de; int r; assert(url); @@ -46,11 +42,11 @@ int pull_find_old_etags( if (!image_root) image_root = "/var/lib/machines"; - escaped_url = xescape(url, FILENAME_ESCAPE); + _cleanup_free_ char *escaped_url = xescape(url, FILENAME_ESCAPE); if (!escaped_url) return -ENOMEM; - d = opendir(image_root); + _cleanup_closedir_ DIR *d = opendir(image_root); if (!d) { if (errno == ENOENT) { *etags = NULL; @@ -60,6 +56,9 @@ int pull_find_old_etags( return -errno; } + _cleanup_strv_free_ char **ans = NULL; + struct dirent *de; + FOREACH_DIRENT_ALL(de, d, return -errno) { _cleanup_free_ char *u = NULL; const char *a, *b; @@ -93,19 +92,21 @@ int pull_find_old_etags( if (a >= b) continue; - r = cunescape_length(a, b - a, 0, &u); - if (r < 0) - return r; + ssize_t l = cunescape_length(a, b - a, 0, &u); + if (l < 0) { + assert(l >= INT8_MIN); + return l; + } if (!http_etag_is_valid(u)) continue; - r = strv_consume(&l, TAKE_PTR(u)); + r = strv_consume(&ans, TAKE_PTR(u)); if (r < 0) return r; } - *etags = TAKE_PTR(l); + *etags = TAKE_PTR(ans); return 0; } diff --git a/src/libsystemd/sd-login/sd-login.c b/src/libsystemd/sd-login/sd-login.c index f6fff204f6..3adf1cef5c 100644 --- a/src/libsystemd/sd-login/sd-login.c +++ b/src/libsystemd/sd-login/sd-login.c @@ -585,8 +585,8 @@ _public_ int sd_session_get_class(const char *session, char **class) { _public_ int sd_session_get_desktop(const char *session, char **desktop) { _cleanup_free_ char *escaped = NULL; - char *t; int r; + ssize_t l; assert_return(desktop, -EINVAL); @@ -594,11 +594,9 @@ _public_ int sd_session_get_desktop(const char *session, char **desktop) { if (r < 0) return r; - r = cunescape(escaped, 0, &t); - if (r < 0) - return r; - - *desktop = t; + l = cunescape(escaped, 0, desktop); + if (l < 0) + return l; return 0; } diff --git a/src/login/logind-inhibit.c b/src/login/logind-inhibit.c index 57198ce652..f8048f6d23 100644 --- a/src/login/logind-inhibit.c +++ b/src/login/logind-inhibit.c @@ -209,18 +209,11 @@ void inhibitor_stop(Inhibitor *i) { } int inhibitor_load(Inhibitor *i) { - - _cleanup_free_ char - *what = NULL, - *uid = NULL, - *pid = NULL, - *who = NULL, - *why = NULL, - *mode = NULL; - + _cleanup_free_ char *what = NULL, *uid = NULL, *pid = NULL, *who = NULL, *why = NULL, *mode = NULL; InhibitWhat w; InhibitMode mm; char *cc; + ssize_t l; int r; r = parse_env_file(NULL, i->state_file, @@ -255,17 +248,17 @@ int inhibitor_load(Inhibitor *i) { } if (who) { - r = cunescape(who, 0, &cc); - if (r < 0) - return log_oom(); + l = cunescape(who, 0, &cc); + if (l < 0) + return log_debug_errno(l, "Failed to unescape \"who\" of inhibitor: %m"); free_and_replace(i->who, cc); } if (why) { - r = cunescape(why, 0, &cc); - if (r < 0) - return log_oom(); + l = cunescape(why, 0, &cc); + if (l < 0) + return log_debug_errno(l, "Failed to unescape \"why\" of inhibitor: %m"); free_and_replace(i->why, cc); } diff --git a/src/network/networkd-dhcp4.c b/src/network/networkd-dhcp4.c index f80adcdbcf..1ba21fc9de 100644 --- a/src/network/networkd-dhcp4.c +++ b/src/network/networkd-dhcp4.c @@ -1735,7 +1735,7 @@ int config_parse_dhcp_mud_url( _cleanup_free_ char *unescaped = NULL; Network *network = data; - int r; + ssize_t l; assert(filename); assert(lvalue); @@ -1746,9 +1746,9 @@ int config_parse_dhcp_mud_url( return 0; } - r = cunescape(rvalue, 0, &unescaped); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, + l = cunescape(rvalue, 0, &unescaped); + if (l < 0) { + log_syntax(unit, LOG_WARNING, filename, line, l, "Failed to Failed to unescape MUD URL, ignoring: %s", rvalue); return 0; } diff --git a/src/network/networkd-dhcp6.c b/src/network/networkd-dhcp6.c index 2b72b618fc..4506932a58 100644 --- a/src/network/networkd-dhcp6.c +++ b/src/network/networkd-dhcp6.c @@ -1814,7 +1814,7 @@ int config_parse_dhcp6_mud_url( _cleanup_free_ char *unescaped = NULL; Network *network = data; - int r; + ssize_t l; assert(filename); assert(lvalue); @@ -1825,9 +1825,9 @@ int config_parse_dhcp6_mud_url( return 0; } - r = cunescape(rvalue, 0, &unescaped); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, + l = cunescape(rvalue, 0, &unescaped); + if (l < 0) { + log_syntax(unit, LOG_WARNING, filename, line, l, "Failed to Failed to unescape MUD URL, ignoring: %s", rvalue); return 0; } diff --git a/src/network/networkd-lldp-tx.c b/src/network/networkd-lldp-tx.c index 45a087b301..c2d269156e 100644 --- a/src/network/networkd-lldp-tx.c +++ b/src/network/networkd-lldp-tx.c @@ -430,15 +430,15 @@ int config_parse_lldp_mud( _cleanup_free_ char *unescaped = NULL; Network *n = data; - int r; + ssize_t l; assert(filename); assert(lvalue); assert(rvalue); - r = cunescape(rvalue, 0, &unescaped); - if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, + l = cunescape(rvalue, 0, &unescaped); + if (l < 0) { + log_syntax(unit, LOG_WARNING, filename, line, l, "Failed to Failed to unescape LLDP MUD URL, ignoring: %s", rvalue); return 0; } diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 04685fecba..aea4eb7b0b 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -1565,7 +1565,7 @@ static int parse_argv(int argc, char *argv[]) { const char *p = optarg; Credential *a; size_t i; - int l; + ssize_t l; r = extract_first_word(&p, &word, ":", EXTRACT_DONT_COALESCE_SEPARATORS); if (r == -ENOMEM) diff --git a/src/shared/bpf-program.c b/src/shared/bpf-program.c index 0f865a7168..4575bb4e2f 100644 --- a/src/shared/bpf-program.c +++ b/src/shared/bpf-program.c @@ -426,6 +426,7 @@ int bpf_program_deserialize_attachment(const char *v, FDSet *fds, BPFProgram **b _cleanup_free_ char *sfd = NULL, *sat = NULL, *unescaped = NULL; _cleanup_(bpf_program_unrefp) BPFProgram *p = NULL; _cleanup_close_ int fd = -1; + ssize_t l; int ifd, at, r; assert(v); @@ -456,9 +457,9 @@ int bpf_program_deserialize_attachment(const char *v, FDSet *fds, BPFProgram **b return at; /* The rest is the path */ - r = cunescape(v, 0, &unescaped); - if (r < 0) - return r; + l = cunescape(v, 0, &unescaped); + if (l < 0) + return l; fd = fdset_remove(fds, ifd); if (fd < 0) diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index a97ffbcea4..1ff1e7600d 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -1096,7 +1096,7 @@ static int bus_append_execute_property(sd_bus_message *m, const char *field, con r = sd_bus_message_append_array(m, 'y', decoded, decoded_size); } else { _cleanup_free_ char *unescaped = NULL; - int l; + ssize_t l; l = cunescape(p, UNESCAPE_ACCEPT_NUL, &unescaped); if (l < 0) @@ -1233,18 +1233,19 @@ static int bus_append_execute_property(sd_bus_message *m, const char *field, con if (streq(field, "StandardInputText")) { _cleanup_free_ char *unescaped = NULL; + ssize_t l; - r = cunescape(eq, 0, &unescaped); - if (r < 0) - return log_error_errno(r, "Failed to unescape text '%s': %m", eq); + l = cunescape(eq, 0, &unescaped); + if (l < 0) + return log_error_errno(l, "Failed to unescape text '%s': %m", eq); if (!strextend(&unescaped, "\n")) return log_oom(); - /* Note that we don't expand specifiers here, but that should be OK, as this is a programmatic - * interface anyway */ + /* Note that we don't expand specifiers here, but that should be OK, as this is a + * programmatic interface anyway */ - return bus_append_byte_array(m, field, unescaped, strlen(unescaped)); + return bus_append_byte_array(m, field, unescaped, l + 1); } if (streq(field, "StandardInputData")) { diff --git a/src/shared/devnode-acl.c b/src/shared/devnode-acl.c index 07e29e1019..9ea89ee920 100644 --- a/src/shared/devnode-acl.c +++ b/src/shared/devnode-acl.c @@ -220,9 +220,11 @@ int devnode_acl_all(const char *seat, if (dir) { FOREACH_DIRENT(dent, dir, return -errno) { _cleanup_free_ char *unescaped_devname = NULL; + ssize_t l; - if (cunescape(dent->d_name, UNESCAPE_RELAX, &unescaped_devname) < 0) - return -ENOMEM; + l = cunescape(dent->d_name, UNESCAPE_RELAX, &unescaped_devname); + if (l < 0) + return l; n = path_join("/dev", unescaped_devname); if (!n) diff --git a/src/shared/serialize.c b/src/shared/serialize.c index 23aac3ef52..47996b9ead 100644 --- a/src/shared/serialize.c +++ b/src/shared/serialize.c @@ -176,6 +176,7 @@ int deserialize_dual_timestamp(const char *value, dual_timestamp *t) { int deserialize_environment(const char *value, char ***list) { _cleanup_free_ char *unescaped = NULL; + ssize_t l; int r; assert(value); @@ -183,9 +184,9 @@ int deserialize_environment(const char *value, char ***list) { /* Changes the *environment strv inline. */ - r = cunescape(value, 0, &unescaped); - if (r < 0) - return log_error_errno(r, "Failed to unescape: %m"); + l = cunescape(value, 0, &unescaped); + if (l < 0) + return log_error_errno(l, "Failed to unescape: %m"); r = strv_env_replace_consume(list, TAKE_PTR(unescaped)); if (r < 0) diff --git a/src/shared/udev-util.c b/src/shared/udev-util.c index f934fc157e..27dfd11a6a 100644 --- a/src/shared/udev-util.c +++ b/src/shared/udev-util.c @@ -351,7 +351,6 @@ void log_device_uevent(sd_device *device, const char *str) { int udev_rule_parse_value(char *str, char **ret_value, char **ret_endpos) { char *i, *j; - int r; bool is_escaped; /* value must be double quotated */ @@ -373,6 +372,7 @@ int udev_rule_parse_value(char *str, char **ret_value, char **ret_endpos) { j[0] = '\0'; } else { _cleanup_free_ char *unescaped = NULL; + ssize_t l; /* find the end position of value */ for (i = str; *i != '"'; i++) { @@ -383,11 +383,12 @@ int udev_rule_parse_value(char *str, char **ret_value, char **ret_endpos) { } i[0] = '\0'; - r = cunescape_length(str, i - str, 0, &unescaped); - if (r < 0) - return r; - assert(r <= i - str); - memcpy(str, unescaped, r + 1); + l = cunescape_length(str, i - str, 0, &unescaped); + if (l < 0) + return l; + + assert(l <= i - str); + memcpy(str, unescaped, l + 1); } *ret_value = str; diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index b32b709ef5..b89555d8e0 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -2775,8 +2775,6 @@ static bool should_include_path(const char *path) { } static int specifier_expansion_from_arg(Item *i) { - _cleanup_free_ char *unescaped = NULL, *resolved = NULL; - char **xattr; int r; assert(i); @@ -2789,33 +2787,37 @@ static int specifier_expansion_from_arg(Item *i) { case CREATE_SYMLINK: case CREATE_FILE: case TRUNCATE_FILE: - case WRITE_FILE: - r = cunescape(i->argument, 0, &unescaped); - if (r < 0) - return log_error_errno(r, "Failed to unescape parameter to write: %s", i->argument); + case WRITE_FILE: { + _cleanup_free_ char *unescaped = NULL, *resolved = NULL; + ssize_t l; + + l = cunescape(i->argument, 0, &unescaped); + if (l < 0) + return log_error_errno(l, "Failed to unescape parameter to write: %s", i->argument); r = specifier_printf(unescaped, PATH_MAX-1, specifier_table, arg_root, NULL, &resolved); if (r < 0) return r; - free_and_replace(i->argument, resolved); - break; - + return free_and_replace(i->argument, resolved); + } case SET_XATTR: - case RECURSIVE_SET_XATTR: + case RECURSIVE_SET_XATTR: { + char **xattr; STRV_FOREACH(xattr, i->xattrs) { + _cleanup_free_ char *resolved = NULL; + r = specifier_printf(*xattr, SIZE_MAX, specifier_table, arg_root, NULL, &resolved); if (r < 0) return r; free_and_replace(*xattr, resolved); } - break; - + return 0; + } default: - break; + return 0; } - return 0; } static int patch_var_run(const char *fname, unsigned line, char **path) { diff --git a/src/xdg-autostart-generator/xdg-autostart-service.c b/src/xdg-autostart-generator/xdg-autostart-service.c index fe73bfe9db..d6e90302fd 100644 --- a/src/xdg-autostart-generator/xdg-autostart-service.c +++ b/src/xdg-autostart-generator/xdg-autostart-service.c @@ -397,10 +397,11 @@ int xdg_autostart_format_exec_start( first_arg = true; for (i = n = 0; exec_split[i]; i++) { _cleanup_free_ char *c = NULL, *raw = NULL, *p = NULL, *escaped = NULL, *quoted = NULL; + ssize_t l; - r = cunescape(exec_split[i], 0, &c); - if (r < 0) - return log_debug_errno(r, "Failed to unescape '%s': %m", exec_split[i]); + l = cunescape(exec_split[i], 0, &c); + if (l < 0) + return log_debug_errno(l, "Failed to unescape '%s': %m", exec_split[i]); if (first_arg) { _cleanup_free_ char *executable = NULL; From 12d729b2ec6b3f4a7b014b3ba47444f7eb6e335a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 9 Jul 2021 14:30:15 +0200 Subject: [PATCH 4/9] nspawn: inline one iterator variable declaration --- src/nspawn/nspawn.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index aea4eb7b0b..ee3d594031 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -1564,7 +1564,6 @@ static int parse_argv(int argc, char *argv[]) { _cleanup_free_ char *word = NULL, *data = NULL; const char *p = optarg; Credential *a; - size_t i; ssize_t l; r = extract_first_word(&p, &word, ":", EXTRACT_DONT_COALESCE_SEPARATORS); @@ -1578,7 +1577,7 @@ static int parse_argv(int argc, char *argv[]) { if (!credential_name_valid(word)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Credential name is not valid: %s", word); - for (i = 0; i < arg_n_credentials; i++) + for (size_t i = 0; i < arg_n_credentials; i++) if (streq(arg_credentials[i].id, word)) return log_error_errno(SYNTHETIC_ERRNO(EEXIST), "Duplicate credential '%s', refusing.", word); From 2744c7bb0176dc6b86a69acd4c449ea9e269e097 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 9 Jul 2021 14:31:04 +0200 Subject: [PATCH 5/9] xdg-autostart: minor refactoring We can't say free_and_replace(exec_split[n++], quoted), because the the argument is evaluated multiple times. But I think that this form is still easier to read. --- src/xdg-autostart-generator/xdg-autostart-service.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/xdg-autostart-generator/xdg-autostart-service.c b/src/xdg-autostart-generator/xdg-autostart-service.c index d6e90302fd..f21d3f54b1 100644 --- a/src/xdg-autostart-generator/xdg-autostart-service.c +++ b/src/xdg-autostart-generator/xdg-autostart-service.c @@ -416,8 +416,8 @@ int xdg_autostart_format_exec_start( if (!escaped) return log_oom(); - free(exec_split[n]); - exec_split[n++] = TAKE_PTR(escaped); + free_and_replace(exec_split[n], escaped); + n++; continue; } @@ -457,8 +457,8 @@ int xdg_autostart_format_exec_start( if (!quoted) return log_oom(); - free(exec_split[n]); - exec_split[n++] = TAKE_PTR(quoted); + free_and_replace(exec_split[n], quoted); + n++; } for (; exec_split[n]; n++) exec_split[n] = mfree(exec_split[n]); From 1421705d9a8233a09f6bd7c25436ed48e640a368 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 9 Jul 2021 14:38:23 +0200 Subject: [PATCH 6/9] core: drop unnecessary initialization cunescape() sets output on success, so initialization is not necessary. There was no comment, but I think they may have been added because the compiler wasn't convinced that the return value is non-negative on success. It could have been confused by the int return type on escape*(), which was changed by the one of preceeding commits to ssize_t, or by the length calculation, so add an assert to help the compiler. For some reason coverity thinks the output can be leaked here (CID #1458111). I don't see how. --- src/basic/escape.c | 1 + src/core/load-fragment.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/basic/escape.c b/src/basic/escape.c index 6095ac43b5..9e75c51dfc 100644 --- a/src/basic/escape.c +++ b/src/basic/escape.c @@ -355,6 +355,7 @@ ssize_t cunescape_length_with_prefix(const char *s, size_t length, const char *p *t = 0; + assert(t >= ans); /* Let static analyzers know that the answer is non-negative. */ *ret = TAKE_PTR(ans); return t - *ret; } diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index a3ac73f823..6eefaaf27d 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -4513,7 +4513,7 @@ int config_parse_set_credential( return 0; } } else { - char *unescaped = NULL; + char *unescaped; ssize_t l; /* We support escape codes here, so that users can insert trailing \n if they like */ From bc1f27ff5515d8e35b881281d6f2f1a7ff1fed27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 9 Jul 2021 14:45:41 +0200 Subject: [PATCH 7/9] creds: drop unnecessary initialization Coverity also thinks a leak happens here, CID #1458112. This seems wrong, but let's add an assert, maybe that'll help. --- src/basic/hexdecoct.c | 1 + src/creds/creds.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/basic/hexdecoct.c b/src/basic/hexdecoct.c index 172ae8a921..8c83a0e71a 100644 --- a/src/basic/hexdecoct.c +++ b/src/basic/hexdecoct.c @@ -645,6 +645,7 @@ ssize_t base64mem_full( *z = 0; *out = r; + assert(z >= r); /* Let static analyzers know that the answer is non-negative. */ return z - r; } diff --git a/src/creds/creds.c b/src/creds/creds.c index 50de685cd4..19893fb361 100644 --- a/src/creds/creds.c +++ b/src/creds/creds.c @@ -193,7 +193,7 @@ static int transcode( switch (arg_transcode) { case TRANSCODE_BASE64: { - char *buf = NULL; + char *buf; ssize_t l; l = base64mem_full(input, input_size, 79, &buf); From 89fa9a6b7b2505aa2ce18febf1e28e79510dfec2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 9 Jul 2021 15:01:00 +0200 Subject: [PATCH 8/9] networkd: add shared parser for mud urls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The same buggy code was triplicated… --- src/network/networkd-dhcp4.c | 30 +++------------------------ src/network/networkd-dhcp6.c | 29 +++----------------------- src/network/networkd-lldp-tx.c | 27 ++++-------------------- src/network/networkd-util.c | 38 +++++++++++++++++++++++++++++++++- src/network/networkd-util.h | 11 ++++++++++ 5 files changed, 58 insertions(+), 77 deletions(-) diff --git a/src/network/networkd-dhcp4.c b/src/network/networkd-dhcp4.c index 1ba21fc9de..8609a63b83 100644 --- a/src/network/networkd-dhcp4.c +++ b/src/network/networkd-dhcp4.c @@ -5,7 +5,6 @@ #include #include -#include "escape.h" #include "alloc-util.h" #include "dhcp-client-internal.h" #include "hostname-setup.h" @@ -26,7 +25,6 @@ #include "string-table.h" #include "strv.h" #include "sysctl-util.h" -#include "web-util.h" static int dhcp4_request_address_and_routes(Link *link, bool announce); static int dhcp4_remove_all(Link *link); @@ -1733,34 +1731,12 @@ int config_parse_dhcp_mud_url( void *data, void *userdata) { - _cleanup_free_ char *unescaped = NULL; Network *network = data; - ssize_t l; - assert(filename); - assert(lvalue); - assert(rvalue); + assert(network); - if (isempty(rvalue)) { - network->dhcp_mudurl = mfree(network->dhcp_mudurl); - return 0; - } - - l = cunescape(rvalue, 0, &unescaped); - if (l < 0) { - log_syntax(unit, LOG_WARNING, filename, line, l, - "Failed to Failed to unescape MUD URL, ignoring: %s", rvalue); - return 0; - } - - if (!http_url_is_valid(unescaped) || strlen(unescaped) > 255) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Failed to parse MUD URL '%s', ignoring: %m", rvalue); - - return 0; - } - - return free_and_strdup_warn(&network->dhcp_mudurl, unescaped); + return config_parse_mud_url(unit, filename, line, section, section_line, lvalue, ltype, rvalue, + &network->dhcp_mudurl); } int config_parse_dhcp_fallback_lease_lifetime(const char *unit, diff --git a/src/network/networkd-dhcp6.c b/src/network/networkd-dhcp6.c index 4506932a58..367be0a756 100644 --- a/src/network/networkd-dhcp6.c +++ b/src/network/networkd-dhcp6.c @@ -9,7 +9,6 @@ #include "sd-dhcp6-client.h" -#include "escape.h" #include "hashmap.h" #include "hostname-setup.h" #include "hostname-util.h" @@ -24,7 +23,6 @@ #include "string-table.h" #include "string-util.h" #include "radv-internal.h" -#include "web-util.h" bool link_dhcp6_with_address_enabled(Link *link) { if (!link_dhcp6_enabled(link)) @@ -1812,33 +1810,12 @@ int config_parse_dhcp6_mud_url( void *data, void *userdata) { - _cleanup_free_ char *unescaped = NULL; Network *network = data; - ssize_t l; - assert(filename); - assert(lvalue); - assert(rvalue); + assert(network); - if (isempty(rvalue)) { - network->dhcp6_mudurl = mfree(network->dhcp6_mudurl); - return 0; - } - - l = cunescape(rvalue, 0, &unescaped); - if (l < 0) { - log_syntax(unit, LOG_WARNING, filename, line, l, - "Failed to Failed to unescape MUD URL, ignoring: %s", rvalue); - return 0; - } - - if (!http_url_is_valid(unescaped) || strlen(unescaped) > UINT8_MAX) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Failed to parse MUD URL '%s', ignoring: %m", rvalue); - return 0; - } - - return free_and_replace(network->dhcp6_mudurl, unescaped); + return config_parse_mud_url(unit, filename, line, section, section_line, lvalue, ltype, rvalue, + &network->dhcp6_mudurl); } DEFINE_CONFIG_PARSE_ENUM(config_parse_dhcp6_client_start_mode, dhcp6_client_start_mode, DHCP6ClientStartMode, diff --git a/src/network/networkd-lldp-tx.c b/src/network/networkd-lldp-tx.c index c2d269156e..e6e0b5ee54 100644 --- a/src/network/networkd-lldp-tx.c +++ b/src/network/networkd-lldp-tx.c @@ -7,7 +7,6 @@ #include "alloc-util.h" #include "env-file.h" -#include "escape.h" #include "fd-util.h" #include "hostname-util.h" #include "missing_network.h" @@ -21,7 +20,6 @@ #include "string-util.h" #include "strv.h" #include "unaligned.h" -#include "web-util.h" /* The LLDP spec calls this "txFastInit", see 9.2.5.19 */ #define LLDP_TX_FAST_INIT 4U @@ -428,29 +426,12 @@ int config_parse_lldp_mud( void *data, void *userdata) { - _cleanup_free_ char *unescaped = NULL; - Network *n = data; - ssize_t l; + Network *network = data; - assert(filename); - assert(lvalue); - assert(rvalue); + assert(network); - l = cunescape(rvalue, 0, &unescaped); - if (l < 0) { - log_syntax(unit, LOG_WARNING, filename, line, l, - "Failed to Failed to unescape LLDP MUD URL, ignoring: %s", rvalue); - return 0; - } - - if (!http_url_is_valid(unescaped) || strlen(unescaped) > 255) { - log_syntax(unit, LOG_WARNING, filename, line, 0, - "Failed to parse LLDP MUD URL '%s', ignoring: %m", rvalue); - - return 0; - } - - return free_and_replace(n->lldp_mud, unescaped); + return config_parse_mud_url(unit, filename, line, section, section_line, lvalue, ltype, rvalue, + &network->lldp_mud); } static const char * const lldp_emit_table[_LLDP_EMIT_MAX] = { diff --git a/src/network/networkd-util.c b/src/network/networkd-util.c index 43ddf124d4..a6b9a7f8b6 100644 --- a/src/network/networkd-util.c +++ b/src/network/networkd-util.c @@ -2,12 +2,13 @@ #include "condition.h" #include "conf-parser.h" +#include "escape.h" #include "networkd-link.h" #include "networkd-util.h" #include "parse-util.h" #include "string-table.h" #include "string-util.h" -#include "util.h" +#include "web-util.h" static const char* const address_family_table[_ADDRESS_FAMILY_MAX] = { [ADDRESS_FAMILY_NO] = "no", @@ -161,6 +162,41 @@ int config_parse_ip_masquerade( return 0; } +int config_parse_mud_url( + const char *unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *lvalue, + int ltype, + const char *rvalue, + char **ret) { + + assert(filename); + assert(lvalue); + assert(rvalue); + assert(ret); + + _cleanup_free_ char *unescaped = NULL; + ssize_t l; + + l = cunescape(rvalue, 0, &unescaped); + if (l < 0) { + log_syntax(unit, LOG_WARNING, filename, line, l, + "Failed to unescape MUD URL, ignoring: %s", rvalue); + return 0; + } + + if (l > UINT8_MAX || !http_url_is_valid(unescaped)) { + log_syntax(unit, LOG_WARNING, filename, line, 0, + "Invalid MUD URL, ignoring: %s", rvalue); + return 0; + } + + return free_and_replace(*ret, unescaped); +} + /* Router lifetime can be set with netlink interface since kernel >= 4.5 * so for the supported kernel we don't need to expire routes in userspace */ int kernel_route_expiration_supported(void) { diff --git a/src/network/networkd-util.h b/src/network/networkd-util.h index 62f38bcdc2..7471f18970 100644 --- a/src/network/networkd-util.h +++ b/src/network/networkd-util.h @@ -23,6 +23,17 @@ CONFIG_PARSER_PROTOTYPE(config_parse_link_local_address_family); CONFIG_PARSER_PROTOTYPE(config_parse_address_family_with_kernel); CONFIG_PARSER_PROTOTYPE(config_parse_ip_masquerade); +int config_parse_mud_url( + const char *unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *lvalue, + int ltype, + const char *rvalue, + char **ret); + const char *address_family_to_string(AddressFamily b) _const_; AddressFamily address_family_from_string(const char *s) _pure_; From d1829af943d0d47c745c3ce789690493e7b8107a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 9 Jul 2021 15:29:47 +0200 Subject: [PATCH 9/9] creds: fix leak of arg_tpm2_device IIUC, "auto" is the same as NULL. There is no need to strdup() anything. Coverity CID#1458113. --- src/creds/creds.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/src/creds/creds.c b/src/creds/creds.c index 19893fb361..95d5bf0cfa 100644 --- a/src/creds/creds.c +++ b/src/creds/creds.c @@ -704,30 +704,20 @@ static int parse_argv(int argc, char *argv[]) { arg_with_key = CRED_AES256_GCM_BY_TPM2_HMAC; break; - case ARG_TPM2_DEVICE: { - _cleanup_free_ char *device = NULL; - + case ARG_TPM2_DEVICE: if (streq(optarg, "list")) return tpm2_list_devices(); - if (!streq(optarg, "auto")) { - device = strdup(optarg); - if (!device) - return log_oom(); - } - - arg_tpm2_device = TAKE_PTR(device); + arg_tpm2_device = streq(optarg, "auto") ? NULL : optarg; break; - } - - case ARG_TPM2_PCRS: { - uint32_t mask; + case ARG_TPM2_PCRS: if (isempty(optarg)) { arg_tpm2_pcr_mask = 0; break; } + uint32_t mask; r = tpm2_parse_pcrs(optarg, &mask); if (r < 0) return r; @@ -738,7 +728,6 @@ static int parse_argv(int argc, char *argv[]) { arg_tpm2_pcr_mask |= mask; break; - } case ARG_NAME: if (isempty(optarg)) {