Userland: Consolidate most PATH resolving into a single implementation

We previously had at least three different implementations for resolving
executables in the PATH, all of which had slightly different
characteristics.

Merge those into a single implementation to keep the behaviour
consistent, and maybe to make that implementation more configurable in
the future.

Userland/DevTools/UserspaceEmulator/main.cpp

@@ -49,7 +49,7 @@ int main(int argc, char** argv, char** env)
     if (arguments[0].contains("/"sv))
         executable_path = Core::File::real_path_for(arguments[0]);
     else
-        executable_path = Core::find_executable_in_path(arguments[0]);
+        executable_path = Core::File::resolve_executable_from_environment(arguments[0]).value_or({});
     if (executable_path.is_empty()) {
         reportln("Cannot find executable for '{}'."sv, arguments[0]);
         return 1;

Userland/Libraries/LibC/unistd.cpp

@@ -179,12 +179,15 @@ int execve(char const* filename, char* const argv[], char* const envp[])
     __RETURN_WITH_ERRNO(rc, rc, -1);
 }
 
+// https://linux.die.net/man/3/execvpe (GNU extension)
 int execvpe(char const* filename, char* const argv[], char* const envp[])
 {
     if (strchr(filename, '/'))
         return execve(filename, argv, envp);
 
     ScopedValueRollback errno_rollback(errno);
+
+    // TODO: Make this use the PATH search implementation from Core::File.
     String path = getenv("PATH");
     if (path.is_empty())
         path = DEFAULT_PATH;

Userland/Libraries/LibCore/DirIterator.cpp

@@ -95,28 +95,6 @@ String DirIterator::next_full_path()
     return builder.to_string();
 }
 
-String find_executable_in_path(String filename)
-{
-    if (filename.is_empty())
-        return {};
-
-    if (filename.starts_with('/')) {
-        if (access(filename.characters(), X_OK) == 0)
-            return filename;
-
-        return {};
-    }
-
-    for (auto directory : String { getenv("PATH") }.split(':')) {
-        auto fullpath = String::formatted("{}/{}", directory, filename);
-
-        if (access(fullpath.characters(), X_OK) == 0)
-            return fullpath;
-    }
-
-    return {};
-}
-
 int DirIterator::fd() const
 {
     if (!m_dir)

Userland/Libraries/LibCore/DirIterator.h

@@ -44,6 +44,4 @@ private:
     bool advance_next();
 };
 
-String find_executable_in_path(String filename);
-
 }

Userland/Libraries/LibCore/File.cpp

@@ -557,4 +557,35 @@ ErrorOr<void, File::RemoveError> File::remove(String const& path, RecursionMode
     return {};
 }
 
+Optional<String> File::resolve_executable_from_environment(StringView filename)
+{
+    if (filename.is_empty())
+        return {};
+
+    // Paths that aren't just a file name generally count as already resolved.
+    if (filename.contains('/')) {
+        if (access(String { filename }.characters(), X_OK) != 0)
+            return {};
+
+        return filename;
+    }
+
+    auto const* path_str = getenv("PATH");
+    StringView path { path_str, strlen(path_str) };
+
+    if (path.is_empty())
+        path = DEFAULT_PATH_SV;
+
+    auto directories = path.split_view(':');
+
+    for (auto directory : directories) {
+        auto file = String::formatted("{}/{}", directory, filename);
+
+        if (access(file.characters(), X_OK) == 0)
+            return file;
+    }
+
+    return {};
+};
+
 }

Userland/Libraries/LibCore/File.h

@@ -110,6 +110,8 @@ public:
     static NonnullRefPtr<File> standard_output();
     static NonnullRefPtr<File> standard_error();
 
+    static Optional<String> resolve_executable_from_environment(StringView filename);
+
 private:
     File(Object* parent = nullptr)
         : IODevice(parent)

Userland/Libraries/LibCore/System.cpp

@@ -953,22 +953,6 @@ ErrorOr<void> adjtime(const struct timeval* delta, struct timeval* old_delta)
 }
 #endif
 
-ErrorOr<String> find_file_in_path(StringView filename)
-{
-    auto const* path_ptr = getenv("PATH");
-    StringView path { path_ptr, strlen(path_ptr) };
-    if (path.is_empty())
-        path = DEFAULT_PATH_SV;
-    auto parts = path.split_view(':');
-    for (auto& part : parts) {
-        auto candidate = String::formatted("{}/{}", part, filename);
-        if (Core::File::exists(candidate)) {
-            return candidate;
-        }
-    }
-    return Error::from_errno(ENOENT);
-}
-
 ErrorOr<void> exec(StringView filename, Span<StringView> arguments, SearchInPath search_in_path, Optional<Span<StringView>> environment)
 {
 #ifdef __serenity__
@@ -1009,8 +993,18 @@ ErrorOr<void> exec(StringView filename, Span<StringView> arguments, SearchInPath
         return {};
     };
 
-    bool should_search_in_path = search_in_path == SearchInPath::Yes && !filename.contains('/');
+    String exec_filename;
-    String exec_filename = should_search_in_path ? TRY(find_file_in_path(filename)) : filename.to_string();
+
+    if (search_in_path == SearchInPath::Yes) {
+        auto maybe_executable = Core::File::resolve_executable_from_environment(filename);
+
+        if (!maybe_executable.has_value())
+            return ENOENT;
+
+        exec_filename = maybe_executable.release_value();
+    } else {
+        exec_filename = filename.to_string();
+    }
 
     params.path = { exec_filename.characters(), exec_filename.length() };
     TRY(run_exec(params));
@@ -1039,21 +1033,16 @@ ErrorOr<void> exec(StringView filename, Span<StringView> arguments, SearchInPath
         if (search_in_path == SearchInPath::Yes && !filename.contains('/')) {
 #    if defined(__APPLE__) || defined(__FreeBSD__)
             // These BSDs don't support execvpe(), so we'll have to manually search the PATH.
-            // This is copy-pasted from LibC's execvpe() with minor changes.
             ScopedValueRollback errno_rollback(errno);
-            String path = getenv("PATH");
+
-            if (path.is_empty())
+            auto maybe_executable = Core::File::resolve_executable_from_environment(filename_string);
-                path = DEFAULT_PATH;
+
-            auto parts = path.split(':');
+            if (!maybe_executable.has_value()) {
-            for (auto& part : parts) {
+                errno_rollback.set_override_rollback_value(ENOENT);
-                auto candidate = String::formatted("{}/{}", part, filename);
+                return Error::from_errno(ENOENT);
-                rc = ::execve(candidate.characters(), argv.data(), envp.data());
-                if (rc < 0 && errno != ENOENT) {
-                    errno_rollback.set_override_rollback_value(errno);
-                    return Error::from_syscall("exec"sv, rc);
-                }
             }
-            errno_rollback.set_override_rollback_value(ENOENT);
+
+            rc = ::execve(maybe_executable.release_value().characters(), argv.data(), envp.data());
 #    else
             rc = ::execvpe(filename_string.characters(), argv.data(), envp.data());
 #    endif

Userland/Libraries/LibCore/System.h

@@ -161,7 +161,6 @@ ErrorOr<Array<int, 2>> pipe2(int flags);
 #ifndef AK_OS_ANDROID
 ErrorOr<void> adjtime(const struct timeval* delta, struct timeval* old_delta);
 #endif
-ErrorOr<String> find_file_in_path(StringView filename);
 enum class SearchInPath {
     No,
     Yes,

Userland/Shell/Builtin.cpp

@@ -232,9 +232,9 @@ int Shell::builtin_type(int argc, char const** argv)
         }
 
         // check if its an executable in PATH
-        auto fullpath = Core::find_executable_in_path(command);
+        auto fullpath = Core::File::resolve_executable_from_environment(command);
-        if (!fullpath.is_empty()) {
+        if (fullpath.has_value()) {
-            printf("%s is %s\n", command.characters(), escape_token(fullpath).characters());
+            printf("%s is %s\n", command.characters(), escape_token(fullpath.release_value()).characters());
             continue;
         }
         something_not_found = true;
@@ -1075,12 +1075,12 @@ int Shell::builtin_kill(int argc, char const** argv)
 {
     // Simply translate the arguments and pass them to `kill'
     Vector<String> replaced_values;
-    auto kill_path = find_in_path("kill"sv);
+    auto kill_path = Core::File::resolve_executable_from_environment("kill"sv);
-    if (kill_path.is_empty()) {
+    if (!kill_path.has_value()) {
         warnln("kill: `kill' not found in PATH");
         return 126;
     }
-    replaced_values.append(kill_path);
+    replaced_values.append(kill_path.release_value());
     for (auto i = 1; i < argc; ++i) {
         if (auto job_id = resolve_job_spec({ argv[i], strlen(argv[1]) }); job_id.has_value()) {
             auto job = find_job(job_id.value());

Userland/Shell/Shell.cpp

@@ -1338,27 +1338,6 @@ String Shell::unescape_token(StringView token)
     return builder.build();
 }
 
-String Shell::find_in_path(StringView program_name)
-{
-    String path = getenv("PATH");
-    if (!path.is_empty()) {
-        auto directories = path.split(':');
-        for (auto const& directory : directories) {
-            Core::DirIterator programs(directory.characters(), Core::DirIterator::SkipDots);
-            while (programs.has_next()) {
-                auto program = programs.next_path();
-                auto program_path = String::formatted("{}/{}", directory, program);
-                if (access(program_path.characters(), X_OK) != 0)
-                    continue;
-                if (program == program_name)
-                    return program_path;
-            }
-        }
-    }
-
-    return {};
-}
-
 void Shell::cache_path()
 {
     if (!m_is_interactive)
@@ -1387,6 +1366,7 @@ void Shell::cache_path()
         cached_path.append({ RunnablePath::Kind::Alias, name });
     }
 
+    // TODO: Can we make this rely on Core::File::resolve_executable_from_environment()?
     String path = getenv("PATH");
     if (!path.is_empty()) {
         auto directories = path.split(':');

Userland/Shell/Shell.h

@@ -157,8 +157,6 @@ public:
     String resolve_path(String) const;
     String resolve_alias(StringView) const;
 
-    static String find_in_path(StringView program_name);
-
     static bool has_history_event(StringView);
 
     RefPtr<AST::Value> get_argument(size_t) const;

Userland/Utilities/pledge.cpp

@@ -5,6 +5,7 @@
  */
 
 #include <LibCore/ArgsParser.h>
+#include <LibCore/File.h>
 #include <LibCore/MappedFile.h>
 #include <LibCore/System.h>
 #include <LibELF/Image.h>
@@ -12,12 +13,12 @@
 
 static ErrorOr<bool> is_dynamically_linked_executable(StringView filename)
 {
-    String exec_filename = filename;
+    auto maybe_executable = Core::File::resolve_executable_from_environment(filename);
-    if (!filename.contains('/')) {
-        exec_filename = TRY(Core::System::find_file_in_path(filename));
-    }
 
-    auto file = TRY(Core::MappedFile::map(exec_filename));
+    if (!maybe_executable.has_value())
+        return ENOENT;
+
+    auto file = TRY(Core::MappedFile::map(maybe_executable.release_value()));
     ELF::Image elf_image(file->bytes());
     return elf_image.is_dynamic();
 }

Userland/Utilities/which.cpp

@@ -5,7 +5,7 @@
  */
 
 #include <LibCore/ArgsParser.h>
-#include <LibCore/DirIterator.h>
+#include <LibCore/File.h>
 #include <LibCore/System.h>
 #include <stdio.h>
 #include <unistd.h>
@@ -20,12 +20,12 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
     args_parser.add_positional_argument(filename, "Name of executable", "executable");
     args_parser.parse(arguments);
 
-    auto fullpath = Core::find_executable_in_path(filename);
+    auto fullpath = Core::File::resolve_executable_from_environment({ filename, strlen(filename) });
-    if (fullpath.is_empty()) {
+    if (!fullpath.has_value()) {
         warnln("no '{}' in path", filename);
         return 1;
     }
 
-    outln("{}", fullpath);
+    outln("{}", fullpath.release_value());
     return 0;
 }