From 1ad6e8b302e87b6891a2bfc35ad397b0afe3d940 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 31 Oct 2018 15:49:19 +0100 Subject: [PATCH] core: split environment block mantained by PID 1's Manager object in two This splits the "environment" field of Manager into two: transient_environment and client_environment. The former is generated from configuration file, kernel cmdline, environment generators. The latter is the one the user can control with "systemctl set-environment" and similar. Both sets are merged transparently whenever needed. Separating the two sets has the benefit that we can safely flush out the former while keeping the latter during daemon reload cycles, so that env var settings from env generators or configuration files do not accumulate, but dynamic API changes are kept around. Note that this change is not entirely transparent to users: if the user first uses "set-environment" to override a transient variable, and then uses "unset-environment" to unset it again things will revert to the original transient variable now, while previously the variable was fully removed. This change in behaviour should not matter too much though I figure. Fixes: #9972 --- src/core/dbus-manager.c | 32 +++++++++++++-- src/core/main.c | 2 +- src/core/manager.c | 91 ++++++++++++++++++++++++++++++----------- src/core/manager.h | 8 +++- src/core/mount.c | 4 +- src/core/service.c | 4 +- src/core/socket.c | 4 +- src/core/swap.c | 4 +- src/core/unit.c | 11 ++++- src/core/unit.h | 2 +- 10 files changed, 125 insertions(+), 37 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 480a143091..677d9c3226 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -216,6 +216,30 @@ static int property_get_progress( return sd_bus_message_append(reply, "d", d); } +static int property_get_environment( + sd_bus *bus, + const char *path, + const char *interface, + const char *property, + sd_bus_message *reply, + void *userdata, + sd_bus_error *error) { + + _cleanup_strv_free_ char **l = NULL; + Manager *m = userdata; + int r; + + assert(bus); + assert(reply); + assert(m); + + r = manager_get_effective_environment(m, &l); + if (r < 0) + return r; + + return sd_bus_message_append_strv(reply, l); +} + static int property_get_show_status( sd_bus *bus, const char *path, @@ -1578,7 +1602,7 @@ static int method_set_environment(sd_bus_message *message, void *userdata, sd_bu if (r == 0) return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */ - r = manager_environment_add(m, NULL, plus); + r = manager_client_environment_modify(m, NULL, plus); if (r < 0) return r; @@ -1610,7 +1634,7 @@ static int method_unset_environment(sd_bus_message *message, void *userdata, sd_ if (r == 0) return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */ - r = manager_environment_add(m, minus, NULL); + r = manager_client_environment_modify(m, minus, NULL); if (r < 0) return r; @@ -1648,7 +1672,7 @@ static int method_unset_and_set_environment(sd_bus_message *message, void *userd if (r == 0) return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */ - r = manager_environment_add(m, minus, plus); + r = manager_client_environment_modify(m, minus, plus); if (r < 0) return r; @@ -2432,7 +2456,7 @@ const sd_bus_vtable bus_manager_vtable[] = { SD_BUS_PROPERTY("NInstalledJobs", "u", bus_property_get_unsigned, offsetof(Manager, n_installed_jobs), 0), SD_BUS_PROPERTY("NFailedJobs", "u", bus_property_get_unsigned, offsetof(Manager, n_failed_jobs), 0), SD_BUS_PROPERTY("Progress", "d", property_get_progress, 0, 0), - SD_BUS_PROPERTY("Environment", "as", NULL, offsetof(Manager, environment), 0), + SD_BUS_PROPERTY("Environment", "as", property_get_environment, 0, 0), SD_BUS_PROPERTY("ConfirmSpawn", "b", bus_property_get_bool, offsetof(Manager, confirm_spawn), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("ShowStatus", "b", property_get_show_status, 0, 0), SD_BUS_PROPERTY("UnitPath", "as", NULL, offsetof(Manager, lookup_paths.search_path), SD_BUS_VTABLE_PROPERTY_CONST), diff --git a/src/core/main.c b/src/core/main.c index 807f5457c2..8a09ab67b5 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -761,7 +761,7 @@ static void set_manager_defaults(Manager *m) { m->default_tasks_max = arg_default_tasks_max; manager_set_default_rlimits(m, arg_default_rlimit); - manager_environment_add(m, NULL, arg_default_environment); + manager_transient_environment_add(m, arg_default_environment); } static void set_manager_settings(Manager *m) { diff --git a/src/core/manager.c b/src/core/manager.c index adf0319701..bee415c725 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -572,12 +572,11 @@ static int manager_setup_signals(Manager *m) { return 0; } -static void manager_sanitize_environment(Manager *m) { - assert(m); +static char** sanitize_environment(char **l) { /* Let's remove some environment variables that we need ourselves to communicate with our clients */ strv_env_unset_many( - m->environment, + l, "EXIT_CODE", "EXIT_STATUS", "INVOCATION_ID", @@ -596,12 +595,16 @@ static void manager_sanitize_environment(Manager *m) { NULL); /* Let's order the environment alphabetically, just to make it pretty */ - strv_sort(m->environment); + strv_sort(l); + + return l; } static int manager_default_environment(Manager *m) { assert(m); + m->transient_environment = strv_free(m->transient_environment); + if (MANAGER_IS_SYSTEM(m)) { /* The system manager always starts with a clean * environment for its children. It does not import @@ -610,20 +613,19 @@ static int manager_default_environment(Manager *m) { * The initial passed environment is untouched to keep * /proc/self/environ valid; it is used for tagging * the init process inside containers. */ - m->environment = strv_new("PATH=" DEFAULT_PATH, - NULL); + m->transient_environment = strv_new("PATH=" DEFAULT_PATH); /* Import locale variables LC_*= from configuration */ - locale_setup(&m->environment); + (void) locale_setup(&m->transient_environment); } else /* The user manager passes its own environment * along to its children. */ - m->environment = strv_copy(environ); + m->transient_environment = strv_copy(environ); - if (!m->environment) + if (!m->transient_environment) return -ENOMEM; - manager_sanitize_environment(m); + sanitize_environment(m->transient_environment); return 0; } @@ -1346,7 +1348,8 @@ Manager* manager_free(Manager *m) { free(m->notify_socket); lookup_paths_free(&m->lookup_paths); - strv_free(m->environment); + strv_free(m->transient_environment); + strv_free(m->client_environment); hashmap_free(m->cgroup_unit); set_free_free(m->unit_path_cache); @@ -3145,7 +3148,7 @@ int manager_serialize( } if (!switching_root) - (void) serialize_strv(f, "env", m->environment); + (void) serialize_strv(f, "env", m->client_environment); if (m->notify_fd >= 0) { r = serialize_fd(f, fds, "notify-fd", m->notify_fd); @@ -3378,7 +3381,7 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { manager_override_log_target(m, target); } else if (startswith(l, "env=")) { - r = deserialize_environment(l + 4, &m->environment); + r = deserialize_environment(l + 4, &m->client_environment); if (r < 0) log_notice_errno(r, "Failed to parse environment entry: \"%s\", ignoring: %m", l); @@ -3801,7 +3804,11 @@ static const char* user_env_generator_binary_paths[] = { static int manager_run_environment_generators(Manager *m) { char **tmp = NULL; /* this is only used in the forked process, no cleanup here */ const char **paths; - void* args[] = {&tmp, &tmp, &m->environment}; + void* args[] = { + [STDOUT_GENERATE] = &tmp, + [STDOUT_COLLECT] = &tmp, + [STDOUT_CONSUME] = &m->transient_environment, + }; if (MANAGER_IS_TEST_RUN(m) && !(m->test_run_flags & MANAGER_TEST_RUN_ENV_GENERATORS)) return 0; @@ -3811,7 +3818,7 @@ static int manager_run_environment_generators(Manager *m) { if (!generator_path_any(paths)) return 0; - return execute_directories(paths, DEFAULT_TIMEOUT_USEC, gather_environment, args, NULL, m->environment); + return execute_directories(paths, DEFAULT_TIMEOUT_USEC, gather_environment, args, NULL, m->transient_environment); } static int manager_run_generators(Manager *m) { @@ -3845,7 +3852,7 @@ static int manager_run_generators(Manager *m) { RUN_WITH_UMASK(0022) (void) execute_directories((const char* const*) paths, DEFAULT_TIMEOUT_USEC, - NULL, NULL, (char**) argv, m->environment); + NULL, NULL, (char**) argv, m->transient_environment); r = 0; @@ -3854,11 +3861,36 @@ finish: return r; } -int manager_environment_add(Manager *m, char **minus, char **plus) { - char **a = NULL, **b = NULL, **l; +int manager_transient_environment_add(Manager *m, char **plus) { + char **a; + assert(m); - l = m->environment; + if (strv_isempty(plus)) + return 0; + + a = strv_env_merge(2, m->transient_environment, plus); + if (!a) + return -ENOMEM; + + sanitize_environment(a); + + return strv_free_and_replace(m->transient_environment, a); +} + +int manager_client_environment_modify( + Manager *m, + char **minus, + char **plus) { + + char **a = NULL, **b = NULL, **l; + + assert(m); + + if (strv_isempty(minus) && strv_isempty(plus)) + return 0; + + l = m->client_environment; if (!strv_isempty(minus)) { a = strv_env_delete(l, 1, minus); @@ -3878,16 +3910,29 @@ int manager_environment_add(Manager *m, char **minus, char **plus) { l = b; } - if (m->environment != l) - strv_free(m->environment); + if (m->client_environment != l) + strv_free(m->client_environment); + if (a != l) strv_free(a); if (b != l) strv_free(b); - m->environment = l; - manager_sanitize_environment(m); + m->client_environment = sanitize_environment(l); + return 0; +} +int manager_get_effective_environment(Manager *m, char ***ret) { + char **l; + + assert(m); + assert(ret); + + l = strv_env_merge(2, m->transient_environment, m->client_environment); + if (!l) + return -ENOMEM; + + *ret = l; return 0; } diff --git a/src/core/manager.h b/src/core/manager.h index 1daa9f2308..45fa5d35d8 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -212,7 +212,8 @@ struct Manager { LookupPaths lookup_paths; Set *unit_path_cache; - char **environment; + char **transient_environment; /* The environment, as determined from config files, kernel cmdline and environment generators */ + char **client_environment; /* Environment variables created by clients through the bus API */ usec_t runtime_watchdog; usec_t shutdown_watchdog; @@ -442,7 +443,10 @@ void manager_clear_jobs(Manager *m); unsigned manager_dispatch_load_queue(Manager *m); -int manager_environment_add(Manager *m, char **minus, char **plus); +int manager_transient_environment_add(Manager *m, char **plus); +int manager_client_environment_modify(Manager *m, char **minus, char **plus); +int manager_get_effective_environment(Manager *m, char ***ret); + int manager_set_default_rlimits(Manager *m, struct rlimit **default_rlimit); int manager_loop(Manager *m); diff --git a/src/core/mount.c b/src/core/mount.c index dafd6ea90b..14f1fa9e50 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -769,7 +769,9 @@ static int mount_spawn(Mount *m, ExecCommand *c, pid_t *_pid) { if (r < 0) return r; - unit_set_exec_params(UNIT(m), &exec_params); + r = unit_set_exec_params(UNIT(m), &exec_params); + if (r < 0) + return r; r = exec_spawn(UNIT(m), c, diff --git a/src/core/service.c b/src/core/service.c index f6f3ecc0e6..8fc8d1ff94 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1523,7 +1523,9 @@ static int service_spawn( } } - unit_set_exec_params(UNIT(s), &exec_params); + r = unit_set_exec_params(UNIT(s), &exec_params); + if (r < 0) + return r; final_env = strv_env_merge(2, exec_params.environment, our_env, NULL); if (!final_env) diff --git a/src/core/socket.c b/src/core/socket.c index d54dadbacf..ad8b9b6a41 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -1882,7 +1882,9 @@ static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) { if (r < 0) return r; - unit_set_exec_params(UNIT(s), &exec_params); + r = unit_set_exec_params(UNIT(s), &exec_params); + if (r < 0) + return r; r = exec_spawn(UNIT(s), c, diff --git a/src/core/swap.c b/src/core/swap.c index dd1ef8e88e..429b2b1062 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -618,7 +618,9 @@ static int swap_spawn(Swap *s, ExecCommand *c, pid_t *_pid) { if (r < 0) goto fail; - unit_set_exec_params(UNIT(s), &exec_params); + r = unit_set_exec_params(UNIT(s), &exec_params); + if (r < 0) + goto fail; r = exec_spawn(UNIT(s), c, diff --git a/src/core/unit.c b/src/core/unit.c index e35f7f26d1..c0ac8e5c3a 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -5023,12 +5023,17 @@ int unit_acquire_invocation_id(Unit *u) { return 0; } -void unit_set_exec_params(Unit *u, ExecParameters *p) { +int unit_set_exec_params(Unit *u, ExecParameters *p) { + int r; + assert(u); assert(p); /* Copy parameters from manager */ - p->environment = u->manager->environment; + r = manager_get_effective_environment(u->manager, &p->environment); + if (r < 0) + return r; + p->confirm_spawn = manager_get_confirm_spawn(u->manager); p->cgroup_supported = u->manager->cgroup_supported; p->prefix = u->manager->prefix; @@ -5037,6 +5042,8 @@ void unit_set_exec_params(Unit *u, ExecParameters *p) { /* Copy paramaters from unit */ p->cgroup_path = u->cgroup_path; SET_FLAG(p->flags, EXEC_CGROUP_DELEGATE, unit_cgroup_delegate(u)); + + return 0; } int unit_fork_helper_process(Unit *u, const char *name, pid_t *ret) { diff --git a/src/core/unit.h b/src/core/unit.h index 1aea658e97..b6ef9d97ee 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -777,7 +777,7 @@ int unit_acquire_invocation_id(Unit *u); bool unit_shall_confirm_spawn(Unit *u); -void unit_set_exec_params(Unit *s, ExecParameters *p); +int unit_set_exec_params(Unit *s, ExecParameters *p); int unit_fork_helper_process(Unit *u, const char *name, pid_t *ret);