From aaf057c4bbc6055040d7d2c1ec3655ff89249ebd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 15 Feb 2021 14:09:30 +0100 Subject: [PATCH 1/9] basic/env-util: add variant of strv_env_replace() that does strdup internally --- src/basic/env-util.c | 22 +++++++++++++++++++--- src/basic/env-util.h | 1 + src/core/manager.c | 12 ++---------- src/home/homectl.c | 15 ++++++++------- src/nspawn/nspawn.c | 10 +++------- src/shared/user-record.c | 12 ++---------- 6 files changed, 35 insertions(+), 37 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index a75186015e..5335c6784d 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -370,9 +370,8 @@ int strv_env_replace(char ***l, char *p) { assert(p); - /* Replace first occurrence of the env var or add a new one in the string list. Drop other occurrences. Edits - * in-place. Does not copy p. p must be a valid key=value assignment. - */ + /* Replace first occurrence of the env var or add a new one in the string list. Drop other + * occurrences. Edits in-place. Does not copy p. p must be a valid key=value assignment. */ t = strchr(p, '='); if (!t) @@ -395,6 +394,23 @@ int strv_env_replace(char ***l, char *p) { return 1; } +int strv_env_replace_strdup(char ***l, const char *assignment) { + int r; + + /* Like strv_env_replace(), but copies the argument. */ + + _cleanup_free_ char *p = strdup(assignment); + if (!p) + return -ENOMEM; + + r = strv_env_replace(l, p); + if (r < 0) + return r; + + TAKE_PTR(p); + return r; +} + char **strv_env_set(char **x, const char *p) { _cleanup_strv_free_ char **ret = NULL; size_t n, m; diff --git a/src/basic/env-util.h b/src/basic/env-util.h index 5d8afec028..922cfe1c44 100644 --- a/src/basic/env-util.h +++ b/src/basic/env-util.h @@ -46,6 +46,7 @@ char **strv_env_set(char **x, const char *p); /* New copy ... */ char **strv_env_unset(char **l, const char *p); /* In place ... */ char **strv_env_unset_many(char **l, ...) _sentinel_; int strv_env_replace(char ***l, char *p); /* In place ... */ +int strv_env_replace_strdup(char ***l, const char *assignment); char *strv_env_get_n(char **l, const char *name, size_t k, unsigned flags) _pure_; char *strv_env_get(char **x, const char *n) _pure_; diff --git a/src/core/manager.c b/src/core/manager.c index 0a7a451835..377c0b1ccc 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -641,22 +641,14 @@ int manager_default_environment(Manager *m) { /* Import locale variables LC_*= from configuration */ (void) locale_setup(&m->transient_environment); } else { - _cleanup_free_ char *k = NULL; - - /* The user manager passes its own environment - * along to its children, except for $PATH. */ + /* The user manager passes its own environment along to its children, except for $PATH. */ m->transient_environment = strv_copy(environ); if (!m->transient_environment) return log_oom(); - k = strdup("PATH=" DEFAULT_USER_PATH); - if (!k) - return log_oom(); - - r = strv_env_replace(&m->transient_environment, k); + r = strv_env_replace_strdup(&m->transient_environment, "PATH=" DEFAULT_USER_PATH); if (r < 0) return log_oom(); - TAKE_PTR(k); } sanitize_environment(m->transient_environment); diff --git a/src/home/homectl.c b/src/home/homectl.c index bf35fa03f0..dfbd12a4df 100644 --- a/src/home/homectl.c +++ b/src/home/homectl.c @@ -2511,7 +2511,7 @@ static int parse_argv(int argc, char *argv[]) { break; case ARG_SETENV: { - _cleanup_free_ char **l = NULL, **k = NULL; + _cleanup_free_ char **l = NULL; _cleanup_(json_variant_unrefp) JsonVariant *ne = NULL; JsonVariant *e; @@ -2524,7 +2524,8 @@ static int parse_argv(int argc, char *argv[]) { } if (!env_assignment_is_valid(optarg)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Environment assignment '%s' not valid.", optarg); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Environment assignment '%s' not valid.", optarg); e = json_variant_by_key(arg_identity_extra, "environment"); if (e) { @@ -2533,13 +2534,13 @@ static int parse_argv(int argc, char *argv[]) { return log_error_errno(r, "Failed to parse JSON environment field: %m"); } - k = strv_env_set(l, optarg); - if (!k) - return log_oom(); + r = strv_env_replace_strdup(&l, optarg); + if (r < 0) + return log_error_errno(r, "Failed to replace JSON environment field: %m"); - strv_sort(k); + strv_sort(l); - r = json_variant_new_array_strv(&ne, k); + r = json_variant_new_array_strv(&ne, l); if (r < 0) return log_error_errno(r, "Failed to allocate environment list JSON: %m"); diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index e1c55d0542..28c51fc76f 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -1109,17 +1109,13 @@ static int parse_argv(int argc, char *argv[]) { break; case 'E': { - char **n; - if (!env_assignment_is_valid(optarg)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Environment variable assignment '%s' is not valid.", optarg); + r = strv_env_replace_strdup(&arg_setenv, optarg); + if (r < 0) + return r; - n = strv_env_set(arg_setenv, optarg); - if (!n) - return log_oom(); - - strv_free_and_replace(arg_setenv, n); arg_settings_mask |= SETTING_ENVIRONMENT; break; } diff --git a/src/shared/user-record.c b/src/shared/user-record.c index 0b07e4427e..569102f5f2 100644 --- a/src/shared/user-record.c +++ b/src/shared/user-record.c @@ -620,7 +620,6 @@ static int json_dispatch_environment(const char *name, JsonVariant *variant, Jso return json_log(variant, flags, SYNTHETIC_ERRNO(EINVAL), "JSON field '%s' is not an array.", strna(name)); for (i = 0; i < json_variant_elements(variant); i++) { - _cleanup_free_ char *c = NULL; JsonVariant *e; const char *a; @@ -633,19 +632,12 @@ static int json_dispatch_environment(const char *name, JsonVariant *variant, Jso if (!env_assignment_is_valid(a)) return json_log(variant, flags, SYNTHETIC_ERRNO(EINVAL), "JSON field '%s' is not an array of environment variables.", strna(name)); - c = strdup(a); - if (!c) - return json_log_oom(variant, flags); - - r = strv_env_replace(&n, c); + r = strv_env_replace_strdup(&n, a); if (r < 0) return json_log_oom(variant, flags); - - c = NULL; } - strv_free_and_replace(*l, n); - return 0; + return strv_free_and_replace(*l, n); } int json_dispatch_user_disposition(const char *name, JsonVariant *variant, JsonDispatchFlags flags, void *userdata) { From b70935ac65be05087a1a694ee19488cb7fce926d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 15 Feb 2021 14:10:30 +0100 Subject: [PATCH 2/9] manager: fix memleak on repeated systemd.setenv keys --- src/core/main.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 85bf558e90..1d9dec55c3 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -452,16 +452,13 @@ static int parse_proc_cmdline_item(const char *key, const char *value, void *dat if (proc_cmdline_value_missing(key, value)) return 0; - if (env_assignment_is_valid(value)) { - char **env; - - env = strv_env_set(arg_default_environment, value); - if (!env) + if (!env_assignment_is_valid(value)) + log_warning("Environment variable assignment '%s' is not valid. Ignoring.", value); + else { + r = strv_env_replace_strdup(&arg_default_environment, value); + if (r < 0) return log_oom(); - - arg_default_environment = env; - } else - log_warning("Environment variable name '%s' is not valid. Ignoring.", value); + } } else if (proc_cmdline_key_streq(key, "systemd.machine_id")) { From ab4ab13c748b86b19ead4ab3bd39a3e4f5d6deb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 15 Feb 2021 14:17:50 +0100 Subject: [PATCH 3/9] locale: inline iterator variable declarations --- src/locale/keymap-util.c | 16 +++++----------- src/locale/localectl.c | 3 +-- src/locale/localed.c | 9 ++++----- 3 files changed, 10 insertions(+), 18 deletions(-) diff --git a/src/locale/keymap-util.c b/src/locale/keymap-util.c index 697133ad84..d0d2d30a34 100644 --- a/src/locale/keymap-util.c +++ b/src/locale/keymap-util.c @@ -66,9 +66,7 @@ static void context_free_vconsole(Context *c) { } static void context_free_locale(Context *c) { - int p; - - for (p = 0; p < _VARIABLE_LC_MAX; p++) + for (LocaleVariable p = 0; p < _VARIABLE_LC_MAX; p++) c->locale[p] = mfree(c->locale[p]); } @@ -85,9 +83,7 @@ void context_clear(Context *c) { }; void locale_simplify(char *locale[_VARIABLE_LC_MAX]) { - int p; - - for (p = VARIABLE_LANG+1; p < _VARIABLE_LC_MAX; p++) + for (LocaleVariable p = VARIABLE_LANG+1; p < _VARIABLE_LC_MAX; p++) if (isempty(locale[p]) || streq_ptr(locale[VARIABLE_LANG], locale[p])) locale[p] = mfree(locale[p]); } @@ -138,13 +134,11 @@ int locale_read_data(Context *c, sd_bus_message *m) { if (r < 0) return r; } else { - int p; - c->locale_mtime = USEC_INFINITY; context_free_locale(c); /* Fill in what we got passed from systemd. */ - for (p = 0; p < _VARIABLE_LC_MAX; p++) { + for (LocaleVariable p = 0; p < _VARIABLE_LC_MAX; p++) { const char *name; name = locale_variable_to_string(p); @@ -294,11 +288,11 @@ int x11_read_data(Context *c, sd_bus_message *m) { int locale_write_data(Context *c, char ***settings) { _cleanup_strv_free_ char **l = NULL; struct stat st; - int r, p; + int r; /* Set values will be returned as strv in *settings on success. */ - for (p = 0; p < _VARIABLE_LC_MAX; p++) { + for (LocaleVariable p = 0; p < _VARIABLE_LC_MAX; p++) { _cleanup_free_ char *t = NULL; char **u; const char *name; diff --git a/src/locale/localectl.c b/src/locale/localectl.c index c267db9edd..548ac8eb2c 100644 --- a/src/locale/localectl.c +++ b/src/locale/localectl.c @@ -55,7 +55,6 @@ static void status_info_clear(StatusInfo *info) { static void print_overridden_variables(void) { _cleanup_(locale_variables_freep) char *variables[_VARIABLE_LC_MAX] = {}; bool print_warning = true; - LocaleVariable j; int r; if (arg_transport != BUS_TRANSPORT_LOCAL) @@ -82,7 +81,7 @@ static void print_overridden_variables(void) { return; } - for (j = 0; j < _VARIABLE_LC_MAX; j++) + for (LocaleVariable j = 0; j < _VARIABLE_LC_MAX; j++) if (variables[j]) { if (print_warning) { log_warning("Warning: Settings on kernel command line override system locale settings in /etc/locale.conf.\n" diff --git a/src/locale/localed.c b/src/locale/localed.c index 97d6592cb8..9c97313edc 100644 --- a/src/locale/localed.c +++ b/src/locale/localed.c @@ -37,8 +37,7 @@ static int locale_update_system_manager(Context *c, sd_bus *bus) { _cleanup_strv_free_ char **l_set = NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - size_t c_set, c_unset; - LocaleVariable p; + size_t c_set = 0, c_unset = 0; int r; assert(bus); @@ -51,7 +50,7 @@ static int locale_update_system_manager(Context *c, sd_bus *bus) { if (!l_set) return log_oom(); - for (p = 0, c_set = 0, c_unset = 0; p < _VARIABLE_LC_MAX; p++) { + for (LocaleVariable p = 0; p < _VARIABLE_LC_MAX; p++) { const char *name; name = locale_variable_to_string(p); @@ -178,7 +177,7 @@ static int property_get_locale( Context *c = userdata; _cleanup_strv_free_ char **l = NULL; - int p, q, r; + int r; r = locale_read_data(c, reply); if (r < 0) @@ -188,7 +187,7 @@ static int property_get_locale( if (!l) return -ENOMEM; - for (p = 0, q = 0; p < _VARIABLE_LC_MAX; p++) { + for (LocaleVariable p = 0, q = 0; p < _VARIABLE_LC_MAX; p++) { char *t; const char *name; From 6f8f8688e15e9e35bfc1f3a8b99974b6eaa99eb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 15 Feb 2021 14:22:18 +0100 Subject: [PATCH 4/9] shared/user-record: inline iterator variable declarations --- src/shared/user-record.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/shared/user-record.c b/src/shared/user-record.c index 569102f5f2..b470a2f37c 100644 --- a/src/shared/user-record.c +++ b/src/shared/user-record.c @@ -608,7 +608,6 @@ static int json_dispatch_access_mode(const char *name, JsonVariant *variant, Jso static int json_dispatch_environment(const char *name, JsonVariant *variant, JsonDispatchFlags flags, void *userdata) { _cleanup_strv_free_ char **n = NULL; char ***l = userdata; - size_t i; int r; if (json_variant_is_null(variant)) { @@ -619,7 +618,7 @@ static int json_dispatch_environment(const char *name, JsonVariant *variant, Jso if (!json_variant_is_array(variant)) return json_log(variant, flags, SYNTHETIC_ERRNO(EINVAL), "JSON field '%s' is not an array.", strna(name)); - for (i = 0; i < json_variant_elements(variant); i++) { + for (size_t i = 0; i < json_variant_elements(variant); i++) { JsonVariant *e; const char *a; @@ -1463,7 +1462,7 @@ int user_group_record_mangle( JsonDispatchFlags json_flags = USER_RECORD_LOAD_FLAGS_TO_JSON_DISPATCH_FLAGS(load_flags); _cleanup_(json_variant_unrefp) JsonVariant *w = NULL; JsonVariant *array[ELEMENTSOF(mask_field) * 2]; - size_t n_retain = 0, i; + size_t n_retain = 0; UserRecordMask m = 0; int r; @@ -1488,7 +1487,7 @@ int user_group_record_mangle( return json_log(v, json_flags, SYNTHETIC_ERRNO(EINVAL), "Stripping everything from record, refusing."); /* Check if we have the special sections and if they match our flags set */ - for (i = 0; i < ELEMENTSOF(mask_field); i++) { + for (size_t i = 0; i < ELEMENTSOF(mask_field); i++) { JsonVariant *e, *k; if (FLAGS_SET(USER_RECORD_STRIP_MASK(load_flags), mask_field[i].mask)) { @@ -1527,16 +1526,15 @@ int user_group_record_mangle( r = json_variant_new_object(&w, array, n_retain); if (r < 0) return json_log(v, json_flags, r, "Failed to allocate new object: %m"); - } else { + } else /* And now check if there's anything else in the record */ - for (i = 0; i < json_variant_elements(v); i += 2) { + for (size_t i = 0; i < json_variant_elements(v); i += 2) { const char *f; bool special = false; - size_t j; assert_se(f = json_variant_string(json_variant_by_index(v, i))); - for (j = 0; j < ELEMENTSOF(mask_field); j++) + for (size_t j = 0; j < ELEMENTSOF(mask_field); j++) if (streq(f, mask_field[j].name)) { /* already covered in the loop above */ special = true; continue; @@ -1550,7 +1548,6 @@ int user_group_record_mangle( break; } } - } if (FLAGS_SET(load_flags, USER_RECORD_REQUIRE_REGULAR) && !FLAGS_SET(m, USER_RECORD_REGULAR)) return json_log(v, json_flags, SYNTHETIC_ERRNO(EBADMSG), "Record lacks basic identity fields, which are required."); From f08231fe07fe15933c3a2de399ac5deb22cf7901 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 15 Feb 2021 14:52:39 +0100 Subject: [PATCH 5/9] basic/env-util: add strv_env_assign() helper --- src/basic/env-util.c | 23 +++++++++++++++ src/basic/env-util.h | 1 + src/hostname/hostnamed.c | 20 ++----------- src/locale/keymap-util.c | 64 ++++++++-------------------------------- 4 files changed, 39 insertions(+), 69 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 5335c6784d..5f2b29d7b5 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -406,7 +406,30 @@ int strv_env_replace_strdup(char ***l, const char *assignment) { r = strv_env_replace(l, p); if (r < 0) return r; + TAKE_PTR(p); + return r; +} +int strv_env_assign(char ***l, const char *key, const char *value) { + int r; + + if (!env_name_is_valid(key)) + return -EINVAL; + + /* NULL removes assignment, "" creates an empty assignment. */ + + if (!value) { + strv_env_unset(*l, key); + return 0; + } + + char *p = strjoin(key, "=", value); + if (!p) + return -ENOMEM; + + r = strv_env_replace(l, p); + if (r < 0) + return r; TAKE_PTR(p); return r; } diff --git a/src/basic/env-util.h b/src/basic/env-util.h index 922cfe1c44..3f8995bf3d 100644 --- a/src/basic/env-util.h +++ b/src/basic/env-util.h @@ -47,6 +47,7 @@ char **strv_env_unset(char **l, const char *p); /* In place ... */ char **strv_env_unset_many(char **l, ...) _sentinel_; int strv_env_replace(char ***l, char *p); /* In place ... */ int strv_env_replace_strdup(char ***l, const char *assignment); +int strv_env_assign(char ***l, const char *key, const char *value); char *strv_env_get_n(char **l, const char *name, size_t k, unsigned flags) _pure_; char *strv_env_get(char **x, const char *n) _pure_; diff --git a/src/hostname/hostnamed.c b/src/hostname/hostnamed.c index 3b2350ed0d..34333a6530 100644 --- a/src/hostname/hostnamed.c +++ b/src/hostname/hostnamed.c @@ -423,25 +423,11 @@ static int context_write_data_machine_info(Context *c) { return r; for (int p = PROP_PRETTY_HOSTNAME; p <= PROP_LOCATION; p++) { - _cleanup_free_ char *t = NULL; - char **u; - assert(name[p]); - if (isempty(c->data[p])) { - strv_env_unset(l, name[p]); - continue; - } - - t = strjoin(name[p], "=", c->data[p]); - if (!t) - return -ENOMEM; - - u = strv_env_set(l, t); - if (!u) - return -ENOMEM; - - strv_free_and_replace(l, u); + r = strv_env_assign(&l, name[p], empty_to_null(c->data[p])); + if (r < 0) + return r; } if (strv_isempty(l)) { diff --git a/src/locale/keymap-util.c b/src/locale/keymap-util.c index d0d2d30a34..e8de1b789a 100644 --- a/src/locale/keymap-util.c +++ b/src/locale/keymap-util.c @@ -292,26 +292,12 @@ int locale_write_data(Context *c, char ***settings) { /* Set values will be returned as strv in *settings on success. */ - for (LocaleVariable p = 0; p < _VARIABLE_LC_MAX; p++) { - _cleanup_free_ char *t = NULL; - char **u; - const char *name; - - name = locale_variable_to_string(p); - assert(name); - - if (isempty(c->locale[p])) - continue; - - if (asprintf(&t, "%s=%s", name, c->locale[p]) < 0) - return -ENOMEM; - - u = strv_env_set(l, t); - if (!u) - return -ENOMEM; - - strv_free_and_replace(l, u); - } + for (LocaleVariable p = 0; p < _VARIABLE_LC_MAX; p++) + if (!isempty(c->locale[p])) { + r = strv_env_assign(&l, locale_variable_to_string(p), c->locale[p]); + if (r < 0) + return r; + } if (strv_isempty(l)) { if (unlink("/etc/locale.conf") < 0) @@ -342,39 +328,13 @@ int vconsole_write_data(Context *c) { if (r < 0 && r != -ENOENT) return r; - if (isempty(c->vc_keymap)) - l = strv_env_unset(l, "KEYMAP"); - else { - _cleanup_free_ char *s = NULL; - char **u; + r = strv_env_assign(&l, "KEYMAP", empty_to_null(c->vc_keymap)); + if (r < 0) + return r; - s = strjoin("KEYMAP=", c->vc_keymap); - if (!s) - return -ENOMEM; - - u = strv_env_set(l, s); - if (!u) - return -ENOMEM; - - strv_free_and_replace(l, u); - } - - if (isempty(c->vc_keymap_toggle)) - l = strv_env_unset(l, "KEYMAP_TOGGLE"); - else { - _cleanup_free_ char *s = NULL; - char **u; - - s = strjoin("KEYMAP_TOGGLE=", c->vc_keymap_toggle); - if (!s) - return -ENOMEM; - - u = strv_env_set(l, s); - if (!u) - return -ENOMEM; - - strv_free_and_replace(l, u); - } + r = strv_env_assign(&l, "KEYMAP_TOGGLE", empty_to_null(c->vc_keymap_toggle)); + if (r < 0) + return r; if (strv_isempty(l)) { if (unlink("/etc/vconsole.conf") < 0) From 99bfce10802d10a62ede7a21f45df5122a7eca27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 15 Feb 2021 14:55:40 +0100 Subject: [PATCH 6/9] basic/env-util: drop now-unused strv_env_set() --- src/basic/env-util.c | 28 ---------------------------- src/basic/env-util.h | 1 - src/test/test-env-util.c | 19 ------------------- 3 files changed, 48 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 5f2b29d7b5..b1313dbfff 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -434,34 +434,6 @@ int strv_env_assign(char ***l, const char *key, const char *value) { return r; } -char **strv_env_set(char **x, const char *p) { - _cleanup_strv_free_ char **ret = NULL; - size_t n, m; - char **k; - - /* Overrides the env var setting of p, returns a new copy */ - - n = strv_length(x); - m = n + 2; - if (m < n) /* overflow? */ - return NULL; - - ret = new(char*, m); - if (!ret) - return NULL; - - *ret = NULL; - k = ret; - - if (env_append(ret, &k, x) < 0) - return NULL; - - if (env_append(ret, &k, STRV_MAKE(p)) < 0) - return NULL; - - return TAKE_PTR(ret); -} - char *strv_env_get_n(char **l, const char *name, size_t k, unsigned flags) { char **i; diff --git a/src/basic/env-util.h b/src/basic/env-util.h index 3f8995bf3d..c7b72b4b47 100644 --- a/src/basic/env-util.h +++ b/src/basic/env-util.h @@ -42,7 +42,6 @@ bool strv_env_name_or_assignment_is_valid(char **l); char **strv_env_merge(size_t n_lists, ...); char **strv_env_delete(char **x, size_t n_lists, ...); /* New copy */ -char **strv_env_set(char **x, const char *p); /* New copy ... */ char **strv_env_unset(char **l, const char *p); /* In place ... */ char **strv_env_unset_many(char **l, ...) _sentinel_; int strv_env_replace(char ***l, char *p); /* In place ... */ diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index ce215fdd36..39be9532b3 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -59,24 +59,6 @@ static void test_strv_env_unset(void) { assert_se(strv_length(l) == 2); } -static void test_strv_env_set(void) { - log_info("/* %s */", __func__); - - _cleanup_strv_free_ char **l = NULL, **r = NULL; - - l = strv_new("PIEP", "SCHLUMPF=SMURFF", "NANANANA=YES"); - assert_se(l); - - r = strv_env_set(l, "WALDO=WALDO"); - assert_se(r); - - assert_se(streq(r[0], "PIEP")); - assert_se(streq(r[1], "SCHLUMPF=SMURFF")); - assert_se(streq(r[2], "NANANANA=YES")); - assert_se(streq(r[3], "WALDO=WALDO")); - assert_se(strv_length(r) == 4); -} - static void test_strv_env_merge(void) { log_info("/* %s */", __func__); @@ -378,7 +360,6 @@ int main(int argc, char *argv[]) { test_strv_env_delete(); test_strv_env_get(); test_strv_env_unset(); - test_strv_env_set(); test_strv_env_merge(); test_env_strv_get_n(); test_replace_env(false); From 73c8cc7164e8ce657acf22284ba01a3623b59565 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 15 Feb 2021 15:01:52 +0100 Subject: [PATCH 7/9] test-env-util: add tests for the two new functions --- src/test/test-env-util.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index 39be9532b3..fd121d975e 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -89,6 +89,37 @@ static void test_strv_env_merge(void) { assert_se(strv_length(r) == 5); } +static void test_strv_env_replace_strdup(void) { + log_info("/* %s */", __func__); + + _cleanup_strv_free_ char **a = NULL; + + assert_se(strv_env_replace_strdup(&a, "a=a") == 1); + assert_se(strv_env_replace_strdup(&a, "b=b") == 1); + assert_se(strv_env_replace_strdup(&a, "a=A") == 0); + + assert_se(strv_length(a) == 2); + strv_sort(a); + assert_se(streq(a[0], "a=A")); + assert_se(streq(a[1], "b=b")); +} + +static void test_strv_env_assign(void) { + log_info("/* %s */", __func__); + + _cleanup_strv_free_ char **a = NULL; + + assert_se(strv_env_assign(&a, "a", "a") == 1); + assert_se(strv_env_assign(&a, "b", "b") == 1); + assert_se(strv_env_assign(&a, "a", "A") == 0); + assert_se(strv_env_assign(&a, "b", NULL) == 0); + + assert_se(strv_env_assign(&a, "a=", "B") == -EINVAL); + + assert_se(strv_length(a) == 1); + assert_se(streq(a[0], "a=A")); +} + static void test_env_strv_get_n(void) { log_info("/* %s */", __func__); @@ -361,6 +392,8 @@ int main(int argc, char *argv[]) { test_strv_env_get(); test_strv_env_unset(); test_strv_env_merge(); + test_strv_env_replace_strdup(); + test_strv_env_assign(); test_env_strv_get_n(); test_replace_env(false); test_replace_env(true); From b230baaeb76dd1f3b6c991257331fc816ee7827b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 16 Feb 2021 16:00:17 +0100 Subject: [PATCH 8/9] shared/exec-util: fix minor memleak p was not freed on error. --- src/shared/exec-util.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/shared/exec-util.c b/src/shared/exec-util.c index 6355ce8ce4..1557086079 100644 --- a/src/shared/exec-util.c +++ b/src/shared/exec-util.c @@ -278,18 +278,12 @@ static int gather_environment_generate(int fd, void *arg) { return r; STRV_FOREACH_PAIR(x, y, new) { - char *p; - if (!env_name_is_valid(*x)) { log_warning("Invalid variable assignment \"%s=...\", ignoring.", *x); continue; } - p = strjoin(*x, "=", *y); - if (!p) - return -ENOMEM; - - r = strv_env_replace(env, p); + r = strv_env_assign(env, *x, *y); if (r < 0) return r; @@ -297,7 +291,7 @@ static int gather_environment_generate(int fd, void *arg) { return -errno; } - return r; + return 0; } static int gather_environment_collect(int fd, void *arg) { From 13734c75b58898d079867e959c8bc572d0ec75d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 16 Feb 2021 16:04:41 +0100 Subject: [PATCH 9/9] Refactor strv_env_replace() into strv_env_replace_consume() All callers of strv_env_replace() would free the argument on error. So let's follow the same pattern as with strv_consume (and similar naming) and unconditionally "use up" the argument. --- src/basic/env-file.c | 6 ++---- src/basic/env-util.c | 32 ++++++++++++-------------------- src/basic/env-util.h | 2 +- src/core/load-fragment.c | 16 +++++++--------- src/shared/serialize.c | 6 ++---- 5 files changed, 24 insertions(+), 38 deletions(-) diff --git a/src/basic/env-file.c b/src/basic/env-file.c index 99c3e3f4a3..11362d8556 100644 --- a/src/basic/env-file.c +++ b/src/basic/env-file.c @@ -385,11 +385,9 @@ static int load_env_file_push( if (!p) return -ENOMEM; - r = strv_env_replace(m, p); - if (r < 0) { - free(p); + r = strv_env_replace_consume(m, p); + if (r < 0) return r; - } if (n_pushed) (*n_pushed)++; diff --git a/src/basic/env-util.c b/src/basic/env-util.c index b1313dbfff..25c4b7c5a4 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -363,7 +363,7 @@ char **strv_env_unset_many(char **l, ...) { return l; } -int strv_env_replace(char ***l, char *p) { +int strv_env_replace_consume(char ***l, char *p) { const char *t, *name; char **f; int r; @@ -371,11 +371,15 @@ int strv_env_replace(char ***l, char *p) { assert(p); /* Replace first occurrence of the env var or add a new one in the string list. Drop other - * occurrences. Edits in-place. Does not copy p. p must be a valid key=value assignment. */ + * occurrences. Edits in-place. Does not copy p and CONSUMES p EVEN ON FAILURE. + * + * p must be a valid key=value assignment. */ t = strchr(p, '='); - if (!t) + if (!t) { + free(p); return -EINVAL; + } name = strndupa(p, t - p); @@ -387,7 +391,7 @@ int strv_env_replace(char ***l, char *p) { } /* We didn't find a match, we need to append p or create a new strv */ - r = strv_push(l, p); + r = strv_consume(l, p); if (r < 0) return r; @@ -395,24 +399,16 @@ int strv_env_replace(char ***l, char *p) { } int strv_env_replace_strdup(char ***l, const char *assignment) { - int r; + /* Like strv_env_replace_consume(), but copies the argument. */ - /* Like strv_env_replace(), but copies the argument. */ - - _cleanup_free_ char *p = strdup(assignment); + char *p = strdup(assignment); if (!p) return -ENOMEM; - r = strv_env_replace(l, p); - if (r < 0) - return r; - TAKE_PTR(p); - return r; + return strv_env_replace_consume(l, p); } int strv_env_assign(char ***l, const char *key, const char *value) { - int r; - if (!env_name_is_valid(key)) return -EINVAL; @@ -427,11 +423,7 @@ int strv_env_assign(char ***l, const char *key, const char *value) { if (!p) return -ENOMEM; - r = strv_env_replace(l, p); - if (r < 0) - return r; - TAKE_PTR(p); - return r; + return strv_env_replace_consume(l, p); } char *strv_env_get_n(char **l, const char *name, size_t k, unsigned flags) { diff --git a/src/basic/env-util.h b/src/basic/env-util.h index c7b72b4b47..347ea33e66 100644 --- a/src/basic/env-util.h +++ b/src/basic/env-util.h @@ -44,7 +44,7 @@ char **strv_env_delete(char **x, size_t n_lists, ...); /* New copy */ char **strv_env_unset(char **l, const char *p); /* In place ... */ char **strv_env_unset_many(char **l, ...) _sentinel_; -int strv_env_replace(char ***l, char *p); /* In place ... */ +int strv_env_replace_consume(char ***l, char *p); /* In place ... */ int strv_env_replace_strdup(char ***l, const char *assignment); int strv_env_assign(char ***l, const char *key, const char *value); diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index d0aeaf1c76..dfab38f77b 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -2636,7 +2636,7 @@ int config_parse_environ( } for (const char *p = rvalue;; ) { - _cleanup_free_ char *word = NULL, *k = NULL; + _cleanup_free_ char *word = NULL, *resolved = NULL; r = extract_first_word(&p, &word, NULL, EXTRACT_CUNESCAPE|EXTRACT_UNQUOTE); if (r == 0) @@ -2650,26 +2650,24 @@ int config_parse_environ( } if (u) { - r = unit_full_printf(u, word, &k); + r = unit_full_printf(u, word, &resolved); if (r < 0) { log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to resolve unit specifiers in %s, ignoring: %m", word); continue; } } else - k = TAKE_PTR(word); + resolved = TAKE_PTR(word); - if (!env_assignment_is_valid(k)) { + if (!env_assignment_is_valid(resolved)) { log_syntax(unit, LOG_WARNING, filename, line, 0, - "Invalid environment assignment, ignoring: %s", k); + "Invalid environment assignment, ignoring: %s", resolved); continue; } - r = strv_env_replace(env, k); + r = strv_env_replace_consume(env, TAKE_PTR(resolved)); if (r < 0) - return log_oom(); - - k = NULL; + return log_error_errno(r, "Failed to update environment: %m"); } } diff --git a/src/shared/serialize.c b/src/shared/serialize.c index 45f57d6a53..42fe5db3fa 100644 --- a/src/shared/serialize.c +++ b/src/shared/serialize.c @@ -175,7 +175,7 @@ int deserialize_dual_timestamp(const char *value, dual_timestamp *t) { } int deserialize_environment(const char *value, char ***list) { - _cleanup_free_ char *unescaped = NULL; + char *unescaped; int r; assert(value); @@ -187,11 +187,9 @@ int deserialize_environment(const char *value, char ***list) { if (r < 0) return log_error_errno(r, "Failed to unescape: %m"); - r = strv_env_replace(list, unescaped); + r = strv_env_replace_consume(list, unescaped); if (r < 0) return log_error_errno(r, "Failed to append environment variable: %m"); - - unescaped = NULL; /* now part of 'list' */ return 0; }