From 26d8ac844c4f35b7b3fb97d62af84dfb352252cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?kleines=20Filmr=C3=B6llchen?= Date: Tue, 27 Jun 2023 18:03:09 +0200 Subject: [PATCH] Terminal: Modernize - Replace C with C++ - Use Core::System, Core::Account and Core::Process wrappers - Remove DeprecatedString - Remove invalid close() call to the pts file descriptor in the shell child (the fd cannot be valid there anymore since it's an automatically-closing fd, we just previously ignored this error) --- Userland/Applications/Terminal/main.cpp | 106 +++++++++++------------- 1 file changed, 50 insertions(+), 56 deletions(-) diff --git a/Userland/Applications/Terminal/main.cpp b/Userland/Applications/Terminal/main.cpp index 435b3fb37e..22112255a1 100644 --- a/Userland/Applications/Terminal/main.cpp +++ b/Userland/Applications/Terminal/main.cpp @@ -6,9 +6,11 @@ #include #include +#include #include #include #include +#include #include #include #include @@ -35,17 +37,7 @@ #include #include #include -#include -#include #include -#include -#include -#include -#include -#include -#include -#include -#include class TerminalChangeListener : public Config::Listener { public: @@ -111,45 +103,45 @@ private: VT::TerminalWidget& m_parent_terminal; }; -static void utmp_update(DeprecatedString const& tty, pid_t pid, bool create) +static ErrorOr utmp_update(StringView tty, pid_t pid, bool create) { - int utmpupdate_pid = fork(); - if (utmpupdate_pid < 0) { - perror("fork"); - return; - } - if (utmpupdate_pid == 0) { - // Be careful here! Because fork() only clones one thread it's - // possible that we deadlock on anything involving a mutex, - // including the heap! So resort to low-level APIs - char pid_str[32]; - snprintf(pid_str, sizeof(pid_str), "%d", pid); - execl("/bin/utmpupdate", "/bin/utmpupdate", "-f", "Terminal", "-p", pid_str, (create ? "-c" : "-d"), tty.characters(), nullptr); - } else { - wait_again: - int status = 0; - if (waitpid(utmpupdate_pid, &status, 0) < 0) { - int err = errno; - if (err == EINTR) - goto wait_again; - perror("waitpid"); - return; + auto pid_string = TRY(String::number(pid)); + Array utmp_update_command { + "-f"sv, + "Terminal"sv, + "-p"sv, + pid_string.bytes_as_string_view(), + (create ? "-c"sv : "-d"sv), + tty, + }; + + auto utmpupdate_pid = TRY(Core::Process::spawn("/bin/utmpupdate"sv, utmp_update_command, {}, Core::Process::KeepAsChild::Yes)); + + Core::System::WaitPidResult status; + auto wait_successful = false; + while (!wait_successful) { + auto result = Core::System::waitpid(utmpupdate_pid, 0); + if (result.is_error() && result.error().code() != EINTR) { + return result.release_error(); + } else if (!result.is_error()) { + status = result.release_value(); + wait_successful = true; } - if (WIFEXITED(status) && WEXITSTATUS(status) != 0) - dbgln("Terminal: utmpupdate exited with status {}", WEXITSTATUS(status)); - else if (WIFSIGNALED(status)) - dbgln("Terminal: utmpupdate exited due to unhandled signal {}", WTERMSIG(status)); } + + if (WIFEXITED(status.status) && WEXITSTATUS(status.status) != 0) + dbgln("Terminal: utmpupdate exited with status {}", WEXITSTATUS(status.status)); + else if (WIFSIGNALED(status.status)) + dbgln("Terminal: utmpupdate exited due to unhandled signal {}", WTERMSIG(status.status)); + + return {}; } -static ErrorOr run_command(DeprecatedString command, bool keep_open) +static ErrorOr run_command(StringView command, bool keep_open) { - DeprecatedString shell = "/bin/Shell"; - auto* pw = getpwuid(getuid()); - if (pw && pw->pw_shell) { - shell = pw->pw_shell; - } - endpwent(); + auto shell = TRY(String::from_deprecated_string(TRY(Core::Account::self(Core::Account::Read::PasswdOnly)).shell())); + if (shell.is_empty()) + shell = TRY("/bin/Shell"_string); Vector arguments; arguments.append(shell); @@ -239,7 +231,10 @@ ErrorOr serenity_main(Main::Arguments arguments) TRY(Core::System::pledge("stdio tty rpath cpath wpath recvfd sendfd proc exec unix sigaction")); struct sigaction act; - memset(&act, 0, sizeof(act)); + act.sa_mask = 0; + // Do not trust that both function pointers overlap. + act.sa_sigaction = nullptr; + act.sa_flags = SA_NOCLDWAIT; act.sa_handler = SIG_IGN; @@ -267,12 +262,11 @@ ErrorOr serenity_main(Main::Arguments arguments) int ptm_fd; pid_t shell_pid = forkpty(&ptm_fd, nullptr, nullptr, nullptr); - if (shell_pid < 0) { - perror("forkpty"); - return 1; - } + if (shell_pid < 0) + return Error::from_errno(errno); + + // We're the child process; run the startup command. if (shell_pid == 0) { - close(ptm_fd); if (!command_to_execute.is_empty()) TRY(run_command(command_to_execute, keep_open)); else @@ -281,7 +275,7 @@ ErrorOr serenity_main(Main::Arguments arguments) } auto ptsname = TRY(Core::System::ptsname(ptm_fd)); - utmp_update(ptsname, shell_pid, true); + TRY(utmp_update(ptsname, shell_pid, true)); auto app_icon = GUI::Icon::default_icon("app-terminal"sv); @@ -356,7 +350,7 @@ ErrorOr serenity_main(Main::Arguments arguments) auto shell_child_process_count = [&] { int background_process_count = 0; - Core::Directory::for_each_entry(DeprecatedString::formatted("/proc/{}/children", shell_pid), Core::DirIterator::Flags::SkipParentAndBaseDir, [&](auto&, auto&) { + Core::Directory::for_each_entry(String::formatted("/proc/{}/children", shell_pid).release_value_but_fixme_should_propagate_errors(), Core::DirIterator::Flags::SkipParentAndBaseDir, [&](auto&, auto&) { ++background_process_count; return IterationDecision::Continue; }).release_value_but_fixme_should_propagate_errors(); @@ -366,17 +360,17 @@ ErrorOr serenity_main(Main::Arguments arguments) auto check_terminal_quit = [&]() -> GUI::Dialog::ExecResult { if (!should_confirm_close) return GUI::MessageBox::ExecResult::OK; - Optional close_message; + Optional close_message; auto title = "Running Process"sv; if (tty_has_foreground_process()) { - close_message = "Close Terminal and kill its foreground process?"; + close_message = "Close Terminal and kill its foreground process?"_string.release_value_but_fixme_should_propagate_errors(); } else { auto child_process_count = shell_child_process_count(); if (child_process_count > 1) { title = "Running Processes"sv; - close_message = DeprecatedString::formatted("Close Terminal and kill its {} background processes?", child_process_count); + close_message = String::formatted("Close Terminal and kill its {} background processes?", child_process_count).release_value_but_fixme_should_propagate_errors(); } else if (child_process_count == 1) { - close_message = "Close Terminal and kill its background process?"; + close_message = "Close Terminal and kill its background process?"_string.release_value_but_fixme_should_propagate_errors(); } } if (close_message.has_value()) @@ -471,6 +465,6 @@ ErrorOr serenity_main(Main::Arguments arguments) modified_state_check_timer->start(); int result = app->exec(); dbgln("Exiting terminal, updating utmp"); - utmp_update(ptsname, 0, false); + TRY(utmp_update(ptsname, 0, false)); return result; }