From ab19db01ae1826efb3cbdf6dcb6a14412f8844d4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 25 May 2020 00:34:58 +0200 Subject: [PATCH 1/4] unit-name: tighten checks for building valid unit names Let's be more thorough that whenever we build a unit name based on parameters, that the result is actually a valid user name. If it isn't fail early. This should allows us to catch various issues earlier, in particular when we synthesize mount units from /proc/self/mountinfo: instead of actually attempting to allocate a mount unit we will fail much earlier when we build the name to synthesize the unit under. Failing early is a good thing generally. --- src/basic/unit-name.c | 63 ++++++++++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 19 deletions(-) diff --git a/src/basic/unit-name.c b/src/basic/unit-name.c index 521364124f..312168054a 100644 --- a/src/basic/unit-name.c +++ b/src/basic/unit-name.c @@ -207,8 +207,9 @@ UnitType unit_name_to_type(const char *n) { } int unit_name_change_suffix(const char *n, const char *suffix, char **ret) { - char *e, *s; + _cleanup_free_ char *s = NULL; size_t a, b; + char *e; assert(n); assert(suffix); @@ -230,8 +231,12 @@ int unit_name_change_suffix(const char *n, const char *suffix, char **ret) { return -ENOMEM; strcpy(mempcpy(s, n, a), suffix); - *ret = s; + /* Make sure the name is still valid (i.e. didn't grow too large due to longer suffix) */ + if (!unit_name_is_valid(s, UNIT_NAME_ANY)) + return -EINVAL; + + *ret = TAKE_PTR(s); return 0; } @@ -253,8 +258,8 @@ int unit_name_build(const char *prefix, const char *instance, const char *suffix } int unit_name_build_from_type(const char *prefix, const char *instance, UnitType type, char **ret) { + _cleanup_free_ char *s = NULL; const char *ut; - char *s; assert(prefix); assert(type >= 0); @@ -264,19 +269,23 @@ int unit_name_build_from_type(const char *prefix, const char *instance, UnitType if (!unit_prefix_is_valid(prefix)) return -EINVAL; - if (instance && !unit_instance_is_valid(instance)) - return -EINVAL; - ut = unit_type_to_string(type); - if (!instance) - s = strjoin(prefix, ".", ut); - else + if (instance) { + if (!unit_instance_is_valid(instance)) + return -EINVAL; + s = strjoin(prefix, "@", instance, ".", ut); + } else + s = strjoin(prefix, ".", ut); if (!s) return -ENOMEM; - *ret = s; + /* Verify that this didn't grow too large (or otherwise is invalid) */ + if (!unit_name_is_valid(s, instance ? UNIT_NAME_INSTANCE : UNIT_NAME_PLAIN)) + return -EINVAL; + + *ret = TAKE_PTR(s); return 0; } @@ -441,8 +450,8 @@ int unit_name_path_unescape(const char *f, char **ret) { } int unit_name_replace_instance(const char *f, const char *i, char **ret) { + _cleanup_free_ char *s = NULL; const char *p, *e; - char *s; size_t a, b; assert(f); @@ -466,7 +475,11 @@ int unit_name_replace_instance(const char *f, const char *i, char **ret) { strcpy(mempcpy(mempcpy(s, f, a + 1), i, b), e); - *ret = s; + /* Make sure the resulting name still is valid, i.e. didn't grow too large */ + if (!unit_name_is_valid(s, UNIT_NAME_INSTANCE)) + return -EINVAL; + + *ret = TAKE_PTR(s); return 0; } @@ -497,8 +510,7 @@ int unit_name_template(const char *f, char **ret) { } int unit_name_from_path(const char *path, const char *suffix, char **ret) { - _cleanup_free_ char *p = NULL; - char *s = NULL; + _cleanup_free_ char *p = NULL, *s = NULL; int r; assert(path); @@ -516,7 +528,11 @@ int unit_name_from_path(const char *path, const char *suffix, char **ret) { if (!s) return -ENOMEM; - *ret = s; + /* Refuse this if this got too long or for some other reason didn't result in a valid name */ + if (!unit_name_is_valid(s, UNIT_NAME_PLAIN)) + return -EINVAL; + + *ret = TAKE_PTR(s); return 0; } @@ -544,6 +560,10 @@ int unit_name_from_path_instance(const char *prefix, const char *path, const cha if (!s) return -ENOMEM; + /* Refuse this if this got too long or for some other reason didn't result in a valid name */ + if (!unit_name_is_valid(s, UNIT_NAME_INSTANCE)) + return -EINVAL; + *ret = s; return 0; } @@ -597,9 +617,9 @@ static bool do_escape_mangle(const char *f, bool allow_globs, char *t) { * If @allow_globs, globs characters are preserved. Otherwise, they are escaped. */ int unit_name_mangle_with_suffix(const char *name, const char *operation, UnitNameMangle flags, const char *suffix, char **ret) { - char *s; - int r; + _cleanup_free_ char *s = NULL; bool mangled, suggest_escape = true; + int r; assert(name); assert(suffix); @@ -657,7 +677,12 @@ int unit_name_mangle_with_suffix(const char *name, const char *operation, UnitNa if ((!(flags & UNIT_NAME_MANGLE_GLOB) || !string_is_glob(s)) && unit_name_to_type(s) < 0) strcat(s, suffix); - *ret = s; + /* Make sure mangling didn't grow this too large (but don't do this check if globbing is allowed, + * since globs generally do not qualify as valid unit names) */ + if (!FLAGS_SET(flags, UNIT_NAME_MANGLE_GLOB) && !unit_name_is_valid(s, UNIT_NAME_ANY)) + return -EINVAL; + + *ret = TAKE_PTR(s); return 1; good: @@ -665,7 +690,7 @@ good: if (!s) return -ENOMEM; - *ret = s; + *ret = TAKE_PTR(s); return 0; } From 5aec5c751a86ec2430b1d38ed925ce29ed35407b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 25 May 2020 00:37:15 +0200 Subject: [PATCH 2/4] unit-name: use TAKE_PTR() where appropriate --- src/basic/unit-name.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/basic/unit-name.c b/src/basic/unit-name.c index 312168054a..10405b711f 100644 --- a/src/basic/unit-name.c +++ b/src/basic/unit-name.c @@ -695,7 +695,8 @@ good: } int slice_build_parent_slice(const char *slice, char **ret) { - char *s, *dash; + _cleanup_free_ char *s = NULL; + char *dash; int r; assert(slice); @@ -718,13 +719,11 @@ int slice_build_parent_slice(const char *slice, char **ret) { strcpy(dash, ".slice"); else { r = free_and_strdup(&s, SPECIAL_ROOT_SLICE); - if (r < 0) { - free(s); + if (r < 0) return r; - } } - *ret = s; + *ret = TAKE_PTR(s); return 1; } From 2c905207db37c691d4abef868165ad5ea2dd0f4f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 25 May 2020 00:37:26 +0200 Subject: [PATCH 3/4] core: improve log messages when we cannot process a mount point Whenever we pick up a new line in /proc/self/mountinfo and want to synthesize a new mount unit from it, let's say which one it is. Moreover, downgrade the log message when we encounter a mount point with an overly long name to LOG_WARNING, since it's generally fine to ignore such mount points. Also, attach a catalog entry to explain the situation further. Prompted-By: #15221 --- catalog/systemd.catalog.in | 31 +++++++++++++++++++++++++++++++ src/core/mount.c | 27 ++++++++++++++++++++++++--- src/systemd/sd-messages.h | 5 +++++ 3 files changed, 60 insertions(+), 3 deletions(-) diff --git a/catalog/systemd.catalog.in b/catalog/systemd.catalog.in index 3342d59422..b29ba6d65b 100644 --- a/catalog/systemd.catalog.in +++ b/catalog/systemd.catalog.in @@ -435,3 +435,34 @@ hyphen and otherwise fully numeric. For further details on strict and relaxed user/group name rules, see: https://systemd.io/USER_NAMES + +-- 1b3bb94037f04bbf81028e135a12d293 +Subject: Failed to generate valid unit name from path '@MOUNT_POINT@'. +Defined-By: systemd +Support: %SUPPORT_URL% + +The following mount point path could not be converted into a valid .mount +unit name: + + @MOUNT_POINT@ + +Typically this means that the path to the mount point is longer than allowed +for valid unit names. + +systemd dynamically synthesizes .mount units for all mount points appearing on +the system. For that a simple escaping algorithm is applied: the absolute path +name is used, with all "/" characters replaced by "-" (the leading one is +removed). Moreover, any non-alphanumeric characters (as well as any of ":", +"-", "_", ".", "\") are replaced by "\xNN" where "NN" is the hexadecimal code +of the character. Finally, ".mount" is suffixed. The resulting string must be +under 256 characters in length to be a valid unit name. This restriction is +made in order for all unit names to also be suitable as file names. If a mount +point appears that — after escaping — is longer than this limit it cannot be +mapped to a unit. In this case systemd will refrain from synthesizing a unit +and cannot be used to manage the mount point. It will not appear in the service +manager's unit table and thus also not be torn down safely and automatically at +system shutdown. + +It is generally recommended to avoid such overly long mount point paths, or — +if used anyway – manage them independently of systemd, i.e. establish them as +well as tear them down automatically at system shutdown by other software. diff --git a/src/core/mount.c b/src/core/mount.c index 48500b4932..86a6098332 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1672,9 +1672,30 @@ static int mount_setup_unit( if (!is_path(where)) return 0; + /* Mount unit names have to be (like all other unit names) short enough to fit into file names. This + * means there's a good chance that overly long mount point paths after mangling them to look like a + * unit name would result in unit names we don't actually consider valid. This should be OK however + * as such long mount point paths should not happen on regular systems — and if they appear + * nonetheless they are generally synthesized by software, and thus managed by that other + * software. Having such long names just means you cannot use systemd to manage those specific mount + * points, which should be an OK restriction to make. After all we don't have to be able to manage + * all mount points in the world — as long as we don't choke on them when we encounter them. */ r = unit_name_from_path(where, ".mount", &e); - if (r < 0) - return log_error_errno(r, "Failed to generate unit name from path '%s': %m", where); + if (r < 0) { + static RateLimit rate_limit = { /* Let's log about this at warning level at most once every + * 5s. Given that we generate this whenever we read the file + * otherwise we probably shouldn't flood the logs with + * this */ + .interval = 5 * USEC_PER_SEC, + .burst = 1, + }; + + return log_struct_errno( + ratelimit_below(&rate_limit) ? LOG_WARNING : LOG_DEBUG, r, + "MESSAGE_ID=" SD_MESSAGE_MOUNT_POINT_PATH_NOT_SUITABLE_STR, + "MOUNT_POINT=%s", where, + LOG_MESSAGE("Failed to generate valid unit name from path '%s', ignoring mount point: %m", where)); + } u = manager_get_unit(m, e); if (u) @@ -1684,7 +1705,7 @@ static int mount_setup_unit( * by the sysadmin having called mount(8) directly. */ r = mount_setup_new_unit(m, e, what, where, options, fstype, &flags, &u); if (r < 0) - return log_warning_errno(r, "Failed to set up mount unit: %m"); + return log_warning_errno(r, "Failed to set up mount unit for '%s': %m", where); /* If the mount changed properties or state, let's notify our clients */ if (flags & (MOUNT_PROC_JUST_CHANGED|MOUNT_PROC_JUST_MOUNTED)) diff --git a/src/systemd/sd-messages.h b/src/systemd/sd-messages.h index 162b650e64..f5dd0a04c7 100644 --- a/src/systemd/sd-messages.h +++ b/src/systemd/sd-messages.h @@ -161,6 +161,11 @@ _SD_BEGIN_DECLARATIONS; #define SD_MESSAGE_UNSAFE_USER_NAME SD_ID128_MAKE(b6,1f,da,c6,12,e9,4b,91,82,28,5b,99,88,43,06,1f) #define SD_MESSAGE_UNSAFE_USER_NAME_STR SD_ID128_MAKE_STR(b6,1f,da,c6,12,e9,4b,91,82,28,5b,99,88,43,06,1f) +#define SD_MESSAGE_MOUNT_POINT_PATH_NOT_SUITABLE \ + SD_ID128_MAKE(1b,3b,b9,40,37,f0,4b,bf,81,02,8e,13,5a,12,d2,93) +#define SD_MESSAGE_MOUNT_POINT_PATH_NOT_SUITABLE_STR \ + SD_ID128_MAKE_STR(1b,3b,b9,40,37,f0,4b,bf,81,02,8e,13,5a,12,d2,93) + _SD_END_DECLARATIONS; #endif From 8dd7cbce42ba3fb31482964a9c0e772a5b5f5d0a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 25 May 2020 16:58:06 +0200 Subject: [PATCH 4/4] journal: expand variables up to PATH_MAX in size in catalog output It makes sense to be able to output every officially valid path, hence bump the variable size limit in catalog entries to 4K (i.e. PATH_MAX). --- src/journal/sd-journal.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/journal/sd-journal.c b/src/journal/sd-journal.c index 9b6c425285..80cd80f356 100644 --- a/src/journal/sd-journal.c +++ b/src/journal/sd-journal.c @@ -45,7 +45,9 @@ #define JOURNAL_FILES_RECHECK_USEC (2 * USEC_PER_SEC) -#define REPLACE_VAR_MAX 256 +/* The maximum size of variable values we'll expand in catalog entries. We bind this to PATH_MAX for now, as + * we want to be able to show all officially valid paths at least */ +#define REPLACE_VAR_MAX PATH_MAX #define DEFAULT_DATA_THRESHOLD (64*1024)