Merge branch 'fix-signal' into 'master'

Fix invalid memory reference in signal and sigaction

See merge request redox-os/relibc!239

src/header/signal/mod.rs

@@ -1,6 +1,6 @@
 //! signal implementation for Redox, following http://pubs.opengroup.org/onlinepubs/7908799/xsh/signal.h.html
 
-use core::{mem, ptr};
+use core::mem;
 
 use cbitset::BitSet;
 
@@ -21,20 +21,20 @@ pub mod sys;
 
 type SigSet = BitSet<[c_ulong; 1]>;
 
-pub (crate) const SIG_DFL: usize = 0;
-pub (crate) const SIG_IGN: usize = 1;
-pub (crate) const SIG_ERR: isize = -1;
+pub(crate) const SIG_DFL: usize = 0;
+pub(crate) const SIG_IGN: usize = 1;
+pub(crate) const SIG_ERR: isize = -1;
 
 pub const SIG_BLOCK: c_int = 0;
 pub const SIG_UNBLOCK: c_int = 1;
 pub const SIG_SETMASK: c_int = 2;
 
 #[repr(C)]
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct sigaction {
-    pub sa_handler: extern "C" fn(c_int),
+    pub sa_handler: Option<extern "C" fn(c_int)>,
     pub sa_flags: c_ulong,
-    pub sa_restorer: unsafe extern "C" fn(),
+    pub sa_restorer: Option<unsafe extern "C" fn()>,
     pub sa_mask: sigset_t,
 }
 
@@ -86,14 +86,12 @@ pub unsafe extern "C" fn sigaction(
     act: *const sigaction,
     oact: *mut sigaction,
 ) -> c_int {
-    let act_opt = if !act.is_null() {
-        let mut act_clone = (*act).clone();
+    let act_opt = act.as_ref().map(|act| {
+        let mut act_clone = act.clone();
         act_clone.sa_flags |= SA_RESTORER as c_ulong;
-        Some(act_clone)
-    } else {
-        None
-    };
-    Sys::sigaction(sig, act_opt.map_or(ptr::null_mut(), |x| &x), oact)
+        act_clone
+    });
+    Sys::sigaction(sig, act_opt.as_ref(), oact.as_mut())
 }
 
 #[no_mangle]
@@ -194,11 +192,14 @@ extern "C" {
 }
 
 #[no_mangle]
-pub extern "C" fn signal(sig: c_int, func: extern "C" fn(c_int)) -> extern "C" fn(c_int) {
+pub extern "C" fn signal(
+    sig: c_int,
+    func: Option<extern "C" fn(c_int)>,
+) -> Option<extern "C" fn(c_int)> {
     let sa = sigaction {
         sa_handler: func,
         sa_flags: SA_RESTART as c_ulong,
-        sa_restorer: __restore_rt,
+        sa_restorer: Some(__restore_rt),
         sa_mask: sigset_t::default(),
     };
     let mut old_sa = unsafe { mem::uninitialized() };

src/platform/linux/signal.rs

@@ -35,14 +35,16 @@ impl PalSignal for Sys {
         e(unsafe { syscall!(SETITIMER, which, new, old) }) as c_int
     }
 
-    unsafe fn sigaction(sig: c_int, act: *const sigaction, oact: *mut sigaction) -> c_int {
-        e(syscall!(
-            RT_SIGACTION,
-            sig,
-            act,
-            oact,
-            mem::size_of::<sigset_t>()
-        )) as c_int
+    fn sigaction(sig: c_int, act: Option<&sigaction>, oact: Option<&mut sigaction>) -> c_int {
+        e(unsafe {
+            syscall!(
+                RT_SIGACTION,
+                sig,
+                act.map_or_else(core::ptr::null, |x| x as *const _),
+                oact.map_or_else(core::ptr::null_mut, |x| x as *mut _),
+                mem::size_of::<sigset_t>()
+            )
+        }) as c_int
     }
 
     fn sigaltstack(ss: *const stack_t, old_ss: *mut stack_t) -> c_int {

src/platform/pal/signal.rs

@@ -15,7 +15,7 @@ pub trait PalSignal: Pal {
 
     fn setitimer(which: c_int, new: *const itimerval, old: *mut itimerval) -> c_int;
 
-    unsafe fn sigaction(sig: c_int, act: *const sigaction, oact: *mut sigaction) -> c_int;
+    fn sigaction(sig: c_int, act: Option<&sigaction>, oact: Option<&mut sigaction>) -> c_int;
 
     fn sigaltstack(ss: *const stack_t, old_ss: *mut stack_t) -> c_int;
 

src/platform/redox/signal.rs

@@ -105,33 +105,28 @@ impl PalSignal for Sys {
         0
     }
 
-    unsafe fn sigaction(sig: c_int, act: *const sigaction, oact: *mut sigaction) -> c_int {
-        let new_opt = if act.is_null() {
-            None
-        } else {
-            let m = (*act).sa_mask;
-            Some(syscall::SigAction {
-                sa_handler: mem::transmute((*act).sa_handler),
+    fn sigaction(sig: c_int, act: Option<&sigaction>, oact: Option<&mut sigaction>) -> c_int {
+        let new_opt = act.map(|act| {
+            let m = act.sa_mask;
+            let sa_handler = unsafe { mem::transmute(act.sa_handler) };
+            syscall::SigAction {
+                sa_handler,
                 sa_mask: [m as u64, 0],
-                sa_flags: syscall::SigActionFlags::from_bits((*act).sa_flags as usize)
+                sa_flags: syscall::SigActionFlags::from_bits(act.sa_flags as usize)
                     .expect("sigaction: invalid bit pattern"),
-            })
-        };
-        let mut old_opt = if oact.is_null() {
-            None
-        } else {
-            Some(syscall::SigAction::default())
-        };
+            }
+        });
+        let mut old_opt = oact.as_ref().map(|_| syscall::SigAction::default());
         let ret = e(syscall::sigaction(
             sig as usize,
             new_opt.as_ref(),
             old_opt.as_mut(),
         )) as c_int;
-        if let Some(old) = old_opt {
-            (*oact).sa_handler = mem::transmute(old.sa_handler);
+        if let (Some(old), Some(oact)) = (old_opt, oact) {
+            oact.sa_handler = unsafe { mem::transmute(old.sa_handler) };
             let m = old.sa_mask;
-            (*oact).sa_mask = m[0] as c_ulong;
-            (*oact).sa_flags = old.sa_flags.bits() as c_ulong;
+            oact.sa_mask = m[0] as c_ulong;
+            oact.sa_flags = old.sa_flags.bits() as c_ulong;
         }
         ret
     }

tests/Makefile

@@ -96,11 +96,11 @@ EXPECT_NAMES=\
 	wchar/wcsrchr \
 	wchar/wcstod \
 	wchar/wcstok \
-	wchar/wcstol
+	wchar/wcstol \
+	signal
 	# TODO: Fix these
 	# netdb/getaddrinfo \
 	# netdb/netdb \
-	# signal
 
 # Binaries that may generate varied output
 NAMES=\

tests/expected/signal.stdout

@@ -0,0 +1,3 @@
+Raising...
+Signal handler called!
+Raised.

tests/signal.c

@@ -11,8 +11,10 @@ void handler(int sig) {
 }
 
 int main(void) {
-    void (*signal_status)(int) = signal(SIGUSR1, &handler);
+    void (*signal_status)(int) = signal(SIGUSR1, handler);
     ERROR_IF(signal, signal_status, == SIG_ERR);
+    signal_status = signal(SIGUSR1, handler);
+    ERROR_IF(signal, signal_status, != handler);
 
     puts("Raising...");