From 9b076e86c036b66536ad17ad1d7e55fb41728d49 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 9 Apr 2024 21:38:16 +0800 Subject: [PATCH 1/5] core/exec-invoke: modernize connect_unix_harder --- src/core/exec-invoke.c | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index ee2c1247a9..c313cef8b9 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -3699,11 +3699,12 @@ static int add_shifted_fd(int *fds, size_t fds_size, size_t *n_fds, int *fd) { } static int connect_unix_harder(const ExecContext *c, const ExecParameters *p, const OpenFile *of, int ofd) { + static const int socket_types[] = { SOCK_DGRAM, SOCK_STREAM, SOCK_SEQPACKET }; + union sockaddr_union addr = { .un.sun_family = AF_UNIX, }; socklen_t sa_len; - static const int socket_types[] = { SOCK_DGRAM, SOCK_STREAM, SOCK_SEQPACKET }; int r; assert(c); @@ -3713,37 +3714,29 @@ static int connect_unix_harder(const ExecContext *c, const ExecParameters *p, co r = sockaddr_un_set_path(&addr.un, FORMAT_PROC_FD_PATH(ofd)); if (r < 0) - return log_exec_error_errno(c, p, r, "Failed to set sockaddr for %s: %m", of->path); - + return log_exec_error_errno(c, p, r, "Failed to set sockaddr for '%s': %m", of->path); sa_len = r; - for (size_t i = 0; i < ELEMENTSOF(socket_types); i++) { + FOREACH_ARRAY(i, socket_types, ELEMENTSOF(socket_types)) { _cleanup_close_ int fd = -EBADF; - fd = socket(AF_UNIX, socket_types[i] | SOCK_CLOEXEC, 0); + fd = socket(AF_UNIX, *i|SOCK_CLOEXEC, 0); if (fd < 0) - return log_exec_error_errno(c, - p, - errno, - "Failed to create socket for %s: %m", + return log_exec_error_errno(c, p, + errno, "Failed to create socket for '%s': %m", of->path); r = RET_NERRNO(connect(fd, &addr.sa, sa_len)); - if (r == -EPROTOTYPE) - continue; - if (r < 0) - return log_exec_error_errno(c, - p, - r, - "Failed to connect socket for %s: %m", + if (r >= 0) + return TAKE_FD(fd); + if (r != -EPROTOTYPE) + return log_exec_error_errno(c, p, + r, "Failed to connect to socket for '%s': %m", of->path); - - return TAKE_FD(fd); } - return log_exec_error_errno(c, - p, - SYNTHETIC_ERRNO(EPROTOTYPE), "Failed to connect socket for \"%s\".", + return log_exec_error_errno(c, p, + SYNTHETIC_ERRNO(EPROTOTYPE), "No suitable socket type to connect to socket '%s'.", of->path); } From c642f43131e77210ac14ec1c2e947be6e4b1dd40 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 9 Apr 2024 21:48:01 +0800 Subject: [PATCH 2/5] core/exec-invoke: modernize get_open_file_fd a bit --- src/core/exec-invoke.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index c313cef8b9..f1e1935e40 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -3741,8 +3741,8 @@ static int connect_unix_harder(const ExecContext *c, const ExecParameters *p, co } static int get_open_file_fd(const ExecContext *c, const ExecParameters *p, const OpenFile *of) { - struct stat st; _cleanup_close_ int fd = -EBADF, ofd = -EBADF; + struct stat st; assert(c); assert(p); @@ -3750,10 +3750,10 @@ static int get_open_file_fd(const ExecContext *c, const ExecParameters *p, const ofd = open(of->path, O_PATH | O_CLOEXEC); if (ofd < 0) - return log_exec_error_errno(c, p, errno, "Could not open \"%s\": %m", of->path); + return log_exec_error_errno(c, p, errno, "Failed to open '%s' as O_PATH: %m", of->path); if (fstat(ofd, &st) < 0) - return log_exec_error_errno(c, p, errno, "Failed to stat %s: %m", of->path); + return log_exec_error_errno(c, p, errno, "Failed to stat '%s': %m", of->path); if (S_ISSOCK(st.st_mode)) { fd = connect_unix_harder(c, p, of, ofd); @@ -3761,10 +3761,11 @@ static int get_open_file_fd(const ExecContext *c, const ExecParameters *p, const return fd; if (FLAGS_SET(of->flags, OPENFILE_READ_ONLY) && shutdown(fd, SHUT_WR) < 0) - return log_exec_error_errno(c, p, errno, "Failed to shutdown send for socket %s: %m", + return log_exec_error_errno(c, p, + errno, "Failed to shutdown send for socket '%s': %m", of->path); - log_exec_debug(c, p, "socket %s opened (fd=%d)", of->path, fd); + log_exec_debug(c, p, "Opened socket '%s' as fd %d.", of->path, fd); } else { int flags = FLAGS_SET(of->flags, OPENFILE_READ_ONLY) ? O_RDONLY : O_RDWR; if (FLAGS_SET(of->flags, OPENFILE_APPEND)) @@ -3774,9 +3775,9 @@ static int get_open_file_fd(const ExecContext *c, const ExecParameters *p, const fd = fd_reopen(ofd, flags | O_CLOEXEC); if (fd < 0) - return log_exec_error_errno(c, p, fd, "Failed to open file %s: %m", of->path); + return log_exec_error_errno(c, p, fd, "Failed to reopen file '%s': %m", of->path); - log_exec_debug(c, p, "file %s opened (fd=%d)", of->path, fd); + log_exec_debug(c, p, "Opened file '%s' as fd %d.", of->path, fd); } return TAKE_FD(fd); @@ -3795,7 +3796,9 @@ static int collect_open_file_fds(const ExecContext *c, ExecParameters *p, size_t fd = get_open_file_fd(c, p, of); if (fd < 0) { if (FLAGS_SET(of->flags, OPENFILE_GRACEFUL)) { - log_exec_debug_errno(c, p, fd, "Failed to get OpenFile= file descriptor for %s, ignoring: %m", of->path); + log_exec_warning_errno(c, p, fd, + "Failed to get OpenFile= file descriptor for '%s', ignoring: %m", + of->path); continue; } @@ -3809,9 +3812,7 @@ static int collect_open_file_fds(const ExecContext *c, ExecParameters *p, size_t if (r < 0) return r; - p->fds[*n_fds] = TAKE_FD(fd); - - (*n_fds)++; + p->fds[(*n_fds)++] = TAKE_FD(fd); } return 0; From 4eb1feb110c008e9b08fa7e3d235ccef89643e75 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 10 Apr 2024 19:18:19 +0800 Subject: [PATCH 3/5] shared/open-file: make open_file_free_many static inline --- src/shared/open-file.c | 10 ++-------- src/shared/open-file.h | 10 ++++++---- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/shared/open-file.c b/src/shared/open-file.c index beebb74cff..09062d8e6e 100644 --- a/src/shared/open-file.c +++ b/src/shared/open-file.c @@ -1,4 +1,3 @@ - /* SPDX-License-Identifier: LGPL-2.1-or-later */ #include @@ -122,21 +121,16 @@ int open_file_to_string(const OpenFile *of, char **ret) { return 0; } -OpenFile *open_file_free(OpenFile *of) { +OpenFile* open_file_free(OpenFile *of) { if (!of) return NULL; free(of->path); free(of->fdname); + return mfree(of); } -void open_file_free_many(OpenFile **head) { - assert(head); - - LIST_CLEAR(open_files, *head, open_file_free); -} - static const char * const open_file_flags_table[_OPENFILE_MAX] = { [OPENFILE_READ_ONLY] = "read-only", [OPENFILE_APPEND] = "append", diff --git a/src/shared/open-file.h b/src/shared/open-file.h index bb63ec8f9c..4999c96ae7 100644 --- a/src/shared/open-file.h +++ b/src/shared/open-file.h @@ -1,8 +1,8 @@ - /* SPDX-License-Identifier: LGPL-2.1-or-later */ #pragma once #include "list.h" +#include "macro.h" typedef enum OpenFileFlag { OPENFILE_READ_ONLY = 1 << 0, @@ -27,10 +27,12 @@ int open_file_validate(const OpenFile *of); int open_file_to_string(const OpenFile *of, char **ret); -OpenFile *open_file_free(OpenFile *of); +OpenFile* open_file_free(OpenFile *of); DEFINE_TRIVIAL_CLEANUP_FUNC(OpenFile*, open_file_free); -void open_file_free_many(OpenFile **head); +static inline void open_file_free_many(OpenFile **head) { + LIST_CLEAR(open_files, *ASSERT_PTR(head), open_file_free); +} -const char *open_file_flags_to_string(OpenFileFlag t) _const_; +const char* open_file_flags_to_string(OpenFileFlag t) _const_; OpenFileFlag open_file_flags_from_string(const char *t) _pure_; From c1f9509f79b1edf53975ebfa2052a23b23c42a49 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 9 Apr 2024 21:54:30 +0800 Subject: [PATCH 4/5] shared/open-file: use xescape to escape ':' Fixes #32179 --- src/shared/open-file.c | 2 +- src/test/test-open-file.c | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/shared/open-file.c b/src/shared/open-file.c index 09062d8e6e..a9c2a11032 100644 --- a/src/shared/open-file.c +++ b/src/shared/open-file.c @@ -95,7 +95,7 @@ int open_file_to_string(const OpenFile *of, char **ret) { assert(of); assert(ret); - s = shell_escape(of->path, ":"); + s = xescape(of->path, ":"); if (!s) return -ENOMEM; diff --git a/src/test/test-open-file.c b/src/test/test-open-file.c index 1b938ec5f7..4c3ba57bc8 100644 --- a/src/test/test-open-file.c +++ b/src/test/test-open-file.c @@ -172,14 +172,12 @@ TEST(open_file_to_string) { assert_se(streq(s, "/proc/1/ns/mnt::read-only")); s = mfree(s); - assert_se(free_and_strdup(&of->path, "/path:with:colon")); - assert_se(free_and_strdup(&of->fdname, "path:with:colon")); + ASSERT_OK(free_and_strdup(&of->path, "/path:with:colon")); + ASSERT_OK(free_and_strdup(&of->fdname, "path:with:colon")); of->flags = 0; - r = open_file_to_string(of, &s); - - assert_se(r >= 0); - assert_se(streq(s, "/path\\:with\\:colon")); + ASSERT_OK(open_file_to_string(of, &s)); + ASSERT_STREQ(s, "/path\\x3awith\\x3acolon"); } DEFINE_TEST_MAIN(LOG_INFO); From 48c21790baffce5bbc67168b84fbab4140300a80 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 10 Apr 2024 19:04:50 +0800 Subject: [PATCH 5/5] test: merge TEST-77-OPENFILE into TEST-23-UNIT-FILE And add coverage for #32179 --- test/TEST-77-OPENFILE/Makefile | 1 - test/TEST-77-OPENFILE/test.sh | 16 ------ .../testsuite-23-openfile-server.socket} | 2 +- .../testsuite-23-openfile-server@.service} | 2 +- test/units/testsuite-23-openfile-child.sh | 15 +++++ test/units/testsuite-23.openfile.sh | 55 +++++++++++++++++++ test/units/testsuite-77-client.sh | 14 ----- test/units/testsuite-77-run.sh | 14 ----- test/units/testsuite-77.service | 10 ---- test/units/testsuite-77.sh | 38 ------------- 10 files changed, 72 insertions(+), 95 deletions(-) delete mode 120000 test/TEST-77-OPENFILE/Makefile delete mode 100755 test/TEST-77-OPENFILE/test.sh rename test/{units/testsuite-77-server.socket => testsuite-23.units/testsuite-23-openfile-server.socket} (56%) rename test/{units/testsuite-77-server@.service => testsuite-23.units/testsuite-23-openfile-server@.service} (66%) create mode 100755 test/units/testsuite-23-openfile-child.sh create mode 100755 test/units/testsuite-23.openfile.sh delete mode 100755 test/units/testsuite-77-client.sh delete mode 100755 test/units/testsuite-77-run.sh delete mode 100644 test/units/testsuite-77.service delete mode 100755 test/units/testsuite-77.sh diff --git a/test/TEST-77-OPENFILE/Makefile b/test/TEST-77-OPENFILE/Makefile deleted file mode 120000 index e9f93b1104..0000000000 --- a/test/TEST-77-OPENFILE/Makefile +++ /dev/null @@ -1 +0,0 @@ -../TEST-01-BASIC/Makefile \ No newline at end of file diff --git a/test/TEST-77-OPENFILE/test.sh b/test/TEST-77-OPENFILE/test.sh deleted file mode 100755 index 92afa4c716..0000000000 --- a/test/TEST-77-OPENFILE/test.sh +++ /dev/null @@ -1,16 +0,0 @@ -#!/usr/bin/env bash -# SPDX-License-Identifier: LGPL-2.1-or-later -set -e - -TEST_DESCRIPTION="Openfile tests" - -# shellcheck source=test/test-functions -. "${TEST_BASE_DIR:?}/test-functions" - -test_append_files() { - local workspace="${1:?}" - echo "Open" >"$workspace/test-77-open.dat" - echo "File" >"$workspace/test-77-file.dat" -} - -do_test "$@" diff --git a/test/units/testsuite-77-server.socket b/test/testsuite-23.units/testsuite-23-openfile-server.socket similarity index 56% rename from test/units/testsuite-77-server.socket rename to test/testsuite-23.units/testsuite-23-openfile-server.socket index 4305077501..c8f759584a 100644 --- a/test/units/testsuite-77-server.socket +++ b/test/testsuite-23.units/testsuite-23-openfile-server.socket @@ -1,5 +1,5 @@ [Unit] -Description=TEST-77-OPENFILE server socket +Description=OpenFile= test server socket [Socket] ListenStream=/tmp/test.sock diff --git a/test/units/testsuite-77-server@.service b/test/testsuite-23.units/testsuite-23-openfile-server@.service similarity index 66% rename from test/units/testsuite-77-server@.service rename to test/testsuite-23.units/testsuite-23-openfile-server@.service index 8e99ac8334..a57c804c43 100644 --- a/test/units/testsuite-77-server@.service +++ b/test/testsuite-23.units/testsuite-23-openfile-server@.service @@ -1,5 +1,5 @@ [Unit] -Description=TEST-77-OPENFILE server +Description=OpenFile= test server service [Service] ExecStart=echo "Socket" diff --git a/test/units/testsuite-23-openfile-child.sh b/test/units/testsuite-23-openfile-child.sh new file mode 100755 index 0000000000..4828b9dc10 --- /dev/null +++ b/test/units/testsuite-23-openfile-child.sh @@ -0,0 +1,15 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +set -eux +set -o pipefail + +# shellcheck source=test/units/util.sh +. "$(dirname "$0")"/util.sh + +assert_eq "$LISTEN_FDS" "$1" +assert_eq "$LISTEN_FDNAMES" "$2" + +for ((i = 3; i < 3 + LISTEN_FDS; i++)); do + read -r -u "$i" text + assert_eq "$text" "${!i}" # Dereference $i to get i'th arg +done diff --git a/test/units/testsuite-23.openfile.sh b/test/units/testsuite-23.openfile.sh new file mode 100755 index 0000000000..61081617a1 --- /dev/null +++ b/test/units/testsuite-23.openfile.sh @@ -0,0 +1,55 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +set -eux +set -o pipefail + +# shellcheck source=test/units/util.sh +. "$(dirname "$0")"/util.sh + +at_exit() { + set +e + + rm -rf /tmp/test-open-file/ +} + +trap at_exit EXIT + +systemctl log-level debug + +# Existing files + +mkdir /tmp/test-open-file +echo "Open" >'/tmp/test-open-file/open.txt' +echo "File" >'/tmp/test-open-file/file:colon.txt' + +systemd-run -p DynamicUser=yes -p EnvironmentFile=-/usr/lib/systemd/systemd-asan-env \ + -p OpenFile='/tmp/test-open-file/open.txt::read-only' \ + -p OpenFile='/tmp/test-open-file/file\x3Acolon.txt:colon' \ + -p RemainAfterExit=yes \ + --unit=test-23-openfile-existing.service \ + --service-type=oneshot \ + /usr/lib/systemd/tests/testdata/units/testsuite-23-openfile-child.sh 2 "open.txt:colon" "Open" "File" + +cmp <(systemctl show -p OpenFile test-23-openfile-existing.service) </test-77-new-file.dat -systemd-run --wait -p OpenFile=/test-77-new-file.dat:new-file:read-only "$(dirname "$0")"/testsuite-77-run.sh - -assert_rc 202 systemd-run --wait -p OpenFile=/test-77-new-file.dat:new-file:read-only -p OpenFile=/test-77-mssing-file.dat:missing-file:read-only "$(dirname "$0")"/testsuite-77-run.sh - -assert_rc 0 systemd-run --wait -p OpenFile=/test-77-new-file.dat:new-file:read-only -p OpenFile=/test-77-mssing-file.dat:missing-file:read-only,graceful "$(dirname "$0")"/testsuite-77-run.sh - -# End -touch /testok