LibCore+Userland+Ladybird: Explicitly manage Core::Process's end-state

This removes the implicit `disown`ing of the Process in the destructor
in favor of up front requests in the constructor. We also force users
to exclusively and explicitly disown, wait or take the pid of the child.
I hope that it will a good way to prevent API misuses, like calling C
functions with the pid or `wait_for_termination` on a disowned child.

It unfortunately prevents the usage of the "old" API with
KeepAsChild::Yes, this is why there are some refactorings that come
with the patch.

This also should make it possible to implement KeepAsChild::No on other
platforms.

Author: LucasChollet

Ladybird/WebDriver/main.cpp

@@ -18,23 +18,22 @@
 
 static Vector<ByteString> certificates;
 
-static ErrorOr<pid_t> launch_process(StringView application, ReadonlySpan<char const*> arguments)
+static ErrorOr<pid_t> launch_process(StringView application, Vector<ByteString> arguments)
 {
     auto paths = TRY(get_paths_for_helper_process(application));
 
-    ErrorOr<pid_t> result = -1;
     for (auto const& path : paths) {
         auto path_view = path.view();
-        result = Core::Process::spawn(path_view, arguments, {}, Core::Process::KeepAsChild::Yes);
-        if (!result.is_error())
-            break;
+        auto maybe_process = Core::Process::spawn(Core::ProcessSpawnOptions { .executable = path_view, .arguments = arguments, .keep_as_child = Core::KeepAsChild::Yes });
+        if (!maybe_process.is_error())
+            return maybe_process.value().take_pid();
     }
-    return result;
+    return -1;
 }
 
 static ErrorOr<pid_t> launch_browser(ByteString const& socket_path)
 {
-    auto arguments = Vector {
+    auto arguments = Vector<ByteString> {
         "--webdriver-content-path",
         socket_path.characters(),
     };
@@ -57,7 +56,7 @@ static ErrorOr<pid_t> launch_headless_browser(ByteString const& socket_path)
 {
     auto resources = ByteString::formatted("{}/res", s_serenity_resource_root);
     return launch_process("headless-browser"sv,
-        Array {
+        Vector<ByteString> {
             "--resources",
             resources.characters(),
             "--webdriver-ipc-path",

Tests/LibCore/TestLibCoreProcess.cpp

@@ -0,0 +1,74 @@
+/*
+ * Copyright (c) 2025, Lucas Chollet <[email protected]>
+ *
+ * SPDX-License-Identifier: BSD-2-Clause
+ */
+
+#include <LibCore/Process.h>
+#include <LibTest/TestCase.h>
+
+TEST_CASE(crash_on_api_misuse)
+{
+    {
+        auto process = TRY_OR_FAIL(Core::Process::spawn({ .executable = "/bin/true",
+            .keep_as_child = Core::KeepAsChild::No }));
+
+        EXPECT_CRASH("calling wait_for_termination() on disowned child", [&] {
+            EXPECT(!process.wait_for_termination().is_error());
+            return Test::Crash::Failure::DidNotCrash;
+        });
+
+        EXPECT_CRASH("calling take_pid() on disowned child", [&] {
+            process.take_pid();
+            return Test::Crash::Failure::DidNotCrash;
+        });
+    }
+
+    {
+        auto process = TRY_OR_FAIL(Core::Process::spawn({ .executable = "/bin/true",
+            .keep_as_child = Core::KeepAsChild::Yes }));
+
+        EXPECT_CRASH("calling take_pid() after wait_for_termination()", [&] {
+            EXPECT(!process.wait_for_termination().is_error());
+            process.take_pid();
+            return Test::Crash::Failure::DidNotCrash;
+        });
+
+        EXPECT_CRASH("calling wait_for_termination() after take_pid()", [&] {
+            EXPECT(!process.wait_for_termination().is_error());
+            process.take_pid();
+            return Test::Crash::Failure::DidNotCrash;
+        });
+        // This creates a zombie process.
+        process.take_pid();
+    }
+
+    EXPECT_CRASH("Require explicit call to wait_for_termination() of take_pid()", [&] {
+        {
+            auto maybe_process = Core::Process::spawn(
+                { .executable = "/bin/true",
+                    .keep_as_child = Core::KeepAsChild::Yes });
+            EXPECT(!maybe_process.is_error());
+        }
+        return Test::Crash::Failure::DidNotCrash;
+    });
+}
+
+TEST_CASE(no_crash)
+{
+    {
+        auto process = TRY_OR_FAIL(Core::Process::spawn({ .executable = "/bin/true",
+            .keep_as_child = Core::KeepAsChild::No }));
+    }
+    {
+        auto process = TRY_OR_FAIL(Core::Process::spawn({ .executable = "/bin/true",
+            .keep_as_child = Core::KeepAsChild::Yes }));
+        TRY_OR_FAIL(process.wait_for_termination());
+    }
+    {
+        auto process = TRY_OR_FAIL(Core::Process::spawn({ .executable = "/bin/true",
+            .keep_as_child = Core::KeepAsChild::Yes }));
+        // This creates a zombie process.
+        process.take_pid();
+    }
+}

Tests/LibELF/TestOrder.cpp

@@ -26,6 +26,7 @@ static ByteBuffer run(ByteString executable)
                 .fd = 1,
             },
         },
+        .keep_as_child = Core::KeepAsChild::Yes,
     }));
     MUST(process.wait_for_termination());
     auto output = MUST(Core::File::open(path_to_captured_output.string(), Core::File::OpenMode::Read));

Userland/Applications/Run/RunWindow.cpp

@@ -108,11 +108,15 @@ void RunWindow::do_run()
 bool RunWindow::run_as_command(ByteString const& run_input)
 {
     // TODO: Query and use the user's preferred shell.
-    auto maybe_child_pid = Core::Process::spawn("/bin/Shell"sv, Array { "-c", run_input.characters() }, {}, Core::Process::KeepAsChild::Yes);
-    if (maybe_child_pid.is_error())
+    auto maybe_child_process = Core::Process::spawn(Core::ProcessSpawnOptions {
+        .executable = "/bin/Shell"sv,
+        .arguments = { "-c", run_input.characters() },
+        .keep_as_child = Core::KeepAsChild::Yes });
+
+    if (maybe_child_process.is_error())
         return false;
 
-    pid_t child_pid = maybe_child_pid.release_value();
+    pid_t child_pid = maybe_child_process.value().take_pid();
 
     // The child shell was able to start. Let's save it to the history immediately so users can see it as the first entry the next time they run this program.
     prepend_history(run_input);

Userland/Applications/Terminal/main.cpp

@@ -103,7 +103,7 @@ class TerminalChangeListener : public Config::Listener {
 static ErrorOr<void> utmp_update(StringView tty, pid_t pid, bool create)
 {
     auto pid_string = String::number(pid);
-    Array utmp_update_command {
+    Vector<ByteString> utmp_update_command {
         "-f"sv,
         "Terminal"sv,
         "-p"sv,
@@ -112,7 +112,11 @@ static ErrorOr<void> utmp_update(StringView tty, pid_t pid, bool create)
         tty,
     };
 
-    auto utmpupdate_pid = TRY(Core::Process::spawn("/bin/utmpupdate"sv, utmp_update_command, {}, Core::Process::KeepAsChild::Yes));
+    auto utmpupdate_pid = TRY(Core::Process::spawn(Core::ProcessSpawnOptions {
+                                  .executable = "/bin/utmpupdate"sv,
+                                  .arguments = utmp_update_command,
+                                  .keep_as_child = Core::KeepAsChild::Yes }))
+                              .take_pid();
 
     Core::System::WaitPidResult status;
     auto wait_successful = false;

Userland/Libraries/LibCore/Process.cpp

@@ -114,7 +114,16 @@ ErrorOr<Process> Process::spawn(ProcessSpawnOptions const& options)
     } else {
         pid = TRY(System::posix_spawn(options.executable.view(), &spawn_actions, nullptr, const_cast<char**>(argv_list.get().data()), Core::Environment::raw_environ()));
     }
-    return Process { pid };
+
+    auto process = Process { pid };
+    if (options.keep_as_child == KeepAsChild::No) {
+        dbgln("Disowning");
+        TRY(process.disown());
+    } else {
+        dbgln("Or not");
+    }
+
+    return process;
 }
 
 ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<ByteString> arguments, ByteString working_directory, KeepAsChild keep_as_child)
@@ -123,14 +132,9 @@ ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<ByteString> argument
         .executable = path,
         .arguments = Vector<ByteString> { arguments },
         .working_directory = working_directory.is_empty() ? Optional<ByteString> {} : Optional<ByteString> { working_directory },
+        .keep_as_child = keep_as_child,
     }));
 
-    if (keep_as_child == KeepAsChild::No)
-        TRY(process.disown());
-    else {
-        // FIXME: This won't be needed if return value is changed to Process.
-        process.m_should_disown = false;
-    }
     return process.m_pid;
 }
 
@@ -145,12 +149,9 @@ ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<StringView> argument
         .executable = path,
         .arguments = backing_strings,
         .working_directory = working_directory.is_empty() ? Optional<ByteString> {} : Optional<ByteString> { working_directory },
+        .keep_as_child = keep_as_child,
     }));
 
-    if (keep_as_child == KeepAsChild::No)
-        TRY(process.disown());
-    else
-        process.m_should_disown = false;
     return process.m_pid;
 }
 
@@ -165,12 +166,9 @@ ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<char const*> argumen
         .executable = path,
         .arguments = backing_strings,
         .working_directory = working_directory.is_empty() ? Optional<ByteString> {} : Optional<ByteString> { working_directory },
+        .keep_as_child = keep_as_child,
     }));
 
-    if (keep_as_child == KeepAsChild::No)
-        TRY(process.disown());
-    else
-        process.m_should_disown = false;
     return process.m_pid;
 }
 
@@ -320,22 +318,20 @@ void Process::wait_for_debugger_and_break()
 
 ErrorOr<void> Process::disown()
 {
-    if (m_pid != 0 && m_should_disown) {
 #ifdef AK_OS_SERENITY
-        TRY(System::disown(m_pid));
+    TRY(System::disown(m_pid));
 #else
-        // FIXME: Support disown outside Serenity.
+    // FIXME: Support disown outside Serenity.
 #endif
-        m_should_disown = false;
-        return {};
-    } else {
-        return Error::from_errno(EINVAL);
-    }
+    m_was_managed = true;
+    return {};
 }
 
 ErrorOr<bool> Process::wait_for_termination()
 {
     VERIFY(m_pid > 0);
+    VERIFY(!m_was_managed);
+    m_was_managed = true;
 
     bool exited_with_code_0 = true;
     int status;
@@ -353,7 +349,6 @@ ErrorOr<bool> Process::wait_for_termination()
         VERIFY_NOT_REACHED();
     }
 
-    m_should_disown = false;
     return exited_with_code_0;
 }
 
@@ -457,7 +452,7 @@ ErrorOr<IPCProcess::ProcessPaths> IPCProcess::paths_for_process(StringView proce
 ErrorOr<IPCProcess::ProcessAndIPCSocket> IPCProcess::spawn_singleton_and_connect_to_process(ProcessSpawnOptions const& options)
 {
     auto [socket_path, pid_path] = TRY(paths_for_process(options.name));
-    Process process { -1 };
+    Optional<Process> process;
 
     if (auto existing_pid = TRY(get_process_pid(options.name, pid_path)); existing_pid.has_value()) {
         process = Process { *existing_pid };
@@ -504,7 +499,7 @@ ErrorOr<IPCProcess::ProcessAndIPCSocket> IPCProcess::spawn_singleton_and_connect
     auto ipc_socket = TRY(LocalSocket::connect(socket_path));
     TRY(ipc_socket->set_blocking(true));
 
-    return ProcessAndIPCSocket { move(process), move(ipc_socket) };
+    return ProcessAndIPCSocket { process.release_value(), move(ipc_socket) };
 }
 
 }

Userland/Libraries/LibCore/Process.h

@@ -35,6 +35,11 @@ struct CloseFile {
 
 }
 
+enum class KeepAsChild {
+    Yes,
+    No
+};
+
 struct ProcessSpawnOptions {
     StringView name {};
     ByteString executable {};
@@ -44,6 +49,8 @@ struct ProcessSpawnOptions {
 
     using FileActionType = Variant<FileAction::OpenFile, FileAction::CloseFile>;
     Vector<FileActionType> file_actions {};
+
+    KeepAsChild keep_as_child = KeepAsChild::No;
 };
 
 class IPCProcess;
@@ -52,27 +59,18 @@ class Process {
     AK_MAKE_NONCOPYABLE(Process);
 
 public:
-    enum class KeepAsChild {
-        Yes,
-        No
-    };
-
     Process(Process&& other)
         : m_pid(exchange(other.m_pid, 0))
-        , m_should_disown(exchange(other.m_should_disown, false))
+        , m_was_managed(exchange(other.m_was_managed, true))
     {
     }
 
-    Process& operator=(Process&& other)
-    {
-        m_pid = exchange(other.m_pid, 0);
-        m_should_disown = exchange(other.m_should_disown, false);
-        return *this;
-    }
+    Process& operator=(Process&& other) = delete;
 
     ~Process()
     {
-        (void)disown();
+        // We want users to explicitly call `disown()`, `take_pid()` or `wait_for_termination()`.
+        VERIFY(m_was_managed);
     }
 
     static ErrorOr<Process> spawn(ProcessSpawnOptions const& options);
@@ -97,19 +95,25 @@ class Process {
     // FIXME: Make it return an exit code.
     ErrorOr<bool> wait_for_termination();
 
+    pid_t take_pid()
+    {
+        VERIFY(!m_was_managed);
+        m_was_managed = true;
+        return exchange(m_pid, 0);
+    }
+
 private:
     friend IPCProcess;
 
     ErrorOr<void> disown();
 
     Process(pid_t pid)
         : m_pid(pid)
-        , m_should_disown(true)
     {
     }
 
-    pid_t m_pid;
-    bool m_should_disown;
+    pid_t m_pid { 0 };
+    bool m_was_managed { false };
 };
 
 class IPCProcess {