core: fix owner check of PIDFile=, and update document (#38115)

Closes #38108.

man/systemd.service.xml

@@ -357,15 +357,17 @@
       <varlistentry>
         <term><varname>PIDFile=</varname></term>
 
-        <listitem><para>Takes a path referring to the PID file of the service. Usage of this option is recommended for
-        services where <varname>Type=</varname> is set to <option>forking</option>. The path specified typically points
-        to a file below <filename>/run/</filename>. If a relative path is specified it is hence prefixed with
-        <filename>/run/</filename>. 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.</para>
+        <listitem><para>Takes a path referring to the PID file of the service. Usage of this option is
+        recommended for services where <varname>Type=</varname> is set to <option>forking</option>. The path
+        specified typically points to a file below <filename>/run/</filename>. If a relative path is
+        specified for system service, then it is hence prefixed with <filename>/run/</filename>, and prefixed
+        with <filename>$XDG_RUNTIME_DIR</filename> if specified in a user service. 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.</para>
 
         <para>Note that PID files should be avoided in modern projects. Use <option>Type=notify</option>,
         <option>Type=notify-reload</option> or <option>Type=simple</option> where possible, which does not

meson.build

@@ -484,6 +484,7 @@ possible_link_flags = [
         '-Wl,--fatal-warnings',
         '-Wl,-z,now',
         '-Wl,-z,relro',
+        '-Wl,--gc-sections',
 ]
 
 if get_option('b_sanitize') == 'none'
@@ -503,15 +504,6 @@ possible_cc_flags = [
         '-fvisibility=hidden',
 ]
 
-if get_option('buildtype') != 'debug'
-        possible_cc_flags += [
-                '-ffunction-sections',
-                '-fdata-sections',
-        ]
-
-        possible_link_flags += '-Wl,--gc-sections'
-endif
-
 if get_option('mode') == 'developer'
         possible_cc_flags += '-fno-omit-frame-pointer'
 endif

src/basic/stdio-util.h

@@ -19,7 +19,7 @@ static inline char* snprintf_ok(char *buf, size_t len, const char *format, ...)
 }
 
 #define xsprintf(buf, fmt, ...) \
-        assert_message_se(snprintf_ok(buf, ELEMENTSOF(buf), fmt, ##__VA_ARGS__), "xsprintf: " #buf "[] must be big enough")
+        assert_message_se(snprintf_ok(buf, ELEMENTSOF(buf), fmt, ##__VA_ARGS__), "xsprintf: buffer too small")
 
 #define VA_FORMAT_ADVANCE(format, ap)                                   \
 do {                                                                    \

src/basic/unit-def.c

@@ -361,6 +361,17 @@ static const char* const job_mode_table[_JOB_MODE_MAX] = {
 
 DEFINE_STRING_TABLE_LOOKUP(job_mode, JobMode);
 
+/* This table maps ExecDirectoryType to the setting it is configured with in the unit */
+static const char* const exec_directory_type_table[_EXEC_DIRECTORY_TYPE_MAX] = {
+        [EXEC_DIRECTORY_RUNTIME]       = "RuntimeDirectory",
+        [EXEC_DIRECTORY_STATE]         = "StateDirectory",
+        [EXEC_DIRECTORY_CACHE]         = "CacheDirectory",
+        [EXEC_DIRECTORY_LOGS]          = "LogsDirectory",
+        [EXEC_DIRECTORY_CONFIGURATION] = "ConfigurationDirectory",
+};
+
+DEFINE_STRING_TABLE_LOOKUP(exec_directory_type, ExecDirectoryType);
+
 Glyph unit_active_state_to_glyph(UnitActiveState state) {
         static const Glyph map[_UNIT_ACTIVE_STATE_MAX] = {
                 [UNIT_ACTIVE]       = GLYPH_BLACK_CIRCLE,

src/basic/unit-def.h

@@ -296,6 +296,16 @@ typedef enum JobMode {
         _JOB_MODE_INVALID = -EINVAL,
 } JobMode;
 
+typedef enum ExecDirectoryType {
+        EXEC_DIRECTORY_RUNTIME,
+        EXEC_DIRECTORY_STATE,
+        EXEC_DIRECTORY_CACHE,
+        EXEC_DIRECTORY_LOGS,
+        EXEC_DIRECTORY_CONFIGURATION,
+        _EXEC_DIRECTORY_TYPE_MAX,
+        _EXEC_DIRECTORY_TYPE_INVALID = -EINVAL,
+} ExecDirectoryType;
+
 char* unit_dbus_path_from_name(const char *name);
 int unit_name_from_dbus_path(const char *path, char **name);
 
@@ -361,4 +371,7 @@ NotifyAccess notify_access_from_string(const char *s) _pure_;
 const char* job_mode_to_string(JobMode t) _const_;
 JobMode job_mode_from_string(const char *s) _pure_;
 
+const char* exec_directory_type_to_string(ExecDirectoryType i) _const_;
+ExecDirectoryType exec_directory_type_from_string(const char *s) _pure_;
+
 Glyph unit_active_state_to_glyph(UnitActiveState state);

src/core/execute.h

@@ -7,7 +7,6 @@
 #include "cgroup-util.h"
 #include "core-forward.h"
 #include "cpu-set-util.h"
-#include "exec-directory-util.h"
 #include "exec-util.h"
 #include "list.h"
 #include "log-context.h"

src/core/service.c

@@ -1204,11 +1204,13 @@ static int service_load_pid_file(Service *s, bool may_warn) {
                 if (fstat(fileno(f), &st) < 0)
                         return log_unit_error_errno(UNIT(s), errno, "Failed to fstat() PID file '%s': %m", s->pid_file);
 
-                if (st.st_uid != 0)
+                if (st.st_uid != getuid())
                         return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(EPERM),
-                                                    "New main PID "PID_FMT" from PID file does not belong to service, and PID file is not owned by root. Refusing.", pidref.pid);
+                                                    "New main PID "PID_FMT" from PID file does not belong to service, and PID file is owned by "UID_FMT" (must be owned by "UID_FMT"). Refusing.",
+                                                    pidref.pid, st.st_uid, getuid());
 
-                log_unit_debug(UNIT(s), "New main PID "PID_FMT" does not belong to service, accepting anyway since PID file is owned by root.", pidref.pid);
+                log_unit_debug(UNIT(s), "New main PID "PID_FMT" does not belong to service, accepting anyway since PID file is owned by "UID_FMT".",
+                               pidref.pid, st.st_uid);
         }
 
         if (s->main_pid_known) {

src/shared/condition.h

@@ -58,12 +58,13 @@ typedef enum ConditionResult {
 } ConditionResult;
 
 typedef struct Condition {
+        /* Use bitfields for ConditionType and ConditionResult to keep the whole struct in 32 bytes. */
         ConditionType type:8;
 
-        bool trigger:1;
-        bool negate:1;
+        bool trigger;
+        bool negate;
 
-        ConditionResult result:6;
+        ConditionResult result:8;
 
         char *parameter;
 

src/shared/exec-directory-util.c

@@ -1,15 +0,0 @@
-/* SPDX-License-Identifier: LGPL-2.1-or-later */
-
-#include "exec-directory-util.h"
-#include "string-table.h"
-
-/* This table maps ExecDirectoryType to the setting it is configured with in the unit */
-static const char* const exec_directory_type_table[_EXEC_DIRECTORY_TYPE_MAX] = {
-        [EXEC_DIRECTORY_RUNTIME]       = "RuntimeDirectory",
-        [EXEC_DIRECTORY_STATE]         = "StateDirectory",
-        [EXEC_DIRECTORY_CACHE]         = "CacheDirectory",
-        [EXEC_DIRECTORY_LOGS]          = "LogsDirectory",
-        [EXEC_DIRECTORY_CONFIGURATION] = "ConfigurationDirectory",
-};
-
-DEFINE_STRING_TABLE_LOOKUP(exec_directory_type, ExecDirectoryType);

src/shared/exec-directory-util.h

@@ -1,19 +0,0 @@
-/* SPDX-License-Identifier: LGPL-2.1-or-later */
-#pragma once
-
-#include <errno.h>
-
-#include "macro-fundamental.h"
-
-typedef enum ExecDirectoryType {
-        EXEC_DIRECTORY_RUNTIME,
-        EXEC_DIRECTORY_STATE,
-        EXEC_DIRECTORY_CACHE,
-        EXEC_DIRECTORY_LOGS,
-        EXEC_DIRECTORY_CONFIGURATION,
-        _EXEC_DIRECTORY_TYPE_MAX,
-        _EXEC_DIRECTORY_TYPE_INVALID = -EINVAL,
-} ExecDirectoryType;
-
-const char* exec_directory_type_to_string(ExecDirectoryType i) _const_;
-ExecDirectoryType exec_directory_type_from_string(const char *s) _pure_;

src/shared/meson.build

@@ -69,7 +69,6 @@ shared_sources = files(
         'elf-util.c',
         'enable-mempool.c',
         'ethtool-util.c',
-        'exec-directory-util.c',
         'exec-util.c',
         'exit-status.c',
         'extension-util.c',

src/shared/open-file.c

@@ -76,8 +76,9 @@ int open_file_validate(const OpenFile *of) {
         if (!fdname_is_valid(of->fdname))
                 return -EINVAL;
 
-        if ((FLAGS_SET(of->flags, OPENFILE_READ_ONLY) + FLAGS_SET(of->flags, OPENFILE_APPEND) +
-             FLAGS_SET(of->flags, OPENFILE_TRUNCATE)) > 1)
+        if (FLAGS_SET(of->flags, OPENFILE_READ_ONLY) +
+            FLAGS_SET(of->flags, OPENFILE_APPEND) +
+            FLAGS_SET(of->flags, OPENFILE_TRUNCATE) > 1)
                 return -EINVAL;
 
         if ((of->flags & ~_OPENFILE_MASK_PUBLIC) != 0)

src/systemctl/systemctl-show.c

@@ -15,7 +15,6 @@
 #include "cgroup-show.h"
 #include "cpu-set-util.h"
 #include "errno-util.h"
-#include "exec-directory-util.h"
 #include "exec-util.h"
 #include "exit-status.h"
 #include "extract-word.h"

src/test/test-string-util.c

@@ -9,6 +9,19 @@
 #include "strv.h"
 #include "tests.h"
 
+TEST(xsprintf) {
+        char buf[5];
+
+        xsprintf(buf, "asdf");
+        xsprintf(buf, "%4s", "a");
+        xsprintf(buf, "%-4s", "a");
+        xsprintf(buf, "%04d", 1);
+
+        ASSERT_SIGNAL(xsprintf(buf, "asdfe"), SIGABRT);
+        ASSERT_SIGNAL(xsprintf(buf, "asdfefghdhdhdhdhd"), SIGABRT);
+        ASSERT_SIGNAL(xsprintf(buf, "%5s", "a"), SIGABRT);
+}
+
 TEST(string_erase) {
         char *x;
         x = strdupa_safe("");