From dedaed3a23b0f28e63cbfddec45c87a825778334 Mon Sep 17 00:00:00 2001 From: kosayoda Date: Fri, 20 Sep 2024 09:50:35 -0400 Subject: [PATCH 1/3] Fix incorrect posix_spawn* error checking The posix_spawn functions do not use `-1` as the sentinel for errors, rather `0` is return on success and every other return value is an error. Adds a test case to test posix_spawn failure. --- src/spawn.rs | 77 ++++++++++++++++++++++++++++++++++------------ test/test_spawn.rs | 14 +++++++++ 2 files changed, 72 insertions(+), 19 deletions(-) diff --git a/src/spawn.rs b/src/spawn.rs index eeb9b7871d..2f84c6b576 100644 --- a/src/spawn.rs +++ b/src/spawn.rs @@ -25,7 +25,9 @@ impl PosixSpawnAttr { let mut attr = mem::MaybeUninit::uninit(); let res = unsafe { libc::posix_spawnattr_init(attr.as_mut_ptr()) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } let attr = unsafe { attr.assume_init() }; Ok(PosixSpawnAttr { attr }) @@ -43,14 +45,18 @@ impl PosixSpawnAttr { &mut self.attr as *mut libc::posix_spawnattr_t, ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } let res = unsafe { libc::posix_spawnattr_init( &mut self.attr as *mut libc::posix_spawnattr_t, ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } Ok(self) } @@ -65,7 +71,9 @@ impl PosixSpawnAttr { flags.bits() as libc::c_short, ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } Ok(()) } @@ -81,7 +89,9 @@ impl PosixSpawnAttr { &mut flags, ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } Ok(PosixSpawnFlags::from_bits_truncate(flags.into())) } @@ -96,7 +106,9 @@ impl PosixSpawnAttr { pgroup.as_raw(), ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } Ok(()) } @@ -113,7 +125,9 @@ impl PosixSpawnAttr { &mut pid, ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } Ok(Pid::from_raw(pid)) } @@ -130,7 +144,9 @@ impl PosixSpawnAttr { sigdefault.as_ref(), ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } Ok(()) } @@ -147,7 +163,9 @@ impl PosixSpawnAttr { sigset.as_mut_ptr(), ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } let sigdefault = unsafe { SigSet::from_sigset_t_unchecked(sigset.assume_init()) }; @@ -164,7 +182,9 @@ impl PosixSpawnAttr { sigdefault.as_ref(), ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } Ok(()) } @@ -181,7 +201,9 @@ impl PosixSpawnAttr { sigset.as_mut_ptr(), ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } let sigdefault = unsafe { SigSet::from_sigset_t_unchecked(sigset.assume_init()) }; @@ -242,7 +264,10 @@ impl PosixSpawnFileActions { let res = unsafe { libc::posix_spawn_file_actions_init(actions.as_mut_ptr()) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } + Ok(unsafe { PosixSpawnFileActions { fa: actions.assume_init(), @@ -262,14 +287,18 @@ impl PosixSpawnFileActions { &mut self.fa as *mut libc::posix_spawn_file_actions_t, ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } let res = unsafe { libc::posix_spawn_file_actions_init( &mut self.fa as *mut libc::posix_spawn_file_actions_t, ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } Ok(self) } @@ -285,7 +314,9 @@ impl PosixSpawnFileActions { newfd, ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } Ok(()) } @@ -311,7 +342,9 @@ impl PosixSpawnFileActions { mode.bits(), ) })?; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } Ok(()) } @@ -327,7 +360,9 @@ impl PosixSpawnFileActions { fd, ) }; - Errno::result(res)?; + if res != 0 { + return Err(Errno::from_raw(res)); + } Ok(()) } @@ -383,8 +418,10 @@ pub fn posix_spawn, SE: AsRef>( env_p.as_ptr(), ) }; + if res != 0 { + return Err(Errno::from_raw(res)); + } - Errno::result(res)?; Ok(Pid::from_raw(pid)) } @@ -412,7 +449,9 @@ pub fn posix_spawnp, SE: AsRef>( env_p.as_ptr(), ) }; + if res != 0 { + return Err(Errno::from_raw(res)); + } - Errno::result(res)?; Ok(Pid::from_raw(pid)) } diff --git a/test/test_spawn.rs b/test/test_spawn.rs index 1283c96ca8..5ad591d309 100644 --- a/test/test_spawn.rs +++ b/test/test_spawn.rs @@ -63,3 +63,17 @@ fn spawn_sleep() { } }; } + +#[test] +fn spawn_fail() { + let bin = &CString::new("3f0ffc950ccd2fb8").unwrap(); + let args = &[ + CString::new("3f0ffc950ccd2fb8").unwrap(), + ]; + let vars: &[CString] = &[]; + let actions = PosixSpawnFileActions::init().unwrap(); + let attr = PosixSpawnAttr::init().unwrap(); + + let result = spawn::posix_spawnp(bin, &actions, &attr, args, vars); + assert!(result.is_err()); +} From 91d37e52c92842fcb929ba3ab810808de82b942c Mon Sep 17 00:00:00 2001 From: kosayoda Date: Mon, 23 Sep 2024 10:36:25 -0400 Subject: [PATCH 2/3] Address rustfmt --- test/test_spawn.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/test_spawn.rs b/test/test_spawn.rs index 5ad591d309..90592e1627 100644 --- a/test/test_spawn.rs +++ b/test/test_spawn.rs @@ -67,9 +67,7 @@ fn spawn_sleep() { #[test] fn spawn_fail() { let bin = &CString::new("3f0ffc950ccd2fb8").unwrap(); - let args = &[ - CString::new("3f0ffc950ccd2fb8").unwrap(), - ]; + let args = &[CString::new("3f0ffc950ccd2fb8").unwrap()]; let vars: &[CString] = &[]; let actions = PosixSpawnFileActions::init().unwrap(); let attr = PosixSpawnAttr::init().unwrap(); From 8d1cda730f70e700f2b9953a6df011b0647932c0 Mon Sep 17 00:00:00 2001 From: kosayoda Date: Mon, 23 Sep 2024 11:23:56 -0400 Subject: [PATCH 3/3] Prevent race conditions on waiting for children --- test/test_spawn.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/test_spawn.rs b/test/test_spawn.rs index 90592e1627..06bb5a30a7 100644 --- a/test/test_spawn.rs +++ b/test/test_spawn.rs @@ -6,6 +6,7 @@ use nix::sys::wait::{waitpid, WaitPidFlag, WaitStatus}; #[test] fn spawn_true() { + let _m = crate::FORK_MTX.lock(); let bin = &CString::new("true").unwrap(); let args = &[ CString::new("true").unwrap(), @@ -32,6 +33,8 @@ fn spawn_true() { #[test] fn spawn_sleep() { + let _m = crate::FORK_MTX.lock(); + let bin = &CString::new("sleep").unwrap(); let args = &[CString::new("sleep").unwrap(), CString::new("30").unwrap()]; let vars: &[CString] = &[]; @@ -66,6 +69,8 @@ fn spawn_sleep() { #[test] fn spawn_fail() { + let _m = crate::FORK_MTX.lock(); + let bin = &CString::new("3f0ffc950ccd2fb8").unwrap(); let args = &[CString::new("3f0ffc950ccd2fb8").unwrap()]; let vars: &[CString] = &[];