From fa07c85956e28db3f6e23c21b65d28d5edb77ba3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 9 Feb 2017 21:01:28 +0100 Subject: dbus: permit seeing process list of units whose unit files are missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, we'd refuse the GetUnitProcesses() bus call if the unit file couldn't be loaded. Which is wrong, as admins should be able to inspect services whose unit files was deleted. Change this logic, so that we permit introspecting the processes of any unit that is loaded, regardless if it has a unit file or not. (Note that we won't load unit files in GetUnitProcess(), but only operate on already loaded ones. That's because only loaded units can have processes — as that's how our GC logic works — and hence loading the unit just for the process tree is pointless, as it would be empty). See: #4995 --- src/core/dbus-manager.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) (limited to 'src/core') diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 0136d38833..b4489ea935 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -848,13 +848,9 @@ static int method_get_unit_processes(sd_bus_message *message, void *userdata, sd if (r < 0) return r; - r = manager_load_unit(m, name, NULL, error, &u); - if (r < 0) - return r; - - r = bus_unit_check_load_state(u, error); - if (r < 0) - return r; + u = manager_get_unit(m, name); + if (!u) + return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_UNIT, "Unit %s not loaded.", name); return bus_unit_method_get_processes(message, u, error); } -- cgit v1.2.3-54-g00ecf From 807fa5d9a01b2bd80ac821d3a165bfef0323c20c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 10 Feb 2017 11:54:18 +0100 Subject: dbus: check selinux privilege before returning process list We protect less interetsing stuff with selinux "status", let's do that here too. --- src/core/dbus-unit.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'src/core') diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 60e889e1ef..f1306a023f 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -1006,6 +1006,10 @@ int bus_unit_method_get_processes(sd_bus_message *message, void *userdata, sd_bu assert(message); + r = mac_selinux_unit_access_check(u, message, "status", error); + if (r < 0) + return r; + pids = set_new(NULL); if (!pids) return -ENOMEM; -- cgit v1.2.3-54-g00ecf From 637d6e5b9c97afdd284f684c319cb4274bb8abcf Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 10 Feb 2017 12:23:22 +0100 Subject: install: when disabling units, do so even if the unit is missing In some cases there might be unit symlinks in .wants/ or .requires/ directories even though the unit is otherwise fully removed. In this case, don't fail removal, but still remove the symlinks. This reworks the symlink marking logic to always add unit files that we are missing to the changes list, but proceed with any symlink removal for them. This way we'll still generate useful hints that a unit is missing if you invoke "systemctl disable idontexist.service", but also still remove any link to it. Fixes: #4995 --- src/core/dbus-manager.c | 154 +++++++++++++++++++++++++++--------------------- src/shared/install.c | 45 +++++++++----- 2 files changed, 118 insertions(+), 81 deletions(-) (limited to 'src/core') diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index b4489ea935..27f948cbdd 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -1912,63 +1912,6 @@ static int send_unit_files_changed(sd_bus *bus, void *userdata) { return sd_bus_send(bus, message, NULL); } -static int reply_unit_file_changes_and_free( - Manager *m, - sd_bus_message *message, - int carries_install_info, - UnitFileChange *changes, - unsigned n_changes) { - - _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; - unsigned i; - int r; - - if (unit_file_changes_have_modification(changes, n_changes)) { - r = bus_foreach_bus(m, NULL, send_unit_files_changed, NULL); - if (r < 0) - log_debug_errno(r, "Failed to send UnitFilesChanged signal: %m"); - } - - r = sd_bus_message_new_method_return(message, &reply); - if (r < 0) - goto fail; - - if (carries_install_info >= 0) { - r = sd_bus_message_append(reply, "b", carries_install_info); - if (r < 0) - goto fail; - } - - r = sd_bus_message_open_container(reply, 'a', "(sss)"); - if (r < 0) - goto fail; - - for (i = 0; i < n_changes; i++) - if (changes[i].type >= 0) { - const char *change = unit_file_change_type_to_string(changes[i].type); - assert(change != NULL); - - r = sd_bus_message_append( - reply, "(sss)", - change, - changes[i].path, - changes[i].source); - if (r < 0) - goto fail; - } - - r = sd_bus_message_close_container(reply); - if (r < 0) - goto fail; - - unit_file_changes_free(changes, n_changes); - return sd_bus_send(NULL, reply, NULL); - -fail: - unit_file_changes_free(changes, n_changes); - return r; -} - /* Create an error reply, using the error information from changes[] * if possible, and fall back to generating an error from error code c. * The error message only describes the first error. @@ -1982,12 +1925,14 @@ static int install_error( unsigned n_changes) { int r; unsigned i; - assert(c < 0); for (i = 0; i < n_changes; i++) + switch(changes[i].type) { + case 0 ... INT_MAX: continue; + case -EEXIST: if (changes[i].source) r = sd_bus_error_setf(error, BUS_ERROR_UNIT_EXISTS, @@ -1998,29 +1943,106 @@ static int install_error( "File %s already exists.", changes[i].path); goto found; + case -ERFKILL: r = sd_bus_error_setf(error, BUS_ERROR_UNIT_MASKED, "Unit file %s is masked.", changes[i].path); goto found; + case -EADDRNOTAVAIL: r = sd_bus_error_setf(error, BUS_ERROR_UNIT_GENERATED, "Unit %s is transient or generated.", changes[i].path); goto found; + case -ELOOP: r = sd_bus_error_setf(error, BUS_ERROR_UNIT_LINKED, "Refusing to operate on linked unit file %s", changes[i].path); goto found; + + case -ENOENT: + r = sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_UNIT, "Unit file %s does not exist.", changes[i].path); + goto found; + default: r = sd_bus_error_set_errnof(error, changes[i].type, "File %s: %m", changes[i].path); goto found; } - r = c; + r = c < 0 ? c : -EINVAL; + found: unit_file_changes_free(changes, n_changes); return r; } +static int reply_unit_file_changes_and_free( + Manager *m, + sd_bus_message *message, + int carries_install_info, + UnitFileChange *changes, + unsigned n_changes, + sd_bus_error *error) { + + _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; + bool bad = false, good = false; + unsigned i; + int r; + + if (unit_file_changes_have_modification(changes, n_changes)) { + r = bus_foreach_bus(m, NULL, send_unit_files_changed, NULL); + if (r < 0) + log_debug_errno(r, "Failed to send UnitFilesChanged signal: %m"); + } + + r = sd_bus_message_new_method_return(message, &reply); + if (r < 0) + goto fail; + + if (carries_install_info >= 0) { + r = sd_bus_message_append(reply, "b", carries_install_info); + if (r < 0) + goto fail; + } + + r = sd_bus_message_open_container(reply, 'a', "(sss)"); + if (r < 0) + goto fail; + + for (i = 0; i < n_changes; i++) { + + if (changes[i].type < 0) { + bad = true; + continue; + } + + r = sd_bus_message_append( + reply, "(sss)", + unit_file_change_type_to_string(changes[i].type), + changes[i].path, + changes[i].source); + if (r < 0) + goto fail; + + good = true; + } + + /* If there was a failed change, and no successful change, then return the first failure as proper method call + * error. */ + if (bad && !good) + return install_error(error, 0, changes, n_changes); + + r = sd_bus_message_close_container(reply); + if (r < 0) + goto fail; + + unit_file_changes_free(changes, n_changes); + return sd_bus_send(NULL, reply, NULL); + +fail: + unit_file_changes_free(changes, n_changes); + return r; +} + static int method_enable_unit_files_generic( sd_bus_message *message, Manager *m, @@ -2057,7 +2079,7 @@ static int method_enable_unit_files_generic( if (r < 0) return install_error(error, r, changes, n_changes); - return reply_unit_file_changes_and_free(m, message, carries_install_info ? r : -1, changes, n_changes); + return reply_unit_file_changes_and_free(m, message, carries_install_info ? r : -1, changes, n_changes, error); } static int method_enable_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) { @@ -2126,7 +2148,7 @@ static int method_preset_unit_files_with_mode(sd_bus_message *message, void *use if (r < 0) return install_error(error, r, changes, n_changes); - return reply_unit_file_changes_and_free(m, message, r, changes, n_changes); + return reply_unit_file_changes_and_free(m, message, r, changes, n_changes, error); } static int method_disable_unit_files_generic( @@ -2161,7 +2183,7 @@ static int method_disable_unit_files_generic( if (r < 0) return install_error(error, r, changes, n_changes); - return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes); + return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes, error); } static int method_disable_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) { @@ -2196,7 +2218,7 @@ static int method_revert_unit_files(sd_bus_message *message, void *userdata, sd_ if (r < 0) return install_error(error, r, changes, n_changes); - return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes); + return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes, error); } static int method_set_default_target(sd_bus_message *message, void *userdata, sd_bus_error *error) { @@ -2227,7 +2249,7 @@ static int method_set_default_target(sd_bus_message *message, void *userdata, sd if (r < 0) return install_error(error, r, changes, n_changes); - return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes); + return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes, error); } static int method_preset_all_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) { @@ -2270,7 +2292,7 @@ static int method_preset_all_unit_files(sd_bus_message *message, void *userdata, if (r < 0) return install_error(error, r, changes, n_changes); - return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes); + return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes, error); } static int method_add_dependency_unit_files(sd_bus_message *message, void *userdata, sd_bus_error *error) { @@ -2310,7 +2332,7 @@ static int method_add_dependency_unit_files(sd_bus_message *message, void *userd if (r < 0) return install_error(error, r, changes, n_changes); - return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes); + return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes, error); } static int method_get_unit_file_links(sd_bus_message *message, void *userdata, sd_bus_error *error) { diff --git a/src/shared/install.c b/src/shared/install.c index f25ed685f6..c027f8ecd3 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -389,6 +389,12 @@ void unit_file_dump_changes(int r, const char *verb, const UnitFileChange *chang verb, changes[i].path); logged = true; break; + + case -ENOENT: + log_error_errno(changes[i].type, "Failed to %s unit, unit %s does not exist.", verb, changes[i].path); + logged = true; + break; + default: assert(changes[i].type < 0); log_error_errno(changes[i].type, "Failed to %s unit, file %s: %m.", @@ -1807,7 +1813,9 @@ static int install_context_mark_for_removal( InstallContext *c, const LookupPaths *paths, Set **remove_symlinks_to, - const char *config_path) { + const char *config_path, + UnitFileChange **changes, + unsigned *n_changes) { UnitFileInstallInfo *i; int r; @@ -1833,19 +1841,26 @@ static int install_context_mark_for_removal( r = install_info_traverse(scope, c, paths, i, SEARCH_LOAD|SEARCH_FOLLOW_CONFIG_SYMLINKS, NULL); if (r == -ENOLINK) { - log_debug_errno(r, "Name %s leads to a dangling symlink, ignoring.", i->name); - continue; - } else if (r == -ENOENT && i->auxiliary) { - /* some unit specified in Also= or similar is missing */ - log_debug_errno(r, "Auxiliary unit %s not found, ignoring.", i->name); - continue; - } else if (r < 0) - return log_debug_errno(r, "Failed to find unit %s: %m", i->name); + log_debug_errno(r, "Name %s leads to a dangling symlink, removing name.", i->name); + unit_file_changes_add(changes, n_changes, UNIT_FILE_IS_DANGLING, i->path ?: i->name, NULL); + } else if (r == -ENOENT) { + + if (i->auxiliary) /* some unit specified in Also= or similar is missing */ + log_debug_errno(r, "Auxiliary unit of %s not found, removing name.", i->name); + else { + log_debug_errno(r, "Unit %s not found, removing name.", i->name); + unit_file_changes_add(changes, n_changes, r, i->path ?: i->name, NULL); + } - if (i->type != UNIT_FILE_TYPE_REGULAR) { - log_debug("Unit %s has type %s, ignoring.", - i->name, - unit_file_type_to_string(i->type) ?: "invalid"); + } else if (r < 0) { + log_debug_errno(r, "Failed to find unit %s, removing name: %m", i->name); + unit_file_changes_add(changes, n_changes, r, i->path ?: i->name, NULL); + } else if (i->type == UNIT_FILE_TYPE_MASKED) { + log_debug("Unit file %s is masked, ignoring.", i->name); + unit_file_changes_add(changes, n_changes, UNIT_FILE_IS_MASKED, i->path ?: i->name, NULL); + continue; + } else if (i->type != UNIT_FILE_TYPE_REGULAR) { + log_debug("Unit %s has type %s, ignoring.", i->name, unit_file_type_to_string(i->type) ?: "invalid"); continue; } @@ -2401,7 +2416,7 @@ int unit_file_disable( return r; } - r = install_context_mark_for_removal(scope, &c, &paths, &remove_symlinks_to, config_path); + r = install_context_mark_for_removal(scope, &c, &paths, &remove_symlinks_to, config_path, changes, n_changes); if (r < 0) return r; @@ -2790,7 +2805,7 @@ static int execute_preset( if (mode != UNIT_FILE_PRESET_ENABLE_ONLY) { _cleanup_set_free_free_ Set *remove_symlinks_to = NULL; - r = install_context_mark_for_removal(scope, minus, paths, &remove_symlinks_to, config_path); + r = install_context_mark_for_removal(scope, minus, paths, &remove_symlinks_to, config_path, changes, n_changes); if (r < 0) return r; -- cgit v1.2.3-54-g00ecf