Fix signal handler initialization in forked child processes (#3284)

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:
 

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<Signal>) -> 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<core::ffi::c_long, Errno> {
+    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<CloneResult, Errno> {
+    unsafe { clone3_raw(args, core::mem::size_of::<clone_args>()) }.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

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

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())

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<dyn Error>> {
     // FIXME: take as a command-line arg
     let python_path = Path::new("/usr/bin/python3");
@@ -1824,6 +1885,14 @@ fn main() -> Result<(), Box<dyn Error>> {
         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