diff --git a/Ladybird/HelperProcess.cpp b/Ladybird/HelperProcess.cpp index 55a1bfbb716294..3cf0e4cb82d685 100644 --- a/Ladybird/HelperProcess.cpp +++ b/Ladybird/HelperProcess.cpp @@ -33,7 +33,7 @@ static ErrorOr> launch_server_process_impl( } for (auto [i, path] : enumerate(candidate_server_paths)) { - Core::ProcessSpawnOptions options { .name = server_name, .arguments = arguments }; + Core::ProcessSpawnOptions options { .name = server_name, .arguments = arguments, .keep_as_child = Core::KeepAsChild::Yes }; if (enable_callgrind_profiling == Ladybird::EnableCallgrindProfiling::Yes) { options.executable = "valgrind"sv; @@ -47,12 +47,13 @@ static ErrorOr> launch_server_process_impl( if (!result.is_error()) { auto process = result.release_value(); + auto pid = process.process.take_pid(); if constexpr (requires { process.client->set_pid(pid_t {}); }) - process.client->set_pid(process.process.pid()); + process.client->set_pid(pid); if (register_with_process_manager == RegisterWithProcessManager::Yes) - WebView::ProcessManager::the().add_process(WebView::process_type_from_name(server_name), process.process.pid()); + WebView::ProcessManager::the().add_process(WebView::process_type_from_name(server_name), pid); if (enable_callgrind_profiling == Ladybird::EnableCallgrindProfiling::Yes) { dbgln(); diff --git a/Ladybird/WebDriver/main.cpp b/Ladybird/WebDriver/main.cpp index bbd7339fa618e1..55d3e089c574f9 100644 --- a/Ladybird/WebDriver/main.cpp +++ b/Ladybird/WebDriver/main.cpp @@ -18,23 +18,22 @@ static Vector certificates; -static ErrorOr launch_process(StringView application, ReadonlySpan arguments) +static ErrorOr launch_process(StringView application, Vector arguments) { auto paths = TRY(get_paths_for_helper_process(application)); - ErrorOr 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 launch_browser(ByteString const& socket_path) { - auto arguments = Vector { + auto arguments = Vector { "--webdriver-content-path", socket_path.characters(), }; @@ -50,14 +49,14 @@ static ErrorOr launch_browser(ByteString const& socket_path) arguments.append("about:blank"); - return launch_process("Ladybird"sv, arguments.span()); + return launch_process("Ladybird"sv, arguments); } static ErrorOr launch_headless_browser(ByteString const& socket_path) { auto resources = ByteString::formatted("{}/res", s_serenity_resource_root); return launch_process("headless-browser"sv, - Array { + Vector { "--resources", resources.characters(), "--webdriver-ipc-path", diff --git a/Tests/LibCore/CMakeLists.txt b/Tests/LibCore/CMakeLists.txt index a159bf2c21719f..3cc17d481c4115 100644 --- a/Tests/LibCore/CMakeLists.txt +++ b/Tests/LibCore/CMakeLists.txt @@ -5,6 +5,7 @@ set(TEST_SOURCES TestLibCoreFilePermissionsMask.cpp TestLibCoreFileWatcher.cpp TestLibCoreMappedFile.cpp + TestLibCoreProcess.cpp TestLibCorePromise.cpp TestLibCoreSharedSingleProducerCircularQueue.cpp TestLibCoreStream.cpp diff --git a/Tests/LibCore/TestLibCoreProcess.cpp b/Tests/LibCore/TestLibCoreProcess.cpp new file mode 100644 index 00000000000000..4fb5d961d877a5 --- /dev/null +++ b/Tests/LibCore/TestLibCoreProcess.cpp @@ -0,0 +1,74 @@ +/* + * Copyright (c) 2025, Lucas Chollet + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include + +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(); + } +} diff --git a/Tests/LibELF/TestOrder.cpp b/Tests/LibELF/TestOrder.cpp index 05e2da85c7d917..db409044ab9c94 100644 --- a/Tests/LibELF/TestOrder.cpp +++ b/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)); diff --git a/Userland/Applications/Run/RunWindow.cpp b/Userland/Applications/Run/RunWindow.cpp index f118425366361b..f112a34bbfacdb 100644 --- a/Userland/Applications/Run/RunWindow.cpp +++ b/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); diff --git a/Userland/Applications/Screenshot/MainWindow.cpp b/Userland/Applications/Screenshot/MainWindow.cpp index 225ee9a1b7b059..db231e33b4a114 100644 --- a/Userland/Applications/Screenshot/MainWindow.cpp +++ b/Userland/Applications/Screenshot/MainWindow.cpp @@ -82,7 +82,7 @@ void MainWindow::take_screenshot() arguments.append("-c"sv); // FIXME: Place common screenshot code into library and use that - MUST(Core::Process::spawn("/bin/shot"sv, arguments, m_destination->text(), Core::Process::KeepAsChild::No)); + MUST(Core::Process::spawn("/bin/shot"sv, arguments, m_destination->text())); } } diff --git a/Userland/Applications/Terminal/main.cpp b/Userland/Applications/Terminal/main.cpp index e953b7df33a568..56128218f66ec6 100644 --- a/Userland/Applications/Terminal/main.cpp +++ b/Userland/Applications/Terminal/main.cpp @@ -103,7 +103,7 @@ class TerminalChangeListener : public Config::Listener { static ErrorOr utmp_update(StringView tty, pid_t pid, bool create) { auto pid_string = String::number(pid); - Array utmp_update_command { + Vector utmp_update_command { "-f"sv, "Terminal"sv, "-p"sv, @@ -112,7 +112,11 @@ static ErrorOr 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; diff --git a/Userland/Libraries/LibCore/Process.cpp b/Userland/Libraries/LibCore/Process.cpp index 153dda5927663d..edd6af4af17583 100644 --- a/Userland/Libraries/LibCore/Process.cpp +++ b/Userland/Libraries/LibCore/Process.cpp @@ -114,7 +114,12 @@ ErrorOr Process::spawn(ProcessSpawnOptions const& options) } else { pid = TRY(System::posix_spawn(options.executable.view(), &spawn_actions, nullptr, const_cast(argv_list.get().data()), Core::Environment::raw_environ())); } - return Process { pid }; + + auto process = Process { pid }; + if (options.keep_as_child == KeepAsChild::No) + TRY(process.disown()); + + return process; } ErrorOr Process::spawn(StringView path, ReadonlySpan arguments, ByteString working_directory, KeepAsChild keep_as_child) @@ -123,15 +128,10 @@ ErrorOr Process::spawn(StringView path, ReadonlySpan argument .executable = path, .arguments = Vector { arguments }, .working_directory = working_directory.is_empty() ? Optional {} : Optional { 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.pid(); + return process.m_pid; } ErrorOr Process::spawn(StringView path, ReadonlySpan arguments, ByteString working_directory, KeepAsChild keep_as_child) @@ -145,13 +145,10 @@ ErrorOr Process::spawn(StringView path, ReadonlySpan argument .executable = path, .arguments = backing_strings, .working_directory = working_directory.is_empty() ? Optional {} : Optional { 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.pid(); + return process.m_pid; } ErrorOr Process::spawn(StringView path, ReadonlySpan arguments, ByteString working_directory, KeepAsChild keep_as_child) @@ -165,13 +162,10 @@ ErrorOr Process::spawn(StringView path, ReadonlySpan argumen .executable = path, .arguments = backing_strings, .working_directory = working_directory.is_empty() ? Optional {} : Optional { 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.pid(); + return process.m_pid; } ErrorOr Process::get_name() @@ -320,22 +314,20 @@ void Process::wait_for_debugger_and_break() ErrorOr 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 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 +345,6 @@ ErrorOr Process::wait_for_termination() VERIFY_NOT_REACHED(); } - m_should_disown = false; return exited_with_code_0; } @@ -457,7 +448,7 @@ ErrorOr IPCProcess::paths_for_process(StringView proce ErrorOr 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; if (auto existing_pid = TRY(get_process_pid(options.name, pid_path)); existing_pid.has_value()) { process = Process { *existing_pid }; @@ -485,7 +476,7 @@ ErrorOr IPCProcess::spawn_singleton_and_connect auto process = TRY(Process::spawn(options)); { auto pid_file = TRY(File::open(pid_path, File::OpenMode::Write)); - TRY(pid_file->write_until_depleted(ByteString::number(process.pid()))); + TRY(pid_file->write_until_depleted(ByteString::number(process.m_pid))); } TRY(System::kill(getpid(), SIGTERM)); @@ -504,7 +495,7 @@ ErrorOr 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) }; } } diff --git a/Userland/Libraries/LibCore/Process.h b/Userland/Libraries/LibCore/Process.h index 7e401f6dc0e14a..df60535d71dda8 100644 --- a/Userland/Libraries/LibCore/Process.h +++ b/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; Vector 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 spawn(ProcessSpawnOptions const& options); @@ -94,24 +92,28 @@ class Process { static void wait_for_debugger_and_break(); static ErrorOr is_being_debugged(); - pid_t pid() const { return m_pid; } - - ErrorOr disown(); - // FIXME: Make it return an exit code. ErrorOr wait_for_termination(); + pid_t take_pid() + { + VERIFY(!m_was_managed); + m_was_managed = true; + return exchange(m_pid, 0); + } + private: friend IPCProcess; + ErrorOr 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 { @@ -148,8 +150,6 @@ class IPCProcess { static ErrorOr> get_process_pid(StringView process_name, StringView pid_path); static ErrorOr create_ipc_socket(ByteString const& socket_path); - pid_t pid() const { return m_process.pid(); } - private: struct ProcessAndIPCSocket { Process process; diff --git a/Userland/Services/KeyboardPreferenceLoader/main.cpp b/Userland/Services/KeyboardPreferenceLoader/main.cpp index d15825b3be76c3..5682448a0f2ad5 100644 --- a/Userland/Services/KeyboardPreferenceLoader/main.cpp +++ b/Userland/Services/KeyboardPreferenceLoader/main.cpp @@ -31,7 +31,7 @@ ErrorOr serenity_main(Main::Arguments) if (active_keymap.is_empty()) active_keymap = keymaps_vector.first(); - TRY(Core::Process::spawn("/bin/keymap"sv, Array { "-m", active_keymap.characters() }, {}, Core::Process::KeepAsChild::Yes)); + TRY(Core::Process::spawn("/bin/keymap"sv, Array { "-m", active_keymap.characters() }, {})); bool enable_num_lock = keyboard_settings_config->read_bool_entry("StartupEnable", "NumLock", true); auto keyboard_device = TRY(Core::File::open("/dev/input/keyboard/0"sv, Core::File::OpenMode::Read)); diff --git a/Userland/Utilities/init.cpp b/Userland/Utilities/init.cpp index 363d04610a1d64..2efe1bdd5a9981 100644 --- a/Userland/Utilities/init.cpp +++ b/Userland/Utilities/init.cpp @@ -46,7 +46,7 @@ static ErrorOr populate_device_node_with_symlink(DeviceNodeType device_nod static ErrorOr spawn_device_mapper_process() { - TRY(Core::Process::spawn("/bin/DeviceMapper"sv, ReadonlySpan {}, {}, Core::Process::KeepAsChild::No)); + TRY(Core::Process::spawn("/bin/DeviceMapper"sv, ReadonlySpan {})); return {}; }