diff --git a/CHANGELOG.md b/CHANGELOG.md index c2f79deb0e..8743e6764e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,11 +10,12 @@ MAJOR changes (breaking): MINOR changes (backwards-compatible): -* +* Added support for the `CLONE_CLEAR_SIGHAND` flag for the `clone3` syscall. PATCH changes (bugfixes): -* +* On fork and fork-like invocations of `clone`, signal handlers are now correctly copied +from the parent instead of reset to default (unless `CLONE_CLEAR_SIGHAND` is used). Full changelog since v3.1.0: diff --git a/src/lib/linux-api/src/sched.rs b/src/lib/linux-api/src/sched.rs index 3f0a95920d..7f42b71e4f 100644 --- a/src/lib/linux-api/src/sched.rs +++ b/src/lib/linux-api/src/sched.rs @@ -46,6 +46,39 @@ pub use bindings::linux_clone_args; #[allow(non_camel_case_types)] pub type clone_args = linux_clone_args; +#[allow(clippy::derivable_impls)] +impl Default for clone_args { + fn default() -> Self { + Self { + flags: Default::default(), + pidfd: Default::default(), + child_tid: Default::default(), + parent_tid: Default::default(), + exit_signal: Default::default(), + stack: Default::default(), + stack_size: Default::default(), + tls: Default::default(), + set_tid: Default::default(), + set_tid_size: Default::default(), + cgroup: Default::default(), + } + } +} + +impl clone_args { + #[inline] + pub fn with_flags(mut self, flags: CloneFlags) -> Self { + self.flags = flags.bits(); + self + } + + #[inline] + pub fn with_exit_signal(mut self, exit_signal: Option) -> Self { + self.exit_signal = u64::try_from(Signal::as_raw(exit_signal)).unwrap(); + self + } +} + unsafe impl shadow_pod::Pod for clone_args {} /// The "dumpable" state, as manipulated via the prctl operations `PR_SET_DUMPABLE` and @@ -131,6 +164,15 @@ pub unsafe fn clone_raw( .map_err(Errno::from) } +/// # Safety +/// +/// Too many requirements to list here. See `clone(2)`. +pub unsafe fn clone3_raw(args: *const clone_args, size: usize) -> Result { + unsafe { linux_syscall::syscall!(linux_syscall::SYS_clone3, args, size,) } + .try_i64() + .map_err(Errno::from) +} + pub enum CloneResult { CallerIsChild, // Caller is the parent; child has the given pid @@ -166,6 +208,19 @@ pub unsafe fn clone( }) } +/// # Safety +/// +/// Too many requirements to list here. See `clone(2)`. +pub unsafe fn clone3(args: &clone_args) -> Result { + unsafe { clone3_raw(args, core::mem::size_of::()) }.map(|res| match res.cmp(&0) { + core::cmp::Ordering::Equal => CloneResult::CallerIsChild, + core::cmp::Ordering::Greater => { + CloneResult::CallerIsParent(Pid::from_raw(res.try_into().unwrap()).unwrap()) + } + core::cmp::Ordering::Less => unreachable!(), + }) +} + /// See `fork(2)`. /// /// # Safety diff --git a/src/lib/shadow-shim-helper-rs/src/shim_shmem.rs b/src/lib/shadow-shim-helper-rs/src/shim_shmem.rs index 1780df53ed..45ffcf0a6e 100644 --- a/src/lib/shadow-shim-helper-rs/src/shim_shmem.rs +++ b/src/lib/shadow-shim-helper-rs/src/shim_shmem.rs @@ -230,6 +230,14 @@ impl ProcessShmemProtected { self.pending_standard_siginfos[signal_idx(signal)] = *info; } + /// # Safety + /// + /// Only valid if pointers in `src` sigactions are valid in `self`'s address + /// space (e.g. `src` is a parent that just forked this process). + pub unsafe fn clone_signal_actions(&mut self, src: &Self) { + self.signal_actions = src.signal_actions + } + /// # Safety /// /// Function pointers in `shd_kernel_sigaction::u` are valid only diff --git a/src/main/host/syscall/handler/clone.rs b/src/main/host/syscall/handler/clone.rs index c1ee09de18..0206ae59eb 100644 --- a/src/main/host/syscall/handler/clone.rs +++ b/src/main/host/syscall/handler/clone.rs @@ -172,6 +172,19 @@ impl SyscallHandler { let do_child_cleartid = flags.contains(CloneFlags::CLONE_CHILD_CLEARTID); handled_flags.insert(CloneFlags::CLONE_CHILD_CLEARTID); + let do_copy_sighandlers = if flags.contains(CloneFlags::CLONE_CLEAR_SIGHAND) { + // clone(2): Specifying this flag together with CLONE_SIGHAND is + // nonsensical and disallowed. + if flags.contains(CloneFlags::CLONE_SIGHAND) { + return Err(Errno::EINVAL.into()); + } + false + } else { + // We only need to copy if they're not shared. + !flags.contains(CloneFlags::CLONE_SIGHAND) + }; + handled_flags.insert(CloneFlags::CLONE_CLEAR_SIGHAND); + if flags.contains(CloneFlags::CLONE_PARENT) { // Handled in `new_forked_process` when creating a new process. // No-op when not creating a new process. @@ -261,6 +274,18 @@ impl SyscallHandler { child.set_tid_address(ctid); } + if do_copy_sighandlers { + let shmem_lock = ctx.objs.host.shim_shmem_lock_borrow_mut().unwrap(); + + let parent_shmem = ctx.objs.process.shmem(); + let parent_shmem_prot = parent_shmem.protected.borrow(&shmem_lock.root); + + let child_shmem = child_process_borrow.as_ref().unwrap().shmem(); + let mut child_shmem_prot = child_shmem.protected.borrow_mut(&shmem_lock.root); + // Safety: pointers in the parent are valid in the child. + unsafe { child_shmem_prot.clone_signal_actions(&parent_shmem_prot) }; + } + drop(child_process_borrow); if let Some(c) = child_process_rc { c.explicit_drop(ctx.objs.host.root()) diff --git a/src/test/clone/test_fork.rs b/src/test/clone/test_fork.rs index d194cc2c2b..504e02c2db 100644 --- a/src/test/clone/test_fork.rs +++ b/src/test/clone/test_fork.rs @@ -1190,6 +1190,67 @@ assert handler_counter == 1, "Handler should have run, but appears not to have"" }) } +fn test_clone_clear_sighand(set_clear_sighand: bool) -> anyhow::Result<()> { + // Use a subprocess since we manipulate signal handler. + run_test_in_subprocess(|| { + let flags = if set_clear_sighand { + CloneFlags::CLONE_CLEAR_SIGHAND + } else { + CloneFlags::empty() + }; + + extern "C" fn sig_handler(_signo: c_int, _info: *mut siginfo_t, _ctx: *mut c_void) {} + + // Configure SIGALRM to somethign other than default. + let signal = nix::sys::signal::Signal::SIGALRM; + let parent_action = SigAction::new( + SigHandler::SigAction(sig_handler), + SaFlags::SA_SIGINFO, + SigSet::empty(), + ); + unsafe { nix::sys::signal::sigaction(signal, &parent_action) }.unwrap(); + + let clone_res = unsafe { + // CLONE_CLEAR_SIGHAND isn't supported by `clone`; we need to use `clone3`. + linux_api::sched::clone3( + &linux_api::sched::clone_args::default() + .with_flags(flags) + .with_exit_signal(Some(Signal::SIGCHLD)), + ) + } + .unwrap(); + let child_pid = match clone_res { + CloneResult::CallerIsChild => { + // Ensure we exit with non-zero exit code on panic. + std::panic::set_hook(Box::new(|info| { + eprintln!("panic: {info:?}"); + unsafe { libc::exit(1) }; + })); + + let default_action = + SigAction::new(SigHandler::SigDfl, SaFlags::empty(), SigSet::empty()); + let prev_action = + unsafe { nix::sys::signal::sigaction(signal, &default_action) }.unwrap(); + + let expected_prev_action = if set_clear_sighand { + &default_action + } else { + &parent_action + }; + assert_eq!(prev_action.handler(), expected_prev_action.handler()); + unsafe { libc::exit(0) }; + } + CloneResult::CallerIsParent(pid) => pid, + }; + + let child_pid = nix::unistd::Pid::from_raw(child_pid.as_raw_nonzero().get()); + assert_eq!( + nix::sys::wait::waitpid(Some(child_pid), None).unwrap(), + nix::sys::wait::WaitStatus::Exited(child_pid, 0) + ); + }) +} + fn main() -> Result<(), Box> { // FIXME: take as a command-line arg let python_path = Path::new("/usr/bin/python3"); @@ -1824,6 +1885,14 @@ fn main() -> Result<(), Box> { all_envs.clone(), )); + for value in [true, false] { + tests.push(ShadowTest::new( + &format!("test_handles_CLONE_CLEAR_SIGHAND={value}"), + move || test_clone_clear_sighand(value), + all_envs.clone(), + )); + } + // It'd be good to test signal config across exec, but this is tricky since // python re-initializes it at startup. We might have to write specialized // programs in C to exec, and have them verify or otherwise output the