Merge pull request #15911 from poettering/unit-name-tighten

pid1: improve logging when we encounter a path that is too long to be converted into a mount unit name

5 files changed, 111 insertions, 28 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/basic/unit-name.c b/src/basic/unit-name.c
index 521364124f..10405b711f 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,12 +690,13 @@ good:
         if (!s)
                 return -ENOMEM;
 
-        *ret = s;
+        *ret = TAKE_PTR(s);
         return 0;
 }
 
 int slice_build_parent_slice(const char *slice, char **ret) {
-        char *s, *dash;
+        _cleanup_free_ char *s = NULL;
+        char *dash;
         int r;
 
         assert(slice);
@@ -693,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;
 }
 
diff --git a/src/core/mount.c b/src/core/mount.c
index 10613d11ae..8b30a4db6a 100644
--- a/src/core/mount.c
+++ b/src/core/mount.c
@@ -1674,9 +1674,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)
@@ -1686,7 +1707,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/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)
 
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