fix!: mark gix::interrupt::init_handler() as unsafe

martinvonz · Byron · commit 59b8104a5320 · 2023-12-08T08:59:20.000+01:00
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.
diff --git a/gix/src/interrupt.rs b/gix/src/interrupt.rs
@@ -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> {
diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs
@@ -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 {
diff --git a/src/porcelain/main.rs b/src/porcelain/main.rs
@@ -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;
