Skip to content

Commit

Permalink
clone3: support CLONE_CLEAR_SIGHAND and don't clear if not set
Browse files Browse the repository at this point in the history
Previously fork-like invocations of clone incorrectly reset signal
handlers, as if `CLONE_CLEAR_SIGHAND` was used.

The default is now fixed to copy signal handlers from the parent
process, and we add support for the `CLONE_CLEAR_SIGHAND` flag to clear
them.
  • Loading branch information
sporksmith committed Jan 17, 2024
1 parent 9497f7a commit ca84567
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 4 deletions.
5 changes: 3 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
8 changes: 8 additions & 0 deletions src/lib/shadow-shim-helper-rs/src/shim_shmem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
25 changes: 25 additions & 0 deletions src/main/host/syscall/handler/clone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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())
Expand Down
3 changes: 1 addition & 2 deletions src/test/clone/test_fork.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1889,8 +1889,7 @@ fn main() -> Result<(), Box<dyn Error>> {
tests.push(ShadowTest::new(
&format!("test_handles_CLONE_CLEAR_SIGHAND={value}"),
move || test_clone_clear_sighand(value),
// TODO: https://github.com/shadow/shadow/issues/3266
libc_only.clone(),
all_envs.clone(),
));
}

Expand Down

0 comments on commit ca84567

Please sign in to comment.