Merge pull request #20609 from DaanDeMeyer/recursive-template

core: Try to prevent infinite recursive template instantiation

man/systemd.unit.xml

@@ -2256,8 +2256,12 @@ OnFailure=failure-handler@%N.service
     services will have acquired an <varname>OnFailure=</varname> dependency on
     <filename index='false'>failure-handler@%N.service</filename>. The
     template instance units will also have gained the dependency which results
-    in the creation of a recursive dependency chain. We can break the chain by
-    disabling the drop-in for the template instance units via a symlink to
+    in the creation of a recursive dependency chain. systemd will try to detect
+    these recursive dependency chains where a template unit directly and
+    recursively depends on itself and will remove such dependencies
+    automatically if it finds them. If systemd doesn't detect the recursive
+    dependency chain, we can break the chain ourselves by disabling the drop-in
+    for the template instance units via a symlink to
     <filename index='false'>/dev/null</filename>:</para>
 
     <programlisting>

src/basic/unit-file.c

@@ -598,9 +598,11 @@ int unit_file_find_fragment(
         if (name_type < 0)
                 return name_type;
 
-        r = add_names(unit_ids_map, unit_name_map, unit_name, NULL, name_type, instance, &names, unit_name);
-        if (r < 0)
-                return r;
+        if (ret_names) {
+                r = add_names(unit_ids_map, unit_name_map, unit_name, NULL, name_type, instance, &names, unit_name);
+                if (r < 0)
+                        return r;
+        }
 
         /* First try to load fragment under the original name */
         r = unit_ids_map_get(unit_ids_map, unit_name, &fragment);
@@ -619,7 +621,7 @@ int unit_file_find_fragment(
                         return log_debug_errno(r, "Cannot load template %s: %m", template);
         }
 
-        if (fragment) {
+        if (fragment && ret_names) {
                 const char *fragment_basename = basename(fragment);
 
                 if (!streq(fragment_basename, unit_name)) {
@@ -631,7 +633,8 @@ int unit_file_find_fragment(
         }
 
         *ret_fragment_path = fragment;
-        *ret_names = TAKE_PTR(names);
+        if (ret_names)
+                *ret_names = TAKE_PTR(names);
 
         return 0;
 }

src/basic/unit-name.c

@@ -8,6 +8,7 @@
 #include "alloc-util.h"
 #include "glob-util.h"
 #include "hexdecoct.h"
+#include "memory-util.h"
 #include "path-util.h"
 #include "special.h"
 #include "string-util.h"
@@ -797,3 +798,26 @@ bool slice_name_is_valid(const char *name) {
 
         return true;
 }
+
+bool unit_name_prefix_equal(const char *a, const char *b) {
+        const char *p, *q;
+
+        assert(a);
+        assert(b);
+
+        if (!unit_name_is_valid(a, UNIT_NAME_ANY) || !unit_name_is_valid(b, UNIT_NAME_ANY))
+                return false;
+
+        p = strchr(a, '@');
+        if (!p)
+                p = strrchr(a, '.');
+
+        q = strchr(b, '@');
+        if (!q)
+                q = strrchr(b, '.');
+
+        assert(p);
+        assert(q);
+
+        return memcmp_nn(a, p - a, b, q - b) == 0;
+}

src/basic/unit-name.h

@@ -62,3 +62,5 @@ static inline int unit_name_mangle(const char *name, UnitNameMangle flags, char
 int slice_build_parent_slice(const char *slice, char **ret);
 int slice_build_subslice(const char *slice, const char *name, char **subslice);
 bool slice_name_is_valid(const char *name);
+
+bool unit_name_prefix_equal(const char *a, const char *b);

src/core/load-fragment.c

@@ -150,6 +150,84 @@ DEFINE_CONFIG_PARSE_ENUM_WITH_DEFAULT(config_parse_numa_policy, mpol, int, -1, "
 DEFINE_CONFIG_PARSE_ENUM(config_parse_status_unit_format, status_unit_format, StatusUnitFormat, "Failed to parse status unit format");
 DEFINE_CONFIG_PARSE_ENUM_FULL(config_parse_socket_timestamping, socket_timestamping_from_string_harder, SocketTimestamping, "Failed to parse timestamping precision");
 
+bool contains_instance_specifier_superset(const char *s) {
+        const char *p, *q;
+        bool percent = false;
+
+        assert(s);
+
+        p = strchr(s, '@');
+        if (!p)
+                return false;
+
+        p++; /* Skip '@' */
+
+        q = strrchr(p, '.');
+        if (!q)
+                q = p + strlen(p);
+
+        /* If the string is just the instance specifier, it's not a superset of the instance. */
+        if (memcmp_nn(p, q - p, "%i", strlen("%i")) == 0)
+                return false;
+
+        /* %i, %n and %N all expand to the instance or a superset of it. */
+        for (; p < q; p++) {
+                if (*p == '%')
+                        percent = !percent;
+                else if (percent) {
+                        if (IN_SET(*p, 'n', 'N', 'i'))
+                                return true;
+                        percent = false;
+                }
+        }
+
+        return false;
+}
+
+/* `name` is the rendered version of `format` via `unit_printf` or similar functions. */
+int unit_is_likely_recursive_template_dependency(Unit *u, const char *name, const char *format) {
+        const char *fragment_path;
+        int r;
+
+        assert(u);
+        assert(name);
+
+        /* If a template unit has a direct dependency on itself that includes the unit instance as part of
+         * the template instance via a unit specifier (%i, %n or %N), this will almost certainly lead to
+         * infinite recursion as systemd will keep instantiating new instances of the template unit.
+         * https://github.com/systemd/systemd/issues/17602 shows a good example of how this can happen in
+         * practice. To guard against this, we check for templates that depend on themselves and have the
+         * instantiated unit instance included as part of the template instance of the dependency via a
+         * specifier.
+         *
+         * For example, if systemd-notify@.service depends on systemd-notify@%n.service, this will result in
+         * infinite recursion.
+         */
+
+        if (!unit_name_is_valid(name, UNIT_NAME_INSTANCE))
+                return false;
+
+        if (!unit_name_prefix_equal(u->id, name))
+                return false;
+
+        if (u->type != unit_name_to_type(name))
+                return false;
+
+        r = unit_file_find_fragment(u->manager->unit_id_map, u->manager->unit_name_map, name, &fragment_path, NULL);
+        if (r < 0)
+                return r;
+
+        /* Fragment paths should also be equal as a custom fragment for a specific template instance
+         * wouldn't necessarily lead to infinite recursion. */
+        if (!path_equal_ptr(u->fragment_path, fragment_path))
+                return false;
+
+        if (!contains_instance_specifier_superset(format))
+                return false;
+
+        return true;
+}
+
 int config_parse_unit_deps(
                 const char *unit,
                 const char *filename,
@@ -189,6 +267,18 @@ int config_parse_unit_deps(
                         continue;
                 }
 
+                r = unit_is_likely_recursive_template_dependency(u, k, word);
+                if (r < 0) {
+                        log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to determine if '%s' is a recursive dependency, ignoring: %m", k);
+                        continue;
+                }
+                if (r > 0) {
+                        log_syntax(unit, LOG_DEBUG, filename, line, 0,
+                                   "Dropping dependency %s=%s that likely leads to infinite recursion.",
+                                   unit_dependency_to_string(d), word);
+                        continue;
+                }
+
                 r = unit_add_dependency_by_name(u, d, k, true, UNIT_DEPENDENCY_FILE);
                 if (r < 0)
                         log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to add dependency on %s, ignoring: %m", k);

src/core/load-fragment.h

@@ -4,6 +4,10 @@
 #include "conf-parser.h"
 #include "unit.h"
 
+/* These functions are declared in the header to make them accessible to unit tests. */
+bool contains_instance_specifier_superset(const char *s);
+int unit_is_likely_recursive_template_dependency(Unit *u, const char *name, const char *format);
+
 /* Config-parsing helpers relevant only for sources under src/core/ */
 int parse_crash_chvt(const char *value, int *data);
 int parse_confirm_spawn(const char *value, char **console);

src/test/test-load-fragment.c

@@ -829,6 +829,71 @@ static void test_config_parse_memory_limit(void) {
 
 }
 
+static void test_contains_instance_specifier_superset(void) {
+        assert_se(contains_instance_specifier_superset("foobar@a%i"));
+        assert_se(contains_instance_specifier_superset("foobar@%ia"));
+        assert_se(contains_instance_specifier_superset("foobar@%n"));
+        assert_se(contains_instance_specifier_superset("foobar@%n.service"));
+        assert_se(contains_instance_specifier_superset("foobar@%N"));
+        assert_se(contains_instance_specifier_superset("foobar@%N.service"));
+        assert_se(contains_instance_specifier_superset("foobar@baz.%N.service"));
+        assert_se(contains_instance_specifier_superset("@%N.service"));
+        assert_se(contains_instance_specifier_superset("@%N"));
+        assert_se(contains_instance_specifier_superset("@%a%N"));
+
+        assert_se(!contains_instance_specifier_superset("foobar@%i.service"));
+        assert_se(!contains_instance_specifier_superset("foobar%ia.service"));
+        assert_se(!contains_instance_specifier_superset("foobar@%%n.service"));
+        assert_se(!contains_instance_specifier_superset("foobar@baz.service"));
+        assert_se(!contains_instance_specifier_superset("%N.service"));
+        assert_se(!contains_instance_specifier_superset("%N"));
+        assert_se(!contains_instance_specifier_superset("@%aN"));
+        assert_se(!contains_instance_specifier_superset("@%a%b"));
+}
+
+static void test_unit_is_recursive_template_dependency(void) {
+        _cleanup_(manager_freep) Manager *m = NULL;
+        Unit *u;
+        int r;
+
+        r = manager_new(UNIT_FILE_USER, MANAGER_TEST_RUN_MINIMAL, &m);
+        if (manager_errno_skip_test(r)) {
+                log_notice_errno(r, "Skipping test: manager_new: %m");
+                return;
+        }
+
+        assert_se(r >= 0);
+        assert_se(manager_startup(m, NULL, NULL, NULL) >= 0);
+
+        assert_se(u = unit_new(m, sizeof(Service)));
+        assert_se(unit_add_name(u, "foobar@1.service") == 0);
+        u->fragment_path = strdup("/foobar@.service");
+
+        assert_se(hashmap_put_strdup(&m->unit_id_map, "foobar@foobar@123.service", "/foobar@.service"));
+        assert_se(hashmap_put_strdup(&m->unit_id_map, "foobar@foobar@456.service", "/custom.service"));
+
+        /* Test that %n, %N and any extension of %i specifiers in the instance are detected as recursive. */
+        assert_se(unit_is_likely_recursive_template_dependency(u, "foobar@foobar@123.service", "foobar@%N.service") == 1);
+        assert_se(unit_is_likely_recursive_template_dependency(u, "foobar@foobar@123.service", "foobar@%n.service") == 1);
+        assert_se(unit_is_likely_recursive_template_dependency(u, "foobar@foobar@123.service", "foobar@a%i.service") == 1);
+        assert_se(unit_is_likely_recursive_template_dependency(u, "foobar@foobar@123.service", "foobar@%ia.service") == 1);
+        assert_se(unit_is_likely_recursive_template_dependency(u, "foobar@foobar@123.service", "foobar@%x%n.service") == 1);
+        /* Test that %i on its own is not detected as recursive. */
+        assert_se(unit_is_likely_recursive_template_dependency(u, "foobar@foobar@123.service", "foobar@%i.service") == 0);
+        /* Test that a specifier other than %i, %n and %N is not detected as recursive. */
+        assert_se(unit_is_likely_recursive_template_dependency(u, "foobar@foobar@123.service", "foobar@%xn.service") == 0);
+        /* Test that an expanded specifier is not detected as recursive. */
+        assert_se(unit_is_likely_recursive_template_dependency(u, "foobar@foobar@123.service", "foobar@foobar@123.service") == 0);
+        /* Test that a dependency with a custom fragment path is not detected as recursive. */
+        assert_se(unit_is_likely_recursive_template_dependency(u, "foobar@foobar@456.service", "foobar@%n.service") == 0);
+        /* Test that a dependency without a fragment path is not detected as recursive. */
+        assert_se(unit_is_likely_recursive_template_dependency(u, "foobar@foobar@789.service", "foobar@%n.service") == 0);
+        /* Test that a dependency with a different prefix is not detected as recursive. */
+        assert_se(unit_is_likely_recursive_template_dependency(u, "quux@foobar@123.service", "quux@%n.service") == 0);
+        /* Test that a dependency of a different type is not detected as recursive. */
+        assert_se(unit_is_likely_recursive_template_dependency(u, "foobar@foobar@123.mount", "foobar@%n.mount") == 0);
+}
+
 int main(int argc, char *argv[]) {
         _cleanup_(rm_rf_physical_and_freep) char *runtime_dir = NULL;
         int r;
@@ -850,6 +915,8 @@ int main(int argc, char *argv[]) {
         TEST_REQ_RUNNING_SYSTEMD(test_install_printf());
         test_unit_dump_config_items();
         test_config_parse_memory_limit();
+        test_contains_instance_specifier_superset();
+        test_unit_is_recursive_template_dependency();
 
         return r;
 }

src/test/test-unit-file.c

@@ -71,6 +71,19 @@ static void test_unit_file_build_name_map(char **ids) {
                  SET_FOREACH(name, names)
                          log_info("    %s", name);
         }
+
+        /* Make sure everything still works if we don't collect names. */
+        STRV_FOREACH(id, ids) {
+                 const char *fragment;
+                 log_info("*** %s ***", *id);
+                 r = unit_file_find_fragment(unit_ids,
+                                             unit_names,
+                                             *id,
+                                             &fragment,
+                                             NULL);
+                 assert_se(r == 0);
+                 log_info("fragment: %s", fragment);
+        }
 }
 
 static void test_runlevel_to_target(void) {

src/test/test-unit-name.c

@@ -870,6 +870,22 @@ static void test_unit_name_from_dbus_path(void) {
         test_unit_name_from_dbus_path_one("/org/freedesktop/systemd1/unit/wpa_5fsupplicant_2eservice", 0, "wpa_supplicant.service");
 }
 
+static void test_unit_name_prefix_equal(void) {
+        log_info("/* %s */", __func__);
+
+        assert_se(unit_name_prefix_equal("a.service", "a.service"));
+        assert_se(unit_name_prefix_equal("a.service", "a.mount"));
+        assert_se(unit_name_prefix_equal("a@b.service", "a.service"));
+        assert_se(unit_name_prefix_equal("a@b.service", "a@c.service"));
+
+        assert_se(!unit_name_prefix_equal("a.service", "b.service"));
+        assert_se(!unit_name_prefix_equal("a.service", "b.mount"));
+        assert_se(!unit_name_prefix_equal("a@a.service", "b.service"));
+        assert_se(!unit_name_prefix_equal("a@a.service", "b@a.service"));
+        assert_se(!unit_name_prefix_equal("a", "b"));
+        assert_se(!unit_name_prefix_equal("a", "a"));
+}
+
 int main(int argc, char* argv[]) {
         _cleanup_(rm_rf_physical_and_freep) char *runtime_dir = NULL;
         int r, rc = 0;
@@ -902,6 +918,7 @@ int main(int argc, char* argv[]) {
         test_unit_name_path_unescape();
         test_unit_name_to_prefix();
         test_unit_name_from_dbus_path();
+        test_unit_name_prefix_equal();
 
         return rc;
 }