From e8a6625422db9d5598b6d640a9f4eec68921ce3d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 14 Dec 2022 14:31:09 +0900 Subject: [PATCH 1/4] sd-id128: make sd_id128_get_machine() or friends return -EUCLEAN when an ID is in an invalid format EINVAL suggests that the caller passes an invalid argument. EIO is for "input/output error", i.e. the error you'd get if the disk or file system is borked, and this error code could be returned by the underlying read/write functions. Let's make the functions return an unambiguous error code. --- man/sd_id128_get_machine.xml | 2 +- src/libsystemd/sd-id128/id128-util.c | 16 +++++++++------- src/libsystemd/sd-id128/sd-id128.c | 18 ++++++++++-------- src/test/test-id128.c | 12 ++++++------ 4 files changed, 26 insertions(+), 22 deletions(-) diff --git a/man/sd_id128_get_machine.xml b/man/sd_id128_get_machine.xml index dbc6d4885d..4f6926fd7d 100644 --- a/man/sd_id128_get_machine.xml +++ b/man/sd_id128_get_machine.xml @@ -166,7 +166,7 @@ - -EIO + -EUCLEAN Returned by any of the functions described here when the configured value has invalid format. diff --git a/src/libsystemd/sd-id128/id128-util.c b/src/libsystemd/sd-id128/id128-util.c index 3395ba2e58..eae2562410 100644 --- a/src/libsystemd/sd-id128/id128-util.c +++ b/src/libsystemd/sd-id128/id128-util.c @@ -43,6 +43,7 @@ bool id128_is_valid(const char *s) { int id128_read_fd(int fd, Id128FormatFlag f, sd_id128_t *ret) { char buffer[SD_ID128_UUID_STRING_MAX + 1]; /* +1 is for trailing newline */ ssize_t l; + int r; assert(fd >= 0); @@ -54,7 +55,7 @@ int id128_read_fd(int fd, Id128FormatFlag f, sd_id128_t *ret) { * This returns the following: * -ENOMEDIUM: an empty string, * -ENOPKG: "uninitialized" or "uninitialized\n", - * -EINVAL: other invalid strings. */ + * -EUCLEAN: other invalid strings. */ l = loop_read(fd, buffer, sizeof(buffer), false); /* we expect a short read of either 32/33 or 36/37 chars */ if (l < 0) @@ -70,33 +71,34 @@ int id128_read_fd(int fd, Id128FormatFlag f, sd_id128_t *ret) { case SD_ID128_STRING_MAX: /* plain UUID with trailing newline */ if (buffer[SD_ID128_STRING_MAX-1] != '\n') - return -EINVAL; + return -EUCLEAN; _fallthrough_; case SD_ID128_STRING_MAX-1: /* plain UUID without trailing newline */ if (!FLAGS_SET(f, ID128_FORMAT_PLAIN)) - return -EINVAL; + return -EUCLEAN; buffer[SD_ID128_STRING_MAX-1] = 0; break; case SD_ID128_UUID_STRING_MAX: /* RFC UUID with trailing newline */ if (buffer[SD_ID128_UUID_STRING_MAX-1] != '\n') - return -EINVAL; + return -EUCLEAN; _fallthrough_; case SD_ID128_UUID_STRING_MAX-1: /* RFC UUID without trailing newline */ if (!FLAGS_SET(f, ID128_FORMAT_UUID)) - return -EINVAL; + return -EUCLEAN; buffer[SD_ID128_UUID_STRING_MAX-1] = 0; break; default: - return -EINVAL; + return -EUCLEAN; } - return sd_id128_from_string(buffer, ret); + r = sd_id128_from_string(buffer, ret); + return r == -EINVAL ? -EUCLEAN : r; } int id128_read(const char *p, Id128FormatFlag f, sd_id128_t *ret) { diff --git a/src/libsystemd/sd-id128/sd-id128.c b/src/libsystemd/sd-id128/sd-id128.c index 8f9801ae37..146c379398 100644 --- a/src/libsystemd/sd-id128/sd-id128.c +++ b/src/libsystemd/sd-id128/sd-id128.c @@ -206,22 +206,22 @@ static int get_invocation_from_keyring(sd_id128_t *ret) { /* Chop off the final description string */ d = strrchr(description, ';'); if (!d) - return -EIO; + return -EUCLEAN; *d = 0; /* Look for the permissions */ p = strrchr(description, ';'); if (!p) - return -EIO; + return -EUCLEAN; errno = 0; perms = strtoul(p + 1, &e, 16); if (errno > 0) return -errno; if (e == p + 1) /* Read at least one character */ - return -EIO; + return -EUCLEAN; if (e != d) /* Must reached the end */ - return -EIO; + return -EUCLEAN; if ((perms & ~MAX_PERMS) != 0) return -EPERM; @@ -231,7 +231,7 @@ static int get_invocation_from_keyring(sd_id128_t *ret) { /* Look for the group ID */ g = strrchr(description, ';'); if (!g) - return -EIO; + return -EUCLEAN; r = parse_gid(g + 1, &gid); if (r < 0) return r; @@ -242,7 +242,7 @@ static int get_invocation_from_keyring(sd_id128_t *ret) { /* Look for the user ID */ u = strrchr(description, ';'); if (!u) - return -EIO; + return -EUCLEAN; r = parse_uid(u + 1, &uid); if (r < 0) return r; @@ -253,13 +253,14 @@ static int get_invocation_from_keyring(sd_id128_t *ret) { if (c < 0) return -errno; if (c != sizeof(sd_id128_t)) - return -EIO; + return -EUCLEAN; return 0; } static int get_invocation_from_environment(sd_id128_t *ret) { const char *e; + int r; assert(ret); @@ -267,7 +268,8 @@ static int get_invocation_from_environment(sd_id128_t *ret) { if (!e) return -ENXIO; - return sd_id128_from_string(e, ret); + r = sd_id128_from_string(e, ret); + return r == -EINVAL ? -EUCLEAN : r; } _public_ int sd_id128_get_invocation(sd_id128_t *ret) { diff --git a/src/test/test-id128.c b/src/test/test-id128.c index 6de0cec426..b7a9b03403 100644 --- a/src/test/test-id128.c +++ b/src/test/test-id128.c @@ -89,7 +89,7 @@ TEST(id128) { assert_se(id128_write_fd(fd, ID128_FORMAT_UUID, id) >= 0); assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2) == -EINVAL); + assert_se(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2) == -EUCLEAN); assert_se(lseek(fd, 0, SEEK_SET) == 0); assert_se(id128_read_fd(fd, ID128_FORMAT_UUID, &id2) >= 0); @@ -107,7 +107,7 @@ TEST(id128) { assert_se(id128_write_fd(fd, ID128_FORMAT_PLAIN, id) >= 0); assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_UUID, &id2) == -EINVAL); + assert_se(id128_read_fd(fd, ID128_FORMAT_UUID, &id2) == -EUCLEAN); assert_se(lseek(fd, 0, SEEK_SET) == 0); assert_se(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2) >= 0); @@ -125,7 +125,7 @@ TEST(id128) { assert_se(write(fd, sd_id128_to_string(id, t), 32) == 32); assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_UUID, &id2) == -EINVAL); + assert_se(id128_read_fd(fd, ID128_FORMAT_UUID, &id2) == -EUCLEAN); assert_se(lseek(fd, 0, SEEK_SET) == 0); assert_se(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2) >= 0); @@ -139,7 +139,7 @@ TEST(id128) { assert_se(write(fd, sd_id128_to_uuid_string(id, q), 36) == 36); assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2) == -EINVAL); + assert_se(id128_read_fd(fd, ID128_FORMAT_PLAIN, &id2) == -EUCLEAN); assert_se(lseek(fd, 0, SEEK_SET) == 0); assert_se(id128_read_fd(fd, ID128_FORMAT_UUID, &id2) >= 0); @@ -162,13 +162,13 @@ TEST(id128) { assert_se(ftruncate(fd, 0) >= 0); assert_se(write(fd, "uninitialized\nfoo", STRLEN("uninitialized\nfoo")) == STRLEN("uninitialized\nfoo")); assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_ANY, NULL) == -EINVAL); + assert_se(id128_read_fd(fd, ID128_FORMAT_ANY, NULL) == -EUCLEAN); assert_se(lseek(fd, 0, SEEK_SET) == 0); assert_se(ftruncate(fd, 0) >= 0); assert_se(write(fd, "uninit", STRLEN("uninit")) == STRLEN("uninit")); assert_se(lseek(fd, 0, SEEK_SET) == 0); - assert_se(id128_read_fd(fd, ID128_FORMAT_ANY, NULL) == -EINVAL); + assert_se(id128_read_fd(fd, ID128_FORMAT_ANY, NULL) == -EUCLEAN); if (sd_booted() > 0 && access("/etc/machine-id", F_OK) >= 0) { assert_se(sd_id128_get_machine_app_specific(SD_ID128_MAKE(f0,3d,aa,eb,1c,33,4b,43,a7,32,17,29,44,bf,77,2e), &id) >= 0); From 786b652c8989834f9218ec82b2d824d5b753fad3 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 14 Dec 2022 13:34:15 +0900 Subject: [PATCH 2/4] sd-id128: allow sd_id128_get_machine() and friend to be called with NULL It may be useful to check if the machine ID or friends is set or not. --- src/libsystemd/sd-id128/sd-id128.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/src/libsystemd/sd-id128/sd-id128.c b/src/libsystemd/sd-id128/sd-id128.c index 146c379398..9fac1bb3fc 100644 --- a/src/libsystemd/sd-id128/sd-id128.c +++ b/src/libsystemd/sd-id128/sd-id128.c @@ -124,8 +124,6 @@ _public_ int sd_id128_get_machine(sd_id128_t *ret) { static thread_local sd_id128_t saved_machine_id = {}; int r; - assert_return(ret, -EINVAL); - if (sd_id128_is_null(saved_machine_id)) { r = id128_read("/etc/machine-id", ID128_FORMAT_PLAIN, &saved_machine_id); if (r < 0) @@ -135,7 +133,8 @@ _public_ int sd_id128_get_machine(sd_id128_t *ret) { return -ENOMEDIUM; } - *ret = saved_machine_id; + if (ret) + *ret = saved_machine_id; return 0; } @@ -143,8 +142,6 @@ _public_ int sd_id128_get_boot(sd_id128_t *ret) { static thread_local sd_id128_t saved_boot_id = {}; int r; - assert_return(ret, -EINVAL); - if (sd_id128_is_null(saved_boot_id)) { r = id128_read("/proc/sys/kernel/random/boot_id", ID128_FORMAT_UUID, &saved_boot_id); if (r == -ENOENT && proc_mounted() == 0) @@ -156,7 +153,8 @@ _public_ int sd_id128_get_boot(sd_id128_t *ret) { return -ENOMEDIUM; } - *ret = saved_boot_id; + if (ret) + *ret = saved_boot_id; return 0; } @@ -276,26 +274,20 @@ _public_ int sd_id128_get_invocation(sd_id128_t *ret) { static thread_local sd_id128_t saved_invocation_id = {}; int r; - assert_return(ret, -EINVAL); - if (sd_id128_is_null(saved_invocation_id)) { /* We first check the environment. The environment variable is primarily relevant for user * services, and sufficiently safe as long as no privilege boundary is involved. */ r = get_invocation_from_environment(&saved_invocation_id); - if (r >= 0) { - *ret = saved_invocation_id; - return 0; - } else if (r != -ENXIO) - return r; - - /* The kernel keyring is relevant for system services (as for user services we don't store - * the invocation ID in the keyring, as there'd be no trust benefit in that). */ - r = get_invocation_from_keyring(&saved_invocation_id); + if (r == -ENXIO) + /* The kernel keyring is relevant for system services (as for user services we don't + * store the invocation ID in the keyring, as there'd be no trust benefit in that). */ + r = get_invocation_from_keyring(&saved_invocation_id); if (r < 0) return r; } - *ret = saved_invocation_id; + if (ret) + *ret = saved_invocation_id; return 0; } From 75fa1f25c08a4b5dc2180893718473be9e4f6bab Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 14 Dec 2022 13:40:42 +0900 Subject: [PATCH 3/4] sd-id128: also refuse an empty invocation ID --- man/sd_id128_get_machine.xml | 3 ++- src/libsystemd/sd-id128/sd-id128.c | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/man/sd_id128_get_machine.xml b/man/sd_id128_get_machine.xml index 4f6926fd7d..075caffb2b 100644 --- a/man/sd_id128_get_machine.xml +++ b/man/sd_id128_get_machine.xml @@ -139,7 +139,8 @@ Returned by sd_id128_get_machine() and sd_id128_get_machine_app_specific() when /etc/machine-id - is empty or all zeros. + is empty or all zeros. Also returned by sd_id128_get_invocation() when the + invocation ID is all zeros. diff --git a/src/libsystemd/sd-id128/sd-id128.c b/src/libsystemd/sd-id128/sd-id128.c index 9fac1bb3fc..ec3a496dba 100644 --- a/src/libsystemd/sd-id128/sd-id128.c +++ b/src/libsystemd/sd-id128/sd-id128.c @@ -284,6 +284,9 @@ _public_ int sd_id128_get_invocation(sd_id128_t *ret) { r = get_invocation_from_keyring(&saved_invocation_id); if (r < 0) return r; + + if (sd_id128_is_null(saved_invocation_id)) + return -ENOMEDIUM; } if (ret) From f0d8358c245b4d0012d8db52dbf42bdebbe1b2bc Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 14 Dec 2022 14:29:25 +0900 Subject: [PATCH 4/4] man: update documents for sd_id128_get_invocation() --- man/sd_id128_get_machine.xml | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/man/sd_id128_get_machine.xml b/man/sd_id128_get_machine.xml index 075caffb2b..cdab87ab63 100644 --- a/man/sd_id128_get_machine.xml +++ b/man/sd_id128_get_machine.xml @@ -94,10 +94,20 @@ has properties similar to the machine ID during that time. sd_id128_get_invocation() returns the invocation ID of the currently executed - service. In its current implementation, this reads and parses the $INVOCATION_ID environment - variable that the service manager sets when activating a service, see - systemd.exec5 for details. The - ID is cached internally. In future a different mechanism to determine the invocation ID may be added. + service. In its current implementation, this tries to read and parse the following: + + + The $INVOCATION_ID environment variable that the service manager sets when + activating a service. + + + An entry in the kernel keyring that the system service manager sets when activating a service. + + + + See systemd.exec5 + for details. The ID is cached internally. In future a different mechanism to determine the invocation ID + may be added. Note that sd_id128_get_machine_app_specific(), sd_id128_get_boot(), sd_id128_get_boot_app_specific(), and