From a9353a5c5b512f107955e56a9812724f40b841d3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 9 Nov 2018 18:19:40 +0100 Subject: [PATCH] core: log about /var/run/ prefix used in PIDFile=, patch it to be /run instead In a way this is a follow-up for a2d1fb882c4308bc10362d971f333c5031d60069, but adds a similar warning for PIDFile=. There's a much stronger case for doing this kind of notification in tmpfiles.d (since it helps relating lines to each other for the purpose of merging them). Doing this for PIDFile= is mostly about being systematic and copying tmpfiles.d/ behaviour here. While we are at it, let's also support relative filenames in PIDFile= now, and prefix them with /run, to make them absolute. Fixes: #10657 --- man/systemd.service.xml | 17 ++++---- src/core/dbus-service.c | 36 ++++++++++++++++- src/core/load-fragment-gperf.gperf.m4 | 2 +- src/core/load-fragment.c | 56 +++++++++++++++++++++++++++ src/core/load-fragment.h | 1 + 5 files changed, 101 insertions(+), 11 deletions(-) diff --git a/man/systemd.service.xml b/man/systemd.service.xml index beb4420cf0f..0ce96cf3cb9 100644 --- a/man/systemd.service.xml +++ b/man/systemd.service.xml @@ -272,14 +272,15 @@ PIDFile= - Takes an absolute path referring to the PID file of the service. Usage of this option is - recommended for services where Type= is set to . The service manager - will read the PID of the main process of the service from this file after start-up of the service. The service - manager will not write to the file configured here, although it will remove the file after the service has shut - down if it still exists. The PID file does not need to be owned by a privileged user, but if it is owned by an - unprivileged user additional safety restrictions are enforced: the file may not be a symlink to a file owned by - a different user (neither directly nor indirectly), and the PID file must refer to a process already belonging - to the service. + Takes a path referring to the PID file of the service. Usage of this option is recommended for + services where Type= is set to . The path specified typically points + to a file below /run/. If a relative path is specified it is hence prefixed with + /run/. The service manager will read the PID of the main process of the service from this + file after start-up of the service. The service manager will not write to the file configured here, although it + will remove the file after the service has shut down if it still exists. The PID file does not need to be owned + by a privileged user, but if it is owned by an unprivileged user additional safety restrictions are enforced: + the file may not be a symlink to a file owned by a different user (neither directly nor indirectly), and the + PID file must refer to a process already belonging to the service. diff --git a/src/core/dbus-service.c b/src/core/dbus-service.c index 1b4c98c7d2d..01094a62128 100644 --- a/src/core/dbus-service.c +++ b/src/core/dbus-service.c @@ -312,8 +312,40 @@ static int bus_service_set_transient_property( if (streq(name, "NotifyAccess")) return bus_set_transient_notify_access(u, name, &s->notify_access, message, flags, error); - if (streq(name, "PIDFile")) - return bus_set_transient_path(u, name, &s->pid_file, message, flags, error); + if (streq(name, "PIDFile")) { + _cleanup_free_ char *n = NULL; + const char *v, *e; + + r = sd_bus_message_read(message, "s", &v); + if (r < 0) + return r; + + n = path_make_absolute(v, u->manager->prefix[EXEC_DIRECTORY_RUNTIME]); + if (!n) + return -ENOMEM; + + path_simplify(n, true); + + if (!path_is_normalized(n)) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "PIDFile= path '%s' is not valid", n); + + e = path_startswith(n, "/var/run/"); + if (e) { + char *z; + + z = strjoin("/run/", e); + if (!z) + return log_oom(); + + if (!UNIT_WRITE_FLAGS_NOOP(flags)) + log_unit_notice(u, "Transient unit's PIDFile= property references path below legacy directory /var/run, updating %s → %s; please update client accordingly.", n, z); + + free_and_replace(s->pid_file, z); + } else + free_and_replace(s->pid_file, n); + + return 1; + } if (streq(name, "USBFunctionDescriptors")) return bus_set_transient_path(u, name, &s->usb_function_descriptors, message, flags, error); diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4 index 869ea81a9e5..7ae38538a4f 100644 --- a/src/core/load-fragment-gperf.gperf.m4 +++ b/src/core/load-fragment-gperf.gperf.m4 @@ -288,7 +288,7 @@ Unit.AssertControlGroupController, config_parse_unit_condition_string, CONDI Unit.AssertNull, config_parse_unit_condition_null, 0, offsetof(Unit, asserts) Unit.CollectMode, config_parse_collect_mode, 0, offsetof(Unit, collect_mode) m4_dnl -Service.PIDFile, config_parse_unit_path_printf, 0, offsetof(Service, pid_file) +Service.PIDFile, config_parse_pid_file, 0, offsetof(Service, pid_file) Service.ExecStartPre, config_parse_exec, SERVICE_EXEC_START_PRE, offsetof(Service, exec_command) Service.ExecStart, config_parse_exec, SERVICE_EXEC_START, offsetof(Service, exec_command) Service.ExecStartPost, config_parse_exec, SERVICE_EXEC_START_POST, offsetof(Service, exec_command) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index bfd1d5b15f0..efefdaed996 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -4235,6 +4235,62 @@ int config_parse_emergency_action( return 0; } +int config_parse_pid_file( + const char *unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *lvalue, + int ltype, + const char *rvalue, + void *data, + void *userdata) { + + _cleanup_free_ char *k = NULL, *n = NULL; + Unit *u = userdata; + char **s = data; + const char *e; + int r; + + assert(filename); + assert(lvalue); + assert(rvalue); + assert(u); + + r = unit_full_printf(u, rvalue, &k); + if (r < 0) { + log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers in '%s', ignoring: %m", rvalue); + return 0; + } + + /* If this is a relative path make it absolute by prefixing the /run */ + n = path_make_absolute(k, u->manager->prefix[EXEC_DIRECTORY_RUNTIME]); + if (!n) + return log_oom(); + + /* Check that the result is a sensible path */ + r = path_simplify_and_warn(n, PATH_CHECK_ABSOLUTE, unit, filename, line, lvalue); + if (r < 0) + return r; + + e = path_startswith(n, "/var/run/"); + if (e) { + char *z; + + z = strjoin("/run/", e); + if (!z) + return log_oom(); + + log_syntax(unit, LOG_NOTICE, filename, line, 0, "PIDFile= references path below legacy directory /var/run/, updating %s → %s; please update the unit file accordingly.", n, z); + + free_and_replace(*s, z); + } else + free_and_replace(*s, n); + + return 0; +} + #define FOLLOW_MAX 8 static int open_follow(char **filename, FILE **_f, Set *names, char **_final) { diff --git a/src/core/load-fragment.h b/src/core/load-fragment.h index 4680f3eae38..c9e54ddfb3e 100644 --- a/src/core/load-fragment.h +++ b/src/core/load-fragment.h @@ -104,6 +104,7 @@ CONFIG_PARSER_PROTOTYPE(config_parse_job_timeout_sec); CONFIG_PARSER_PROTOTYPE(config_parse_job_running_timeout_sec); CONFIG_PARSER_PROTOTYPE(config_parse_log_extra_fields); CONFIG_PARSER_PROTOTYPE(config_parse_collect_mode); +CONFIG_PARSER_PROTOTYPE(config_parse_pid_file); /* gperf prototypes */ const struct ConfigPerfItem* load_fragment_gperf_lookup(const char *key, GPERF_LEN_TYPE length);