path-util: rework find_binary(), fsck_exists() and mkfs_exists()

Modernize the code a bit:

- Get rid of FOREACH_WORD_SEPARATOR() loop in favour of a
  extract_first_word() loop.

- Remove find_binary()'s "local" flag. It's not reasonably possible to
  look for binaries on remote systems, we hence should not pretend we
  could.

- When we cannot find a suitable binary, return the last error returned
  from access() rather than ENOENT unconditionally.

- Rework fsck_exists() and mkfs_exists() to return 1 on success, 0 if
  the implementation is missing and negative on real errors. This is
  more like we do it in other functions.

- Make sure we also detect direct fsck symlinks to "true", rather than
  just absolute ones to /bin/true.

6 files changed, 99 insertions, 75 deletions

diff --git a/src/basic/path-util.c b/src/basic/path-util.c
index 1039623305..9aeb8c23fd 100644
--- a/src/basic/path-util.c
+++ b/src/basic/path-util.c
@@ -698,7 +698,6 @@ int path_is_os_tree(const char *path) {
         /* We use /usr/lib/os-release as flag file if something is an OS */
         p = strjoina(path, "/usr/lib/os-release");
         r = access(p, F_OK);
-
         if (r >= 0)
                 return 1;
 
@@ -709,56 +708,67 @@ int path_is_os_tree(const char *path) {
         return r >= 0;
 }
 
-int find_binary(const char *name, bool local, char **filename) {
+int find_binary(const char *name, char **ret) {
+        int last_error, r;
+        const char *p;
+
         assert(name);
 
         if (is_path(name)) {
-                if (local && access(name, X_OK) < 0)
+                if (access(name, X_OK) < 0)
                         return -errno;
 
-                if (filename) {
-                        char *p;
+                if (ret) {
+                        char *rs;
 
-                        p = path_make_absolute_cwd(name);
-                        if (!p)
+                        rs = path_make_absolute_cwd(name);
+                        if (!rs)
                                 return -ENOMEM;
 
-                        *filename = p;
+                        *ret = rs;
                 }
 
                 return 0;
-        } else {
-                const char *path;
-                const char *word, *state;
-                size_t l;
-
-                /**
-                 * Plain getenv, not secure_getenv, because we want
-                 * to actually allow the user to pick the binary.
-                 */
-                path = getenv("PATH");
-                if (!path)
-                        path = DEFAULT_PATH;
-
-                FOREACH_WORD_SEPARATOR(word, l, path, ":", state) {
-                        _cleanup_free_ char *p = NULL;
-
-                        if (asprintf(&p, "%.*s/%s", (int) l, word, name) < 0)
-                                return -ENOMEM;
+        }
 
-                        if (access(p, X_OK) < 0)
-                                continue;
+        /**
+         * Plain getenv, not secure_getenv, because we want
+         * to actually allow the user to pick the binary.
+         */
+        p = getenv("PATH");
+        if (!p)
+                p = DEFAULT_PATH;
+
+        last_error = -ENOENT;
+
+        for (;;) {
+                _cleanup_free_ char *j = NULL, *element = NULL;
+
+                r = extract_first_word(&p, &element, ":", EXTRACT_RELAX|EXTRACT_DONT_COALESCE_SEPARATORS);
+                if (r < 0)
+                        return r;
+                if (r == 0)
+                        break;
+
+                j = strjoin(element, "/", name, NULL);
+                if (!j)
+                        return -ENOMEM;
+
+                if (access(j, X_OK) >= 0) {
+                        /* Found it! */
 
-                        if (filename) {
-                                *filename = path_kill_slashes(p);
-                                p = NULL;
+                        if (ret) {
+                                *ret = path_kill_slashes(j);
+                                j = NULL;
                         }
 
                         return 0;
                 }
 
-                return -ENOENT;
+                last_error = -errno;
         }
+
+        return last_error;
 }
 
 bool paths_check_timestamp(const char* const* paths, usec_t *timestamp, bool update) {
@@ -800,7 +810,9 @@ static int binary_is_good(const char *binary) {
         _cleanup_free_ char *p = NULL, *d = NULL;
         int r;
 
-        r = find_binary(binary, true, &p);
+        r = find_binary(binary, &p);
+        if (r == -ENOENT)
+                return 0;
         if (r < 0)
                 return r;
 
@@ -808,28 +820,38 @@ static int binary_is_good(const char *binary) {
          * fsck */
 
         r = readlink_malloc(p, &d);
-        if (r >= 0 &&
-            (path_equal(d, "/bin/true") ||
-             path_equal(d, "/usr/bin/true") ||
-             path_equal(d, "/dev/null")))
-                return -ENOENT;
+        if (r == -EINVAL) /* not a symlink */
+                return 1;
+        if (r < 0)
+                return r;
 
-        return 0;
+        return !path_equal(d, "true") &&
+               !path_equal(d, "/bin/true") &&
+               !path_equal(d, "/usr/bin/true") &&
+               !path_equal(d, "/dev/null");
 }
 
 int fsck_exists(const char *fstype) {
         const char *checker;
 
-        checker = strjoina("fsck.", fstype);
+        assert(fstype);
 
+        if (streq(fstype, "auto"))
+                return -EINVAL;
+
+        checker = strjoina("fsck.", fstype);
         return binary_is_good(checker);
 }
 
 int mkfs_exists(const char *fstype) {
         const char *mkfs;
 
-        mkfs = strjoina("mkfs.", fstype);
+        assert(fstype);
 
+        if (streq(fstype, "auto"))
+                return -EINVAL;
+
+        mkfs = strjoina("mkfs.", fstype);
         return binary_is_good(mkfs);
 }
 
diff --git a/src/basic/path-util.h b/src/basic/path-util.h
index 71e25f1e57..03e1ac5d60 100644
--- a/src/basic/path-util.h
+++ b/src/basic/path-util.h
@@ -58,7 +58,7 @@ int path_is_mount_point(const char *path, int flags);
 int path_is_read_only_fs(const char *path);
 int path_is_os_tree(const char *path);
 
-int find_binary(const char *name, bool local, char **filename);
+int find_binary(const char *name, char **filename);
 
 bool paths_check_timestamp(const char* const* paths, usec_t *paths_ts_usec, bool update);
 
diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c
index 30c846f01d..72a6940849 100644
--- a/src/fsck/fsck.c
+++ b/src/fsck/fsck.c
@@ -366,12 +366,12 @@ int main(int argc, char *argv[]) {
         r = sd_device_get_property_value(dev, "ID_FS_TYPE", &type);
         if (r >= 0) {
                 r = fsck_exists(type);
-                if (r == -ENOENT) {
-                        log_info("fsck.%s doesn't exist, not checking file system on %s", type, device);
-                        r = 0;
+                if (r < 0)
+                        log_warning_errno(r, "Couldn't detect if fsck.%s may be used for %s, proceeding: %m", type, device);
+                else if (r == 0) {
+                        log_info("fsck.%s doesn't exist, not checking file system on %s.", type, device);
                         goto finish;
-                } else if (r < 0)
-                        log_warning_errno(r, "Couldn't detect if fsck.%s may be used for %s: %m", type, device);
+                }
         }
 
         if (arg_show_progress) {
diff --git a/src/run/run.c b/src/run/run.c
index 93d8cd1d08..25ef04a7d2 100644
--- a/src/run/run.c
+++ b/src/run/run.c
@@ -1153,14 +1153,20 @@ int main(int argc, char* argv[]) {
         if (r <= 0)
                 goto finish;
 
-        if (argc > optind) {
-                r = find_binary(argv[optind], arg_transport == BUS_TRANSPORT_LOCAL, &command);
+        if (argc > optind && arg_transport == BUS_TRANSPORT_LOCAL) {
+                /* Patch in an absolute path */
+
+                r = find_binary(argv[optind], &command);
                 if (r < 0) {
-                        log_error_errno(r, "Failed to find executable %s%s: %m",
-                                        argv[optind],
-                                        arg_transport == BUS_TRANSPORT_LOCAL ? "" : " on local system");
+                        log_error_errno(r, "Failed to find executable %s: %m", argv[optind]);
+                        goto finish;
+                }
+                if (r == 0) {
+                        log_error("Couldn't find executable %s.", argv[optind]);
+                        r = -ENOENT;
                         goto finish;
                 }
+
                 argv[optind] = command;
         }
 
diff --git a/src/shared/generator.c b/src/shared/generator.c
index e58bbea77c..d912bcd9e1 100644
--- a/src/shared/generator.c
+++ b/src/shared/generator.c
@@ -32,10 +32,9 @@
 #include "dropin.h"
 
 static int write_fsck_sysroot_service(const char *dir, const char *what) {
-        const char *unit;
-        _cleanup_free_ char *device = NULL;
-        _cleanup_free_ char *escaped;
+        _cleanup_free_ char *device = NULL, *escaped = NULL;
         _cleanup_fclose_ FILE *f = NULL;
+        const char *unit;
         int r;
 
         escaped = cescape(what);
@@ -101,16 +100,17 @@ int generator_write_fsck_deps(
 
         if (!isempty(fstype) && !streq(fstype, "auto")) {
                 r = fsck_exists(fstype);
-                if (r == -ENOENT) {
+                if (r < 0)
+                        log_warning_errno(r, "Checking was requested for %s, but couldn't detect if fsck.%s may be used, proceeding: %m", what, fstype);
+                else if (r == 0) {
                         /* treat missing check as essentially OK */
-                        log_debug_errno(r, "Checking was requested for %s, but fsck.%s does not exist: %m", what, fstype);
+                        log_debug("Checking was requested for %s, but fsck.%s does not exist.", what, fstype);
                         return 0;
-                } else if (r < 0)
-                        return log_warning_errno(r, "Checking was requested for %s, but fsck.%s cannot be used: %m", what, fstype);
+                }
         }
 
         if (path_equal(where, "/")) {
-                char *lnk;
+                const char *lnk;
 
                 lnk = strjoina(dir, "/" SPECIAL_LOCAL_FS_TARGET ".wants/systemd-fsck-root.service");
 
diff --git a/src/test/test-path-util.c b/src/test/test-path-util.c
index fce4e81a09..a930196a6c 100644
--- a/src/test/test-path-util.c
+++ b/src/test/test-path-util.c
@@ -104,32 +104,28 @@ static void test_path(void) {
         }
 }
 
-static void test_find_binary(const char *self, bool local) {
+static void test_find_binary(const char *self) {
         char *p;
 
-        assert_se(find_binary("/bin/sh", local, &p) == 0);
+        assert_se(find_binary("/bin/sh", &p) == 0);
         puts(p);
-        assert_se(streq(p, "/bin/sh"));
+        assert_se(path_equal(p, "/bin/sh"));
         free(p);
 
-        assert_se(find_binary(self, local, &p) == 0);
+        assert_se(find_binary(self, &p) == 0);
         puts(p);
         assert_se(endswith(p, "/test-path-util"));
         assert_se(path_is_absolute(p));
         free(p);
 
-        assert_se(find_binary("sh", local, &p) == 0);
+        assert_se(find_binary("sh", &p) == 0);
         puts(p);
         assert_se(endswith(p, "/sh"));
         assert_se(path_is_absolute(p));
         free(p);
 
-        assert_se(find_binary("xxxx-xxxx", local, &p) == -ENOENT);
-
-        assert_se(find_binary("/some/dir/xxxx-xxxx", local, &p) ==
-                  (local ? -ENOENT : 0));
-        if (!local)
-                free(p);
+        assert_se(find_binary("xxxx-xxxx", &p) == -ENOENT);
+        assert_se(find_binary("/some/dir/xxxx-xxxx", &p) == -ENOENT);
 }
 
 static void test_prefixes(void) {
@@ -210,9 +206,10 @@ static void test_fsck_exists(void) {
         unsetenv("PATH");
 
         /* fsck.minix is provided by util-linux and will probably exist. */
-        assert_se(fsck_exists("minix") == 0);
+        assert_se(fsck_exists("minix") == 1);
 
-        assert_se(fsck_exists("AbCdE") == -ENOENT);
+        assert_se(fsck_exists("AbCdE") == 0);
+        assert_se(fsck_exists("/../bin/") == 0);
 }
 
 static void test_make_relative(void) {
@@ -450,8 +447,7 @@ static void test_path_is_mount_point(void) {
 
 int main(int argc, char **argv) {
         test_path();
-        test_find_binary(argv[0], true);
-        test_find_binary(argv[0], false);
+        test_find_binary(argv[0]);
         test_prefixes();
         test_path_join();
         test_fsck_exists();