systemctl: require correctly formed unit names when enabling units

Makefile.am

@@ -700,7 +700,8 @@ systemctl_SOURCES = \
 	src/sd-daemon.c \
 	src/cgroup-show.c \
 	src/cgroup-util.c \
-	src/exit-status.c
+	src/exit-status.c \
+	src/unit-name.c
 
 systemctl_CFLAGS = \
 	$(AM_CFLAGS) \

fixme

@@ -4,9 +4,6 @@ v11:
 
 * verify ordering of random-seed-load and base.target!
 
-* systemctl enable /lib/systemd/system/?*.service
-  creates wrong subdirs in /etc/ssytemd/system/ which prevent further bootup
-
 later:
 
 * do not throw error when .service file is linked to /dev/null
@@ -97,8 +94,6 @@ later:
 
 * document locale.conf, vconsole.conf and possibly the tempfiles.d and modules-load.d mechanism.
 
-* when /proc/self/mountinfo is not parsable, proceed with next line
-
 * beefed up tmpwatch that reads tmpfiles.d
 
 * /lib/systemd/system/systemd-readahead-replay.service
@@ -109,6 +104,10 @@ later:
 
 * Restart=on-failure and Restart=on-abort
 
+* kill sessions on shutdown
+
+* when processes remain in a service even though the start command failed enter active
+
 External:
 
 * place /etc/inittab with explaining blurb.

src/systemctl.c

@@ -53,6 +53,7 @@
 #include "exit-status.h"
 #include "bus-errors.h"
 #include "build.h"
+#include "unit-name.h"
 
 static const char *arg_type = NULL;
 static char **arg_property = NULL;
@@ -3237,28 +3238,16 @@ static void install_info_hashmap_free(Hashmap *m) {
         hashmap_free(m);
 }
 
-static bool unit_name_valid(const char *name) {
-
-        /* This is a minimal version of unit_name_valid() from
-         * unit-name.c */
-
-        if (!*name)
-                return false;
-
-        if (ignore_file(name))
-                return false;
-
-        return true;
-}
-
 static int install_info_add(const char *name) {
         InstallInfo *i;
         int r;
 
         assert(will_install);
 
-        if (!unit_name_valid(name))
+        if (!unit_name_is_valid_no_type(name)) {
+                log_warning("Unit name %s is not a valid unit name.", name);
                 return -EINVAL;
+        }
 
         if (hashmap_get(have_installed, name) ||
             hashmap_get(will_install, name))
@@ -3310,11 +3299,13 @@ static int config_parse_also(
                 if (!(n = strndup(w, l)))
                         return -ENOMEM;
 
-                r = install_info_add(n);
+                if ((r = install_info_add(n)) < 0) {
-                free(n);
+                        log_warning("Cannot install unit %s: %s", n, strerror(-r));
-
+                        free(n);
-                if (r < 0)
                         return r;
+                }
+
+                free(n);
         }
 
         return 0;
@@ -3639,7 +3630,7 @@ static int install_info_symlink_alias(const char *verb, InstallInfo *i, const ch
 
         STRV_FOREACH(s, i->aliases) {
 
-                if (!unit_name_valid(*s)) {
+                if (!unit_name_is_valid_no_type(*s)) {
                         log_error("Invalid name %s.", *s);
                         r = -EINVAL;
                         goto finish;
@@ -3658,7 +3649,6 @@ static int install_info_symlink_alias(const char *verb, InstallInfo *i, const ch
                 if (streq(verb, "disable"))
                         rmdir_parents(alias_path, config_path);
         }
-
         r = 0;
 
 finish:
@@ -3677,7 +3667,7 @@ static int install_info_symlink_wants(const char *verb, InstallInfo *i, const ch
         assert(config_path);
 
         STRV_FOREACH(s, i->wanted_by) {
-                if (!unit_name_valid(*s)) {
+                if (!unit_name_is_valid_no_type(*s)) {
                         log_error("Invalid name %s.", *s);
                         r = -EINVAL;
                         goto finish;
@@ -3840,8 +3830,10 @@ static int enable_unit(DBusConnection *bus, char **args, unsigned n) {
                 }
 
         for (j = 1; j < n; j++)
-                if ((r = install_info_add(args[j])) < 0)
+                if ((r = install_info_add(args[j])) < 0) {
+                        log_warning("Cannot install unit %s: %s", args[j], strerror(-r));
                         goto finish;
+                }
 
         while ((i = hashmap_first(will_install))) {
                 int q;

src/unit-name.c

@@ -21,8 +21,9 @@
 
 #include <errno.h>
 #include <string.h>
+#include <assert.h>
 
-#include "unit.h"
+#include "util.h"
 #include "unit-name.h"
 
 #define VALID_CHARS                             \
@@ -31,20 +32,7 @@
         "ABCDEFGHIJKLMNOPQRSTUVWXYZ"            \
         ":-_.\\"
 
-UnitType unit_name_to_type(const char *n) {
+bool unit_name_is_valid_no_type(const char *n) {
-        UnitType t;
-
-        assert(n);
-
-        for (t = 0; t < _UNIT_TYPE_MAX; t++)
-                if (endswith(n, unit_vtable[t]->suffix))
-                        return t;
-
-        return _UNIT_TYPE_INVALID;
-}
-
-bool unit_name_is_valid(const char *n) {
-        UnitType t;
         const char *e, *i, *at;
 
         /* Valid formats:
@@ -58,13 +46,8 @@ bool unit_name_is_valid(const char *n) {
         if (strlen(n) >= UNIT_NAME_MAX)
                 return false;
 
-        t = unit_name_to_type(n);
+        e = strrchr(n, '.');
-        if (t < 0 || t >= _UNIT_TYPE_MAX)
+        if (!e || e == n)
-                return false;
-
-        assert_se(e = strrchr(n, '.'));
-
-        if (e == n)
                 return false;
 
         for (i = n, at = NULL; i < e; i++) {
@@ -167,7 +150,7 @@ char *unit_name_change_suffix(const char *n, const char *suffix) {
         size_t a, b;
 
         assert(n);
-        assert(unit_name_is_valid(n));
+        assert(unit_name_is_valid_no_type(n));
         assert(suffix);
 
         assert_se(e = strrchr(n, '.'));
@@ -190,7 +173,6 @@ char *unit_name_build(const char *prefix, const char *instance, const char *suff
         assert(unit_prefix_is_valid(prefix));
         assert(!instance || unit_instance_is_valid(instance));
         assert(suffix);
-        assert(unit_name_to_type(suffix) >= 0);
 
         if (!instance)
                 return strappend(prefix, suffix);
@@ -226,7 +208,6 @@ char *unit_name_build_escape(const char *prefix, const char *instance, const cha
 
         assert(prefix);
         assert(suffix);
-        assert(unit_name_to_type(suffix) >= 0);
 
         /* Takes a arbitrary string for prefix and instance plus a
          * suffix and makes a nice string suitable as unit name of it,

src/unit-name.h

@@ -22,15 +22,15 @@
   along with systemd; If not, see <http://www.gnu.org/licenses/>.
 ***/
 
-#include "unit.h"
+#include <stdbool.h>
 
-UnitType unit_name_to_type(const char *n);
+#define UNIT_NAME_MAX 256
 
 int unit_name_to_instance(const char *n, char **instance);
 char* unit_name_to_prefix(const char *n);
 char* unit_name_to_prefix_and_instance(const char *n);
 
-bool unit_name_is_valid(const char *n);
+bool unit_name_is_valid_no_type(const char *n);
 bool unit_prefix_is_valid(const char *p);
 bool unit_instance_is_valid(const char *i);
 

src/unit.c

@@ -2207,6 +2207,28 @@ bool unit_pending_active(Unit *u) {
         return false;
 }
 
+UnitType unit_name_to_type(const char *n) {
+        UnitType t;
+
+        assert(n);
+
+        for (t = 0; t < _UNIT_TYPE_MAX; t++)
+                if (endswith(n, unit_vtable[t]->suffix))
+                        return t;
+
+        return _UNIT_TYPE_INVALID;
+}
+
+bool unit_name_is_valid(const char *n) {
+        UnitType t;
+
+        t = unit_name_to_type(n);
+        if (t < 0 || t >= _UNIT_TYPE_MAX)
+                return false;
+
+        return unit_name_is_valid_no_type(n);
+}
+
 static const char* const unit_load_state_table[_UNIT_LOAD_STATE_MAX] = {
         [UNIT_STUB] = "stub",
         [UNIT_LOADED] = "loaded",

src/unit.h

@@ -39,7 +39,6 @@ typedef enum UnitDependency UnitDependency;
 #include "socket-util.h"
 #include "execute.h"
 
-#define UNIT_NAME_MAX 256
 #define DEFAULT_TIMEOUT_USEC (60*USEC_PER_SEC)
 #define DEFAULT_RESTART_USEC (100*USEC_PER_MSEC)
 
@@ -504,6 +503,9 @@ bool unit_pending_active(Unit *u);
 
 int unit_add_default_target_dependency(Unit *u, Unit *target);
 
+UnitType unit_name_to_type(const char *n);
+bool unit_name_is_valid(const char *n);
+
 const char *unit_load_state_to_string(UnitLoadState i);
 UnitLoadState unit_load_state_from_string(const char *s);