From 555171cb5517e9a7a1c846a33055ff6e644940dd Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 3 Sep 2023 11:10:23 -0700 Subject: [PATCH] Adopt Rust postfork code This adopts the Rust postfork code, bridging it from C++ exec module. We use direct function calls for the bridge, rather than cxx/autocxx, so that we can be sure that no memory allocations or other shenanigans are happening. --- CMakeLists.txt | 2 +- fish-rust/src/fork_exec/postfork.rs | 57 +++++++++++++++++++++++++++++ src/exec.cpp | 50 ++++++++++++++++++++++--- src/postfork.h | 52 -------------------------- 4 files changed, 102 insertions(+), 59 deletions(-) delete mode 100644 src/postfork.h diff --git a/CMakeLists.txt b/CMakeLists.txt index dcf7edb40..417439cf6 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -120,7 +120,7 @@ set(FISH_SRCS src/io.cpp src/null_terminated_array.cpp src/operation_context.cpp src/output.cpp src/pager.cpp src/parse_execution.cpp src/parse_util.cpp - src/parser.cpp src/parser_keywords.cpp src/path.cpp src/postfork.cpp + src/parser.cpp src/parser_keywords.cpp src/path.cpp src/proc.cpp src/reader.cpp src/screen.cpp src/signals.cpp src/utf8.cpp src/wcstringutil.cpp src/wgetopt.cpp src/wildcard.cpp diff --git a/fish-rust/src/fork_exec/postfork.rs b/fish-rust/src/fork_exec/postfork.rs index 5f8edc3ab..3720c45f1 100644 --- a/fish-rust/src/fork_exec/postfork.rs +++ b/fish-rust/src/fork_exec/postfork.rs @@ -545,3 +545,60 @@ fn get_interpreter<'a>(command: &CStr, buffer: &'a mut [u8]) -> Option<&'a CStr> }; Some(CStr::from_bytes_with_nul(&buffer[offset..idx.max(offset)]).unwrap()) } + +/// Set up redirections and signal handling in the child process. +mod ffi { + use super::*; + #[no_mangle] + pub extern "C" fn child_setup_process( + claim_tty_from: pid_t, + sigmask: *const libc::sigset_t, + is_forked: bool, + dup2s: *const Dup2List, + ) -> i32 { + let sigmask = unsafe { sigmask.as_ref() }; + let dup2s = unsafe { &*dup2s }; + super::child_setup_process(claim_tty_from, sigmask, is_forked, dup2s) + } + + #[no_mangle] + pub extern "C" fn safe_report_exec_error( + err: i32, + actual_cmd: *const c_char, + argvv: *const *const c_char, + envv: *const *const c_char, + ) { + super::safe_report_exec_error(err, actual_cmd, argvv, envv) + } + + #[no_mangle] + pub extern "C" fn execute_fork() -> pid_t { + super::execute_fork() + } + + #[no_mangle] + pub extern "C" fn execute_setpgid(pid: pid_t, pgroup: pid_t, is_parent: bool) -> i32 { + super::execute_setpgid(pid, pgroup, is_parent) + } + + #[no_mangle] + pub extern "C" fn report_setpgid_error( + err: i32, + is_parent: bool, + pid: pid_t, + desired_pgid: pid_t, + job_id: c_int, + command_str: *const c_char, + argv0_str: *const c_char, + ) { + super::report_setpgid_error( + err, + is_parent, + pid, + desired_pgid, + job_id, + command_str, + argv0_str, + ) + } +} diff --git a/src/exec.cpp b/src/exec.cpp index 270d6b117..8606a3a99 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -49,7 +49,6 @@ #include "null_terminated_array.h" #include "parse_tree.h" #include "parser.h" -#include "postfork.h" #include "proc.h" #include "reader.h" #include "redirection.h" @@ -145,6 +144,22 @@ bool is_thompson_shell_script(const char *path) { return res; } +// Implemented in postfork.rs. We don't use the FFI bridge to avoid the risk of allocations. +extern "C" { +void safe_report_exec_error(int err, const char *actual_cmd, const char *const *argv, + const char *const *envv); + +int child_setup_process(pid_t claim_tty_from, const sigset_t *sigmask, bool is_forked, + const dup2_list_t *dup2s); + +pid_t execute_fork(); + +int execute_setpgid(pid_t pid, pid_t pgroup, bool is_parent); + +void report_setpgid_error(int err, bool is_parent, pid_t pid, pid_t desired_pgid, job_id_t job_id, + const char *cmd, const char *argv0); +} + /// This function is executed by the child process created by a call to fork(). It should be called /// after \c child_setup_process. It calls execve to replace the fish process image with the command /// specified in \c p. It never returns. Called in a forked child! Do not allocate memory, etc. @@ -244,10 +259,17 @@ static void internal_exec(env_stack_t &vars, job_t *j, const io_chain_t &block_i return; } + sigset_t blocked_signals_storage; + sigemptyset(&blocked_signals_storage); + const sigset_t *blocked_signals = nullptr; + if (blocked_signals_for_job(*j, &blocked_signals_storage)) { + blocked_signals = &blocked_signals_storage; + } + // child_setup_process makes sure signals are properly set up. dup2_list_t redirs = dup2_list_resolve_chain_shim(all_ios); - if (child_setup_process(false /* not claim_tty */, *j, false /* not is_forked */, redirs) == - 0) { + if (child_setup_process(false /* not claim_tty */, blocked_signals, false /* not is_forked */, + &redirs) == 0) { // Decrement SHLVL as we're removing ourselves from the shell "stack". if (is_interactive_session()) { auto shlvl_var = vars.get(L"SHLVL", ENV_GLOBAL | ENV_EXPORT); @@ -408,6 +430,20 @@ static launch_result_t fork_child_for_process(const std::shared_ptr &job, pid_t claim_tty_from = (p->leads_pgrp && job->group->wants_terminal()) ? getpgrp() : INVALID_PID; + // Decide if the job wants to set a custom sigmask. + sigset_t blocked_signals_storage; + sigemptyset(&blocked_signals_storage); + const sigset_t *blocked_signals = nullptr; + if (blocked_signals_for_job(*job, &blocked_signals_storage)) { + blocked_signals = &blocked_signals_storage; + } + + // Narrow the command name for error reporting before fork, + // to avoid allocations in the forked child. + std::string narrow_cmd = wcs2string(job->command_wcstr()); + std::string narrow_argv0 = wcs2string(p->argv0()); + job_id_t job_id = job->job_id(); + pid_t pid = execute_fork(); if (pid < 0) { return launch_result_t::failed; @@ -420,18 +456,20 @@ static launch_result_t fork_child_for_process(const std::shared_ptr &job, if (p->leads_pgrp) { job->group->set_pgid(p->pid); } + { auto pgid = job->group->get_pgid(); if (pgid) { if (int err = execute_setpgid(p->pid, pgid->value, is_parent)) { - report_setpgid_error(err, is_parent, pgid->value, job.get(), p); + report_setpgid_error(err, is_parent, p->pid, pgid->value, job_id, + narrow_cmd.c_str(), narrow_argv0.c_str()); } } } if (!is_parent) { // Child process. - child_setup_process(claim_tty_from, *job, true, dup2s); + child_setup_process(claim_tty_from, blocked_signals, true, &dup2s); child_action(); DIE("Child process returned control to fork_child lambda!"); } @@ -533,7 +571,7 @@ static launch_result_t exec_external_command(parser_t &parser, const std::shared const char *actual_cmd = actual_cmd_str.c_str(); filename_ref_t file = parser.libdata().current_filename; -#if FISH_USE_POSIX_SPAWN +#if HAVE_SPAWN_H // Prefer to use posix_spawn, since it's faster on some systems like OS X. if (can_use_posix_spawn_for_job(j, dup2s)) { ++s_fork_count; // spawn counts as a fork+exec diff --git a/src/postfork.h b/src/postfork.h deleted file mode 100644 index 681cd3b70..000000000 --- a/src/postfork.h +++ /dev/null @@ -1,52 +0,0 @@ -// Functions that we may safely call after fork(), of which there are very few. In particular we -// cannot allocate memory, since we're insane enough to call fork from a multithreaded process. -#ifndef FISH_POSTFORK_H -#define FISH_POSTFORK_H - -#include "config.h" - -#ifdef HAVE_SPAWN_H -#include -#endif -#ifndef FISH_USE_POSIX_SPAWN -#define FISH_USE_POSIX_SPAWN HAVE_SPAWN_H -#endif - -#include "common.h" -#include "maybe.h" - -struct Dup2List; -using dup2_list_t = Dup2List; -class job_t; -class process_t; - -/// Tell the proc \p pid to join process group \p pgroup. -/// If \p is_child is true, we are the child process; otherwise we are fish. -/// Called by both parent and child; this is an unavoidable race inherent to Unix. -/// If is_parent is set, then we are the parent process and should swallow EACCESS. -/// \return 0 on success, an errno error code on failure. -int execute_setpgid(pid_t pid, pid_t pgroup, bool is_parent); - -/// Report the error code \p err for a failed setpgid call. -/// Note not all errors should be reported; in particular EACCESS is expected and benign in the -/// parent only. -void report_setpgid_error(int err, bool is_parent, pid_t desired_pgid, const job_t *j, - const process_t *p); - -/// Initialize a new child process. This should be called right away after forking in the child -/// process. This resets signal handlers and applies IO redirections. -/// -/// If \p claim_tty_from is >= 0 and owns the tty, use tcsetpgrp() to claim it. -/// -/// \return 0 on success, -1 on failure, in which case an error will be printed. -int child_setup_process(pid_t claim_tty_from, const job_t &job, bool is_forked, - const dup2_list_t &dup2s); - -/// Call fork(), retrying on failure a few times. -pid_t execute_fork(); - -/// Report an error from failing to exec or posix_spawn a command. -void safe_report_exec_error(int err, const char *actual_cmd, const char *const *argv, - const char *const *envv); - -#endif