From 2b2bb1bb9042274de2b39e4c79578aed6e78f38a Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 21 Oct 2024 15:30:13 +0200 Subject: [PATCH] Don't return to parent after fork This can cause closed files to be closed again, which will panic and is a violation of IO safety. --- src/exec/mod.rs | 16 ---------------- src/exec/no_pty.rs | 17 ++++++++--------- src/exec/use_pty/monitor.rs | 12 +++++++----- src/exec/use_pty/parent.rs | 18 +++++++++--------- 4 files changed, 24 insertions(+), 39 deletions(-) diff --git a/src/exec/mod.rs b/src/exec/mod.rs index 7d1bbc093..e64b68506 100644 --- a/src/exec/mod.rs +++ b/src/exec/mod.rs @@ -19,7 +19,6 @@ use crate::{ common::Environment, log::dev_warn, system::{ - _exit, interface::ProcessId, killpg, signal::{consts::*, signal_name}, @@ -46,14 +45,6 @@ use self::{ /// Returns the [`ExitReason`] of the command and a function that restores the default handler for /// signals once its called. pub fn run_command(options: &impl RunOptions, env: Environment) -> io::Result { - match run_command_internal(options, env)? { - ProcessOutput::SudoExit { output } => Ok(output), - // We call `_exit` instead of `exit` to avoid flushing the parent's IO streams by accident. - ProcessOutput::ChildExit => _exit(1), - } -} - -fn run_command_internal(options: &impl RunOptions, env: Environment) -> io::Result { // FIXME: should we pipe the stdio streams? let qualified_path = options.command()?; let mut command = Command::new(qualified_path); @@ -127,13 +118,6 @@ pub struct ExecOutput { pub restore_signal_handlers: Box, } -enum ProcessOutput { - // The main process exited. - SudoExit { output: ExecOutput }, - // A forked child process exited. - ChildExit, -} - /// Exit reason for the command executed by sudo. #[derive(Debug)] pub enum ExitReason { diff --git a/src/exec/no_pty.rs b/src/exec/no_pty.rs index 93175b7cc..0acb62b97 100644 --- a/src/exec/no_pty.rs +++ b/src/exec/no_pty.rs @@ -4,7 +4,7 @@ use super::{ event::PollEvent, event::{EventRegistry, Process, StopReason}, io_util::was_interrupted, - terminate_process, ExitReason, HandleSigchld, ProcessOutput, + terminate_process, ExecOutput, ExitReason, HandleSigchld, }; use crate::{ common::bin_serde::BinPipe, @@ -17,7 +17,7 @@ use crate::{ exec::{handle_sigchld, opt_fmt, signal_fmt}, log::{dev_error, dev_info, dev_warn}, system::{ - fork, getpgid, getpgrp, + _exit, fork, getpgid, getpgrp, interface::ProcessId, kill, killpg, term::{Terminal, UserTerm}, @@ -26,7 +26,7 @@ use crate::{ }, }; -pub(super) fn exec_no_pty(sudo_pid: ProcessId, mut command: Command) -> io::Result { +pub(super) fn exec_no_pty(sudo_pid: ProcessId, mut command: Command) -> io::Result { // FIXME (ogsudo): Initialize the policy plugin's session here. // Block all the signals until we are done setting up the signal handlers so we don't miss @@ -74,7 +74,8 @@ pub(super) fn exec_no_pty(sudo_pid: ProcessId, mut command: Command) -> io::Resu errpipe_tx.write(&error_code).ok(); } - return Ok(ProcessOutput::ChildExit); + // We call `_exit` instead of `exit` to avoid flushing the parent's IO streams by accident. + _exit(1); }; dev_info!("executed command with pid {command_pid}"); @@ -95,11 +96,9 @@ pub(super) fn exec_no_pty(sudo_pid: ProcessId, mut command: Command) -> io::Resu StopReason::Exit(reason) => reason, }; - Ok(ProcessOutput::SudoExit { - output: crate::exec::ExecOutput { - command_exit_reason, - restore_signal_handlers: Box::new(move || drop(closure.signal_handlers)), - }, + Ok(crate::exec::ExecOutput { + command_exit_reason, + restore_signal_handlers: Box::new(move || drop(closure.signal_handlers)), }) } diff --git a/src/exec/use_pty/monitor.rs b/src/exec/use_pty/monitor.rs index bb321db43..24d86acf0 100644 --- a/src/exec/use_pty/monitor.rs +++ b/src/exec/use_pty/monitor.rs @@ -17,7 +17,7 @@ use crate::{ exec::{ event::{PollEvent, StopReason}, use_pty::{SIGCONT_BG, SIGCONT_FG}, - ProcessOutput, + ExecOutput, }, log::{dev_error, dev_info, dev_warn}, system::FileCloser, @@ -25,7 +25,7 @@ use crate::{ use crate::{ exec::{handle_sigchld, terminate_process, HandleSigchld}, system::{ - fork, getpgid, getpgrp, + _exit, fork, getpgid, getpgrp, interface::ProcessId, kill, setpgid, setsid, term::{PtyFollower, Terminal}, @@ -44,7 +44,7 @@ pub(super) fn exec_monitor( backchannel: &mut MonitorBackchannel, mut file_closer: FileCloser, original_set: Option, -) -> io::Result { +) -> io::Result { // SIGTTIN and SIGTTOU are ignored here but the docs state that it shouldn't // be possible to receive them in the first place. Investigate match SignalHandler::register(SIGTTIN, SignalHandlerBehavior::Ignore) { @@ -103,7 +103,8 @@ pub(super) fn exec_monitor( errpipe_tx.write(&error_code).ok(); } - return Ok(ProcessOutput::ChildExit); + // We call `_exit` instead of `exit` to avoid flushing the parent's IO streams by accident. + _exit(1); }; // Send the command's PID to the parent. @@ -187,7 +188,8 @@ pub(super) fn exec_monitor( // FIXME (ogsudo): The tty is restored here if selinux is available. - Ok(ProcessOutput::ChildExit) + // We call `_exit` instead of `exit` to avoid flushing the parent's IO streams by accident. + _exit(1); } // FIXME: This should return `io::Result` but `!` is not stable yet. diff --git a/src/exec/use_pty/parent.rs b/src/exec/use_pty/parent.rs index 37e530df6..19b5a2bb6 100644 --- a/src/exec/use_pty/parent.rs +++ b/src/exec/use_pty/parent.rs @@ -8,7 +8,6 @@ use crate::exec::use_pty::monitor::exec_monitor; use crate::exec::use_pty::SIGCONT_FG; use crate::exec::{ cond_fmt, handle_sigchld, opt_fmt, signal_fmt, terminate_process, ExecOutput, HandleSigchld, - ProcessOutput, }; use crate::exec::{ io_util::retry_while_interrupted, @@ -22,7 +21,9 @@ use crate::system::signal::{ }; use crate::system::term::{Pty, PtyFollower, PtyLeader, TermSize, Terminal, UserTerm}; use crate::system::wait::WaitOptions; -use crate::system::{chown, fork, getpgrp, kill, killpg, FileCloser, ForkResult, Group, User}; +use crate::system::{ + chown, fork, getpgrp, kill, killpg, FileCloser, ForkResult, Group, User, _exit, +}; use crate::system::{getpgid, interface::ProcessId}; use super::pipe::Pipe; @@ -32,7 +33,7 @@ pub(in crate::exec) fn exec_pty( sudo_pid: ProcessId, mut command: Command, user_tty: UserTerm, -) -> io::Result { +) -> io::Result { // Allocate a pseudoterminal. let pty = get_pty()?; @@ -199,7 +200,8 @@ pub(in crate::exec) fn exec_pty( } } - return Ok(ProcessOutput::ChildExit); + // We call `_exit` instead of `exit` to avoid flushing the parent's IO streams by accident. + _exit(1); }; // Close the file descriptors that we don't access @@ -252,11 +254,9 @@ pub(in crate::exec) fn exec_pty( } } - Ok(ProcessOutput::SudoExit { - output: ExecOutput { - command_exit_reason: exit_reason?, - restore_signal_handlers: Box::new(move || drop(closure.signal_handlers)), - }, + Ok(ExecOutput { + command_exit_reason: exit_reason?, + restore_signal_handlers: Box::new(move || drop(closure.signal_handlers)), }) }