core/load-fragment: refuse units with errors in certain directives

If an error is encountered in any of the Exec* lines, WorkingDirectory,
SELinuxContext, ApparmorProfile, SmackProcessLabel, Service (in .socket
units), User, or Group, refuse to load the unit. If the config stanza
has support, ignore the failure if '-' is present.

For those configuration directives, even if we started the unit, it's
pretty likely that it'll do something unexpected (like write files
in a wrong place, or with a wrong context, or run with wrong permissions,
etc). It seems better to refuse to start the unit and have the admin
clean up the configuration without giving the service a chance to mess
up stuff.

Note that all "security" options that restrict what the unit can do
(Capabilities, AmbientCapabilities, Restrict*, SystemCallFilter, Limit*,
PrivateDevices, Protect*, etc) are _not_ treated like this. Such options are
only supplementary, and are not always available depending on the architecture
and compilation options, so unit authors have to make sure that the service
runs correctly without them anyway.

Fixes #6237, #6277.

diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c
index 6a6dadda2bc344ae6b133738224d36b1623a85e7..49a5c53f96f74d40961981c10267c6c7610e31dd 100644 (file)
--- a/src/core/load-fragment.c
+++ b/src/core/load-fragment.c
@@ -636,26 +636,36 @@ int config_parse_exec(
 
                 r = unit_full_printf(u, f, &path);
                 if (r < 0) {
-                        log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers on %s, ignoring: %m", f);
-                        return 0;
+                        log_syntax(unit, LOG_ERR, filename, line, r,
+                                   "Failed to resolve unit specifiers on %s%s: %m",
+                                   f, ignore ? ", ignoring" : "");
+                        return ignore ? 0 : -ENOEXEC;
                 }
 
                 if (isempty(path)) {
                         /* First word is either "-" or "@" with no command. */
-                        log_syntax(unit, LOG_ERR, filename, line, 0, "Empty path in command line, ignoring: \"%s\"", rvalue);
-                        return 0;
+                        log_syntax(unit, LOG_ERR, filename, line, 0,
+                                   "Empty path in command line%s: \"%s\"",
+                                   ignore ? ", ignoring" : "", rvalue);
+                        return ignore ? 0 : -ENOEXEC;
                 }
                 if (!string_is_safe(path)) {
-                        log_syntax(unit, LOG_ERR, filename, line, 0, "Executable path contains special characters, ignoring: %s", rvalue);
-                        return 0;
+                        log_syntax(unit, LOG_ERR, filename, line, 0,
+                                   "Executable path contains special characters%s: %s",
+                                   ignore ? ", ignoring" : "", rvalue);
+                        return ignore ? 0 : -ENOEXEC;
                 }
                 if (!path_is_absolute(path)) {
-                        log_syntax(unit, LOG_ERR, filename, line, 0, "Executable path is not absolute, ignoring: %s", rvalue);
-                        return 0;
+                        log_syntax(unit, LOG_ERR, filename, line, 0,
+                                   "Executable path is not absolute%s: %s",
+                                   ignore ? ", ignoring" : "", rvalue);
+                        return ignore ? 0 : -ENOEXEC;
                 }
                 if (endswith(path, "/")) {
-                        log_syntax(unit, LOG_ERR, filename, line, 0, "Executable path specifies a directory, ignoring: %s", rvalue);
-                        return 0;
+                        log_syntax(unit, LOG_ERR, filename, line, 0,
+                                   "Executable path specifies a directory%s: %s",
+                                   ignore ? ", ignoring" : "", rvalue);
+                        return ignore ? 0 : -ENOEXEC;
                 }
 
                 if (!separate_argv0) {
@@ -708,12 +718,14 @@ int config_parse_exec(
                         if (r == 0)
                                 break;
                         if (r < 0)
-                                return 0;
+                                return ignore ? 0 : -ENOEXEC;
 
                         r = unit_full_printf(u, word, &resolved);
                         if (r < 0) {
-                                log_syntax(unit, LOG_ERR, filename, line, 0, "Failed to resolve unit specifiers on %s, ignoring: %m", word);
-                                return 0;
+                                log_syntax(unit, LOG_ERR, filename, line, r,
+                                           "Failed to resolve unit specifiers on %s%s: %m",
+                                           word, ignore ? ", ignoring" : "");
+                                return ignore ? 0 : -ENOEXEC;
                         }
 
                         if (!GREEDY_REALLOC(n, nbufsize, nlen + 2))
@@ -724,8 +736,10 @@ int config_parse_exec(
                 }
 
                 if (!n || !n[0]) {
-                        log_syntax(unit, LOG_ERR, filename, line, 0, "Empty executable name or zeroeth argument, ignoring: %s", rvalue);
-                        return 0;
+                        log_syntax(unit, LOG_ERR, filename, line, 0,
+                                   "Empty executable name or zeroeth argument%s: %s",
+                                   ignore ? ", ignoring" : "", rvalue);
+                        return ignore ? 0 : -ENOEXEC;
                 }
 
                 nce = new0(ExecCommand, 1);
@@ -1332,8 +1346,10 @@ int config_parse_exec_selinux_context(
 
         r = unit_full_printf(u, rvalue, &k);
         if (r < 0) {
-                log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers, ignoring: %m");
-                return 0;
+                log_syntax(unit, LOG_ERR, filename, line, r,
+                           "Failed to resolve specifiers%s: %m",
+                           ignore ? ", ignoring" : "");
+                return ignore ? 0 : -ENOEXEC;
         }
 
         free(c->selinux_context);
@@ -1380,8 +1396,10 @@ int config_parse_exec_apparmor_profile(
 
         r = unit_full_printf(u, rvalue, &k);
         if (r < 0) {
-                log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers, ignoring: %m");
-                return 0;
+                log_syntax(unit, LOG_ERR, filename, line, r,
+                           "Failed to resolve specifiers%s: %m",
+                           ignore ? ", ignoring" : "");
+                return ignore ? 0 : -ENOEXEC;
         }
 
         free(c->apparmor_profile);
@@ -1428,8 +1446,10 @@ int config_parse_exec_smack_process_label(
 
         r = unit_full_printf(u, rvalue, &k);
         if (r < 0) {
-                log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers, ignoring: %m");
-                return 0;
+                log_syntax(unit, LOG_ERR, filename, line, r,
+                           "Failed to resolve specifiers%s: %m",
+                           ignore ? ", ignoring" : "");
+                return ignore ? 0 : -ENOEXEC;
         }
 
         free(c->smack_process_label);
@@ -1647,19 +1667,19 @@ int config_parse_socket_service(
 
         r = unit_name_printf(UNIT(s), rvalue, &p);
         if (r < 0) {
-                log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers, ignoring: %s", rvalue);
-                return 0;
+                log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers: %s", rvalue);
+                return -ENOEXEC;
         }
 
         if (!endswith(p, ".service")) {
-                log_syntax(unit, LOG_ERR, filename, line, 0, "Unit must be of type service, ignoring: %s", rvalue);
-                return 0;
+                log_syntax(unit, LOG_ERR, filename, line, 0, "Unit must be of type service: %s", rvalue);
+                return -ENOEXEC;
         }
 
         r = manager_load_unit(UNIT(s)->manager, p, NULL, &error, &x);
         if (r < 0) {
-                log_syntax(unit, LOG_ERR, filename, line, r, "Failed to load unit %s, ignoring: %s", rvalue, bus_error_message(&error, r));
-                return 0;
+                log_syntax(unit, LOG_ERR, filename, line, r, "Failed to load unit %s: %s", rvalue, bus_error_message(&error, r));
+                return -ENOEXEC;
         }
 
         unit_ref_set(&s->service, x);
@@ -1907,13 +1927,13 @@ int config_parse_user_group(
 
                 r = unit_full_printf(u, rvalue, &k);
                 if (r < 0) {
-                        log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers in %s, ignoring: %m", rvalue);
-                        return 0;
+                        log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers in %s: %m", rvalue);
+                        return -ENOEXEC;
                 }
 
                 if (!valid_user_group_name_or_id(k)) {
-                        log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid user/group name or numeric ID, ignoring: %s", k);
-                        return 0;
+                        log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid user/group name or numeric ID: %s", k);
+                        return -ENOEXEC;
                 }
 
                 n = k;
@@ -1971,19 +1991,19 @@ int config_parse_user_group_strv(
                 if (r == -ENOMEM)
                         return log_oom();
                 if (r < 0) {
-                        log_syntax(unit, LOG_ERR, filename, line, r, "Invalid syntax, ignoring: %s", rvalue);
-                        break;
+                        log_syntax(unit, LOG_ERR, filename, line, r, "Invalid syntax: %s", rvalue);
+                        return -ENOEXEC;
                 }
 
                 r = unit_full_printf(u, word, &k);
                 if (r < 0) {
-                        log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers in %s, ignoring: %m", word);
-                        continue;
+                        log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers in %s: %m", word);
+                        return -ENOEXEC;
                 }
 
                 if (!valid_user_group_name_or_id(k)) {
-                        log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid user/group name or numeric ID, ignoring: %s", k);
-                        continue;
+                        log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid user/group name or numeric ID: %s", k);
+                        return -ENOEXEC;
                 }
 
                 r = strv_push(users, k);
@@ -2142,25 +2162,28 @@ int config_parse_working_directory(
 
                 r = unit_full_printf(u, rvalue, &k);
                 if (r < 0) {
-                        log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers in working directory path '%s', ignoring: %m", rvalue);
-                        return 0;
+                        log_syntax(unit, LOG_ERR, filename, line, r,
+                                   "Failed to resolve unit specifiers in working directory path '%s'%s: %m",
+                                   rvalue, missing_ok ? ", ignoring" : "");
+                        return missing_ok ? 0 : -ENOEXEC;
                 }
 
                 path_kill_slashes(k);
 
                 if (!utf8_is_valid(k)) {
                         log_syntax_invalid_utf8(unit, LOG_ERR, filename, line, rvalue);
-                        return 0;
+                        return missing_ok ? 0 : -ENOEXEC;
                 }
 
                 if (!path_is_absolute(k)) {
-                        log_syntax(unit, LOG_ERR, filename, line, 0, "Working directory path '%s' is not absolute, ignoring.", rvalue);
-                        return 0;
+                        log_syntax(unit, LOG_ERR, filename, line, 0,
+                                   "Working directory path '%s' is not absolute%s.",
+                                   rvalue, missing_ok ? ", ignoring" : "");
+                        return missing_ok ? 0 : -ENOEXEC;
                 }
 
-                free_and_replace(c->working_directory, k);
-
                 c->working_directory_home = false;
+                free_and_replace(c->working_directory, k);
         }
 
         c->working_directory_missing_ok = missing_ok;
@@ -4456,8 +4479,11 @@ int unit_load_fragment(Unit *u) {
                         return r;
 
                 r = load_from_path(u, k);
-                if (r < 0)
+                if (r < 0) {
+                        if (r == -ENOEXEC)
+                                log_unit_notice(u, "Unit configuration has fatal error, unit will not be started.");
                         return r;
+                }
 
                 if (u->load_state == UNIT_STUB) {
                         SET_FOREACH(t, u->names, i) {

diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c
index 12f48bf435580fc7cde9869b31b67c433c61832b..fd797b587ec2cca38ba436b3a9bea1c7dc944d51 100644 (file)
--- a/src/test/test-unit-file.c
+++ b/src/test/test-unit-file.c
@@ -146,7 +146,7 @@ static void test_config_parse_exec(void) {
         r = config_parse_exec(NULL, "fake", 4, "section", 1,
                               "LValue", 0, "/RValue/ argv0 r1",
                               &c, u);
-        assert_se(r == 0);
+        assert_se(r == -ENOEXEC);
         assert_se(c1->command_next == NULL);
 
         log_info("/* honour_argv0 */");
@@ -161,7 +161,7 @@ static void test_config_parse_exec(void) {
         r = config_parse_exec(NULL, "fake", 3, "section", 1,
                               "LValue", 0, "@/RValue",
                               &c, u);
-        assert_se(r == 0);
+        assert_se(r == -ENOEXEC);
         assert_se(c1->command_next == NULL);
 
         log_info("/* no command, whitespace only, reset */");
@@ -220,7 +220,7 @@ static void test_config_parse_exec(void) {
                               "-@/RValue argv0 r1 ; ; "
                               "/goo/goo boo",
                               &c, u);
-        assert_se(r >= 0);
+        assert_se(r == -ENOEXEC);
         c1 = c1->command_next;
         check_execcommand(c1, "/RValue", "argv0", "r1", NULL, true);
 
@@ -374,7 +374,7 @@ static void test_config_parse_exec(void) {
                 r = config_parse_exec(NULL, "fake", 4, "section", 1,
                                       "LValue", 0, path,
                                       &c, u);
-                assert_se(r == 0);
+                assert_se(r == -ENOEXEC);
                 assert_se(c1->command_next == NULL);
         }
 
@@ -401,21 +401,21 @@ static void test_config_parse_exec(void) {
         r = config_parse_exec(NULL, "fake", 4, "section", 1,
                               "LValue", 0, "/path\\",
                               &c, u);
-        assert_se(r == 0);
+        assert_se(r == -ENOEXEC);
         assert_se(c1->command_next == NULL);
 
         log_info("/* missing ending ' */");
         r = config_parse_exec(NULL, "fake", 4, "section", 1,
                               "LValue", 0, "/path 'foo",
                               &c, u);
-        assert_se(r == 0);
+        assert_se(r == -ENOEXEC);
         assert_se(c1->command_next == NULL);
 
         log_info("/* missing ending ' with trailing backslash */");
         r = config_parse_exec(NULL, "fake", 4, "section", 1,
                               "LValue", 0, "/path 'foo\\",
                               &c, u);
-        assert_se(r == 0);
+        assert_se(r == -ENOEXEC);
         assert_se(c1->command_next == NULL);
 
         log_info("/* invalid space between modifiers */");