Skip to content

Commit

Permalink
fix!: mark gix::interrupt::init_handler() as unsafe
Browse files Browse the repository at this point in the history
The passed `interrupt()` argument will be called from a signal
handler, so that needs to be documented and the call sites need to
state that they fulfill the contract.

Thanks to @Manishearth for pointing this out.
  • Loading branch information
martinvonz authored and Byron committed Dec 8, 2023
1 parent d77bc0e commit 59b8104
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 9 deletions.
5 changes: 4 additions & 1 deletion gix/src/interrupt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,10 @@ mod init {
///
/// It will abort the process on second press and won't inform the user about this behaviour either as we are unable to do so without
/// deadlocking even when trying to write to stderr directly.
pub fn init_handler(
///
/// SAFETY: `interrupt()` will be called from a signal handler. See `signal_hook::low_level::register()` for details about.
#[allow(unsafe_code)]
pub unsafe fn init_handler(
grace_count: usize,
interrupt: impl Fn() + Send + Sync + Clone + 'static,
) -> io::Result<Deregister> {
Expand Down
12 changes: 8 additions & 4 deletions src/plumbing/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,14 @@ pub fn main() -> Result<()> {
let auto_verbose = !progress && !args.no_verbose;

let should_interrupt = Arc::new(AtomicBool::new(false));
gix::interrupt::init_handler(1, {
let should_interrupt = Arc::clone(&should_interrupt);
move || should_interrupt.store(true, Ordering::SeqCst)
})?;
#[allow(unsafe_code)]
unsafe {
// SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
gix::interrupt::init_handler(1, {
let should_interrupt = Arc::clone(&should_interrupt);
move || should_interrupt.store(true, Ordering::SeqCst)
})?;
}

match cmd {
Subcommands::Status(crate::plumbing::options::status::Platform {
Expand Down
12 changes: 8 additions & 4 deletions src/porcelain/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,14 @@ pub fn main() -> Result<()> {
time::util::local_offset::set_soundness(time::util::local_offset::Soundness::Unsound);
}
let should_interrupt = Arc::new(AtomicBool::new(false));
gix::interrupt::init_handler(1, {
let should_interrupt = Arc::clone(&should_interrupt);
move || should_interrupt.store(true, Ordering::SeqCst)
})?;
#[allow(unsafe_code)]
unsafe {
// SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
gix::interrupt::init_handler(1, {
let should_interrupt = Arc::clone(&should_interrupt);
move || should_interrupt.store(true, Ordering::SeqCst)
})?;
}
let trace = false;
let verbose = !args.quiet;
let progress = args.progress;
Expand Down

0 comments on commit 59b8104

Please sign in to comment.