diff --git a/NEWS b/NEWS index 3e474dd3c45..e57e756c6ff 100644 --- a/NEWS +++ b/NEWS @@ -11,6 +11,10 @@ CHANGES WITH 263 in spe: Feature Removals and Incompatible Changes: + * Transient *Directory= drop-ins now escape colons and quotes in + paths set through either D-Bus signature, preserving the paths + when the unit is reloaded. + * systemd-networkd and networkctl have been merged into a single multi-call binary, with the former becoming a symlink to the latter. This should generally be invisible to users, but some packaging and diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index 16467218452..d7163ac758b 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -3718,16 +3718,27 @@ int bus_exec_context_set_transient_property( exec_directory_done(d); unit_write_settingf(u, flags, name, "%s=", name); } else { + _cleanup_strv_free_ char **escaped = NULL; _cleanup_free_ char *joined = NULL; STRV_FOREACH(source, l) { r = exec_directory_add(d, *source, /* symlink= */ NULL, /* flags= */ 0); if (r < 0) return log_oom(); + + /* unit_concat_strv() quotes each path, so only tuple separators and + * double quotes need extra escaping before parsing it again. */ + _cleanup_free_ char *source_escaped = xescape(*source, ":\""); + if (!source_escaped) + return log_oom(); + + r = strv_consume(&escaped, TAKE_PTR(source_escaped)); + if (r < 0) + return log_oom(); } exec_directory_sort(d); - joined = unit_concat_strv(l, UNIT_ESCAPE_SPECIFIERS); + joined = unit_concat_strv(escaped, UNIT_ESCAPE_SPECIFIERS); if (!joined) return -ENOMEM; @@ -4183,7 +4194,10 @@ int bus_exec_context_set_transient_property( return 1; - } else if (STR_IN_SET(name, "StateDirectorySymlink", "RuntimeDirectorySymlink", "CacheDirectorySymlink", "LogsDirectorySymlink")) { + } else if (STR_IN_SET(name, + "StateDirectorySymlink", "RuntimeDirectorySymlink", "CacheDirectorySymlink", + "LogsDirectorySymlink", "ConfigurationDirectorySymlink")) { + const char *const escape_chars = ":\"' "; char *source, *destination; ExecDirectory *directory; uint64_t symlink_flags; @@ -4208,6 +4222,12 @@ int bus_exec_context_set_transient_property( if (isempty(destination)) destination = NULL; else { + /* Symlinks are not supported for ConfigurationDirectory=, matching what the + * unit file parser accepts. The flags field is still useful there, though. */ + if (i == EXEC_DIRECTORY_CONFIGURATION) + return sd_bus_error_setf(reterr_error, SD_BUS_ERROR_INVALID_ARGS, + "Symlink destination is not supported for %s=.", + exec_directory_type_to_string(i)); if (!path_is_valid(destination)) return sd_bus_error_setf(reterr_error, SD_BUS_ERROR_INVALID_ARGS, "Destination path %s is not valid.", destination); if (path_is_absolute(destination)) @@ -4223,24 +4243,29 @@ int bus_exec_context_set_transient_property( if (r < 0) return r; - /* Need to store them in the unit with the escapes, so that they can be parsed again */ - source_escaped = xescape(source, ":"); + /* Escape tuple separators, quotes and whitespace so the unit file parser + * preserves each path when it splits the assignment into words and fields. */ + source_escaped = xescape(source, escape_chars); if (!source_escaped) return -ENOMEM; if (destination) { - destination_escaped = xescape(destination, ":"); + destination_escaped = xescape(destination, escape_chars); if (!destination_escaped) return -ENOMEM; } + const char *flags_string = exec_directory_flags_to_string(symlink_flags); + assert(flags_string); /* Validated before applying the property. */ + unit_write_settingf( u, flags|UNIT_ESCAPE_SPECIFIERS, exec_directory_type_to_string(i), - "%s=%s%s%s%s", + "%s=%s%s%s%s%s", exec_directory_type_to_string(i), source_escaped, - destination_escaped || FLAGS_SET(symlink_flags, EXEC_DIRECTORY_READ_ONLY) ? ":" : "", - destination_escaped, - FLAGS_SET(symlink_flags, EXEC_DIRECTORY_READ_ONLY) ? ":ro" : ""); + destination || !isempty(flags_string) ? ":" : "", + strempty(destination_escaped), + isempty(flags_string) ? "" : ":", + flags_string); } } if (r < 0) diff --git a/src/test/test-exec-directory.c b/src/test/test-exec-directory.c index e1712ad9006..c09551d262d 100644 --- a/src/test/test-exec-directory.c +++ b/src/test/test-exec-directory.c @@ -1,14 +1,29 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include "sd-bus.h" + +#include "alloc-util.h" +#include "bus-internal.h" #include "cgroup.h" +#include "dbus-execute.h" #include "dynamic-user.h" #include "execute-serialize.h" #include "execute.h" #include "fd-util.h" #include "fdset.h" #include "fileio.h" +#include "load-fragment.h" +#include "manager.h" +#include "rm-rf.h" +#include "service.h" +#include "string-util.h" #include "strv.h" #include "tests.h" +#include "unit.h" + +static char *runtime_dir = NULL; + +STATIC_DESTRUCTOR_REGISTER(runtime_dir, rm_rf_physical_and_freep); static const char* const paths[] = { "plain", "with space", "with'quote", "with\"quote", "with:colon", @@ -31,6 +46,106 @@ static void assert_directory( } } +static void test_transient_directory_one( + sd_bus *bus, + Manager *manager, + ExecDirectoryType type, + const char *source, + const char *destination, + bool tuple, + uint64_t flags, + int expected) { + + _cleanup_(sd_bus_message_unrefp) sd_bus_message *message = NULL; + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + _cleanup_(exec_context_done) ExecContext context = {}; + _cleanup_(exec_directory_done) ExecDirectory parsed = {}; + _cleanup_free_ char *text = NULL; + _cleanup_(unit_freep) Unit *unit = NULL; + size_t size = 0; + FILE *f; + + ASSERT_OK(unit_new_for_name(manager, sizeof(Service), "test-exec-directory.service", &unit)); + unit->last_section_private = 1; + ASSERT_NOT_NULL(f = open_memstream_unlocked(&text, &size)); + unit->transient_file = f; + + ASSERT_OK(sd_bus_message_new(bus, &message, SD_BUS_MESSAGE_METHOD_CALL)); + if (tuple) + ASSERT_OK(sd_bus_message_append(message, "a(sst)", 1, source, strempty(destination), flags)); + else + ASSERT_OK(sd_bus_message_append(message, "as", 1, source)); + ASSERT_OK(sd_bus_message_seal(message, 1, 0)); + ASSERT_OK(sd_bus_message_rewind(message, true)); + + /* Validate first without applying anything, matching the manager's two-pass handling. */ + ASSERT_EQ(bus_exec_context_set_transient_property( + unit, &context, + tuple ? exec_directory_type_symlink_to_string(type) : exec_directory_type_to_string(type), + message, 0, &error), expected); + ASSERT_EQ(context.directories[type].n_items, 0U); + sd_bus_error_free(&error); + ASSERT_OK(sd_bus_message_rewind(message, true)); + + ASSERT_EQ(bus_exec_context_set_transient_property( + unit, &context, + tuple ? exec_directory_type_symlink_to_string(type) : exec_directory_type_to_string(type), + message, UNIT_RUNTIME, &error), expected); + if (expected < 0) { + ASSERT_TRUE(sd_bus_error_has_name(&error, SD_BUS_ERROR_INVALID_ARGS)); + ASSERT_EQ(context.directories[type].n_items, 0U); + return; + } + + ASSERT_OK_ERRNO(fflush(f)); + assert_directory(&context.directories[type], source, destination, flags); + char *value = ASSERT_NOT_NULL(strchr(text, '=')) + 1; + delete_trailing_chars(value, "\n"); + ASSERT_OK(config_parse_exec_directories( + "test.service", "test.conf", 1, "Service", 1, + exec_directory_type_to_string(type), 0, value, &parsed, unit)); + assert_directory(&parsed, source, destination, flags); +} + +TEST(transient_directory_roundtrip) { + _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(manager_freep) Manager *manager = NULL; + int r; + + ASSERT_OK(sd_bus_new(&bus)); + bus->state = BUS_RUNNING; /* Only construct messages locally; no bus connection is needed. */ + + r = manager_new(RUNTIME_SCOPE_USER, MANAGER_TEST_RUN_MINIMAL, &manager); + if (manager_errno_skip_test(r)) { + log_notice_errno(r, "Skipping test: manager_new: %m"); + return; + } + ASSERT_OK(r); + ASSERT_OK(manager_startup(manager, NULL, NULL, NULL, NULL)); + + for (ExecDirectoryType type = 0; type < _EXEC_DIRECTORY_TYPE_MAX; type++) { + FOREACH_ELEMENT(path, paths) { + test_transient_directory_one(bus, manager, type, *path, + /* destination= */ NULL, /* tuple= */ false, + /* flags= */ 0, 1); + FOREACH_ELEMENT(flags, ((const uint64_t[]) { + 0, EXEC_DIRECTORY_READ_ONLY, EXEC_DIRECTORY_ONLY_CREATE, + UINT64_C(1) << 32, (UINT64_C(1) << 32) | EXEC_DIRECTORY_READ_ONLY, + UINT64_C(1) << 63, UINT64_MAX })) { + bool valid_flags = (*flags & ~((uint64_t) _EXEC_DIRECTORY_FLAGS_PUBLIC)) == 0; + + test_transient_directory_one(bus, manager, type, *path, + /* destination= */ NULL, /* tuple= */ true, + *flags, valid_flags ? 1 : -EINVAL); + test_transient_directory_one(bus, manager, type, "source", *path, + /* tuple= */ true, *flags, + valid_flags && type != EXEC_DIRECTORY_CONFIGURATION ? 1 : -EINVAL); + } + } + + } +} + TEST(executor_directory_roundtrip) { FOREACH_ELEMENT(path, paths) { _cleanup_(exec_context_done) ExecContext context = {}, parsed = {}; @@ -75,4 +190,9 @@ TEST(executor_directory_roundtrip) { } } -DEFINE_TEST_MAIN(LOG_DEBUG); +static int intro(void) { + ASSERT_NOT_NULL(runtime_dir = setup_fake_runtime_dir()); + return EXIT_SUCCESS; +} + +DEFINE_TEST_MAIN_WITH_INTRO(LOG_DEBUG, intro);