Skip to content

Commit

Permalink
Don't return to parent after fork
Browse files Browse the repository at this point in the history
This can cause closed files to be closed again, which will panic and is
a violation of IO safety.
  • Loading branch information
bjorn3 committed Oct 21, 2024
1 parent b107f22 commit 2b2bb1b
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 39 deletions.
16 changes: 0 additions & 16 deletions src/exec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ use crate::{
common::Environment,
log::dev_warn,
system::{
_exit,
interface::ProcessId,
killpg,
signal::{consts::*, signal_name},
Expand All @@ -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<ExecOutput> {
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<ProcessOutput> {
// FIXME: should we pipe the stdio streams?
let qualified_path = options.command()?;
let mut command = Command::new(qualified_path);
Expand Down Expand Up @@ -127,13 +118,6 @@ pub struct ExecOutput {
pub restore_signal_handlers: Box<dyn FnOnce()>,
}

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 {
Expand Down
17 changes: 8 additions & 9 deletions src/exec/no_pty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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},
Expand All @@ -26,7 +26,7 @@ use crate::{
},
};

pub(super) fn exec_no_pty(sudo_pid: ProcessId, mut command: Command) -> io::Result<ProcessOutput> {
pub(super) fn exec_no_pty(sudo_pid: ProcessId, mut command: Command) -> io::Result<ExecOutput> {
// 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
Expand Down Expand Up @@ -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}");
Expand All @@ -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)),
})
}

Expand Down
12 changes: 7 additions & 5 deletions src/exec/use_pty/monitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@ use crate::{
exec::{
event::{PollEvent, StopReason},
use_pty::{SIGCONT_BG, SIGCONT_FG},
ProcessOutput,
ExecOutput,
},
log::{dev_error, dev_info, dev_warn},
system::FileCloser,
};
use crate::{
exec::{handle_sigchld, terminate_process, HandleSigchld},
system::{
fork, getpgid, getpgrp,
_exit, fork, getpgid, getpgrp,
interface::ProcessId,
kill, setpgid, setsid,
term::{PtyFollower, Terminal},
Expand All @@ -44,7 +44,7 @@ pub(super) fn exec_monitor(
backchannel: &mut MonitorBackchannel,
mut file_closer: FileCloser,
original_set: Option<SignalSet>,
) -> io::Result<ProcessOutput> {
) -> io::Result<ExecOutput> {
// 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) {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
18 changes: 9 additions & 9 deletions src/exec/use_pty/parent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
Expand All @@ -32,7 +33,7 @@ pub(in crate::exec) fn exec_pty(
sudo_pid: ProcessId,
mut command: Command,
user_tty: UserTerm,
) -> io::Result<ProcessOutput> {
) -> io::Result<ExecOutput> {
// Allocate a pseudoterminal.
let pty = get_pty()?;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)),
})
}

Expand Down

0 comments on commit 2b2bb1b

Please sign in to comment.