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.

7 files changed, 56 insertions, 56 deletions

diff --git a/Userland/Libraries/LibC/unistd.cpp b/Userland/Libraries/LibC/unistd.cpp
index 0e44c2df5a..cd2f91bc89 100644
--- a/Userland/Libraries/LibC/unistd.cpp
+++ b/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;
diff --git a/Userland/Libraries/LibCore/DirIterator.cpp b/Userland/Libraries/LibCore/DirIterator.cpp
index 527c36b579..36c9c8ea2c 100644
--- a/Userland/Libraries/LibCore/DirIterator.cpp
+++ b/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)
diff --git a/Userland/Libraries/LibCore/DirIterator.h b/Userland/Libraries/LibCore/DirIterator.h
index 3fdd3d613c..e3a4784771 100644
--- a/Userland/Libraries/LibCore/DirIterator.h
+++ b/Userland/Libraries/LibCore/DirIterator.h
@@ -44,6 +44,4 @@ private:
     bool advance_next();
 };
 
-String find_executable_in_path(String filename);
-
 }
diff --git a/Userland/Libraries/LibCore/File.cpp b/Userland/Libraries/LibCore/File.cpp
index 436ac612bd..c3a83a5c85 100644
--- a/Userland/Libraries/LibCore/File.cpp
+++ b/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 {};
+};
+
 }
diff --git a/Userland/Libraries/LibCore/File.h b/Userland/Libraries/LibCore/File.h
index c331deaf66..02eb5ce34f 100644
--- a/Userland/Libraries/LibCore/File.h
+++ b/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)
diff --git a/Userland/Libraries/LibCore/System.cpp b/Userland/Libraries/LibCore/System.cpp
index 8ffcd93870..5497a1bff1 100644
--- a/Userland/Libraries/LibCore/System.cpp
+++ b/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 = should_search_in_path ? TRY(find_file_in_path(filename)) : filename.to_string();
+    String exec_filename;
+
+    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())
-                path = DEFAULT_PATH;
-            auto parts = path.split(':');
-            for (auto& part : parts) {
-                auto candidate = String::formatted("{}/{}", part, filename);
-                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);
-                }
+
+            auto maybe_executable = Core::File::resolve_executable_from_environment(filename_string);
+
+            if (!maybe_executable.has_value()) {
+                errno_rollback.set_override_rollback_value(ENOENT);
+                return Error::from_errno(ENOENT);
             }
-            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
diff --git a/Userland/Libraries/LibCore/System.h b/Userland/Libraries/LibCore/System.h
index 7875d8737d..56628527c2 100644
--- a/Userland/Libraries/LibCore/System.h
+++ b/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,