Fail on unknown (alphanumerical) specifiers

The code intentionally ignored unknown specifiers, treating them as text. This
needs to change because otherwise we can never add a new specifier in a backwards
compatible way. So just treat an unknown (potential) specifier as an error.

In principle this is a break of backwards compatibility, but the previous
behaviour was pretty much useless, since the expanded value could change every
time we add new specifiers, which we do all the time.

As a compromise for backwards compatibility, only fail on alphanumerical
characters. This should cover the most cases where an unescaped percent
character is used, like size=5% and such, which behave the same as before with
this patch. OTOH, this means that we will not be able to use non-alphanumerical
specifiers without breaking backwards compatibility again. I think that's an
acceptable compromise.

v2:
- add NEWS entry

v3:
- only fail on alphanumerical

diff --git a/NEWS b/NEWS
index 77b38f5d6ca760a52089515f99ca1498e7fb267b..de3d28d66885d880222be25e863b63df41f69cbe 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,13 @@ CHANGES WITH 236 in spe:
           numdummies=0, preventing the kernel from automatically creating
           dummy0. All dummy interfaces must now be explicitly created.
 
+        * Unknown specifiers are now rejected. This applies to units and
+          tmpfiles.d configuration. Any percent characters that are followed by
+          a letter or digit that are not supposed to be interpreted as the
+          beginning of a specifier should be escaped by doubling ("%%").
+          (So "size=5%" is still accepted, as well as "size=5%,foo=bar", but
+          not "LABEL=x%y%z" since %y and %z are not valid specifiers today.)
+
         * systemd-resolved now maintains a new dynamic
           /run/systemd/resolve/stub-resolv.conf compatibility file. It is
           recommended to make /etc/resolv.conf a symlink to it. This file

diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml
index 9c40562e08b56af469d2854fb1d075a67a6fcc05..06a7bbd3bafbdfb04c3c760f7251686780529392 100644 (file)
--- a/man/systemd.unit.xml
+++ b/man/systemd.unit.xml
@@ -1297,7 +1297,8 @@
 
     <para>Many settings resolve specifiers which may be used to write
     generic unit files referring to runtime or unit parameters that
-    are replaced when the unit files are loaded. The following
+    are replaced when the unit files are loaded. Specifiers must be known
+    and resolvable for the setting to be valid. The following
     specifiers are understood:</para>
 
     <table>

diff --git a/man/tmpfiles.d.xml b/man/tmpfiles.d.xml
index d983984fd67437713a154faa0518483d9bac6a07..642a124eac13f06bf60fcf58bf965e7cd5df62b3 100644 (file)
--- a/man/tmpfiles.d.xml
+++ b/man/tmpfiles.d.xml
@@ -594,6 +594,7 @@ r! /tmp/.X[0-9]*-lock</programlisting>
     <title>Specifiers</title>
 
     <para>Specifiers can be used in the "path" and "argument" fields.
+    An unknown or unresolvable specifier is treated as invalid configuration.
     The following expansions are understood:</para>
       <table>
         <title>Specifiers available</title>

diff --git a/src/shared/specifier.c b/src/shared/specifier.c
index 9bef890346226f5f7f743bfb2b9fc53910feb3c5..9034cc20d728b77144b1c666b89fce434539df71 100644 (file)
--- a/src/shared/specifier.c
+++ b/src/shared/specifier.c
@@ -41,6 +41,10 @@
  *
  */
 
+/* Any ASCII character or digit: our pool of potential specifiers,
+ * and "%" used for escaping. */
+#define POSSIBLE_SPECIFIERS ALPHANUMERICAL "%"
+
 int specifier_printf(const char *text, const Specifier table[], void *userdata, char **_ret) {
         char *ret, *t;
         const char *f;
@@ -97,7 +101,10 @@ int specifier_printf(const char *text, const Specifier table[], void *userdata,
 
                                         ret = n;
                                         t = n + j + k;
-                                } else {
+                                } else if (strchr(POSSIBLE_SPECIFIERS, *f))
+                                        /* Oops, an unknown specifier. */
+                                        return -EBADSLT;
+                                else {
                                         *(t++) = '%';
                                         *(t++) = *f;
                                 }

diff --git a/src/test/test-systemd-tmpfiles.py b/src/test/test-systemd-tmpfiles.py
index 0f45bd4f761a97f5ef3931b08551f7af8968a3cd..75d8fd36577b94d096bce2b16800511bb33d0e7f 100755 (executable)
--- a/src/test/test-systemd-tmpfiles.py
+++ b/src/test/test-systemd-tmpfiles.py
@@ -32,10 +32,10 @@ if __name__ == '__main__':
     test_invalid_line('f++ /too/many/plusses')
     test_invalid_line('f+!+ /too/many/plusses')
     test_invalid_line('f!+! /too/many/bangs')
-    #test_invalid_line('w /unresolved/argument - - - - "%Y"')
-    #test_invalid_line('w /unresolved/argument/sandwich - - - - "%v%Y%v"')
-    #test_invalid_line('w /unresolved/filename/%Y - - - - "whatever"')
-    #test_invalid_line('w /unresolved/filename/sandwich/%v%Y%v - - - - "whatever"')
+    test_invalid_line('w /unresolved/argument - - - - "%Y"')
+    test_invalid_line('w /unresolved/argument/sandwich - - - - "%v%Y%v"')
+    test_invalid_line('w /unresolved/filename/%Y - - - - "whatever"')
+    test_invalid_line('w /unresolved/filename/sandwich/%v%Y%v - - - - "whatever"')
     test_invalid_line('w - - - - - "no file specfied"')
     test_invalid_line('C - - - - - "no file specfied"')
     test_invalid_line('C non/absolute/path - - - - -')

diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c
index e0d96d612d890e439af11d0a225e2f20c504cfb8..4e615288da4437848160905cc099b4c1e65b4b89 100644 (file)
--- a/src/tmpfiles/tmpfiles.c
+++ b/src/tmpfiles/tmpfiles.c
@@ -1867,7 +1867,8 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool
                         &age,
                         NULL);
         if (r < 0) {
-                if (r == -EINVAL) /* invalid quoting and such */
+                if (IN_SET(r, -EINVAL, -EBADSLT))
+                        /* invalid quoting and such or an unknown specifier */
                         *invalid_config = true;
                 return log_error_errno(r, "[%s:%u] Failed to parse line: %m", fname, line);
         }
@@ -1916,8 +1917,8 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool
         if (r == -ENOKEY)
                 return log_unresolvable_specifier(fname, line);
         if (r < 0) {
-                /* ENOMEM is the only return value left after ENOKEY,
-                 * so *invalid_config should not be set. */
+                if (IN_SET(r, -EINVAL, -EBADSLT))
+                        *invalid_config = true;
                 return log_error_errno(r, "[%s:%u] Failed to replace specifiers: %s", fname, line, path);
         }
 
@@ -2028,7 +2029,7 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool
                         return -EBADMSG;
                 }
                 r = parse_attribute_from_arg(&i);
-                if (r == -EINVAL)
+                if (IN_SET(r, -EINVAL, -EBADSLT))
                         *invalid_config = true;
                 if (r < 0)
                         return r;
@@ -2054,9 +2055,12 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool
         r = specifier_expansion_from_arg(&i);
         if (r == -ENOKEY)
                 return log_unresolvable_specifier(fname, line);
-        if (r < 0)
+        if (r < 0) {
+                if (IN_SET(r, -EINVAL, -EBADSLT))
+                        *invalid_config = true;
                 return log_error_errno(r, "[%s:%u] Failed to substitute specifiers in argument: %m",
                                        fname, line);
+        }
 
         if (arg_root) {
                 char *p;