diff --git a/Cargo.toml b/Cargo.toml index 377a321c9..f2d4a7117 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,7 +29,7 @@ name = "visudo" path = "bin/visudo.rs" [dependencies] -libc = "0.2.127" +libc = "0.2.149" glob = "0.3.0" log = { version = "0.4.11", features = ["std"] } diff --git a/src/cutils/mod.rs b/src/cutils/mod.rs index ab1f489a5..a643afe78 100644 --- a/src/cutils/mod.rs +++ b/src/cutils/mod.rs @@ -24,11 +24,13 @@ extern "C" { } pub fn set_errno(no: libc::c_int) { + // SAFETY: errno_location is a thread-local pointer to an integer, so we are the only writers unsafe { *errno_location() = no }; } pub fn sysconf(name: libc::c_int) -> Option { set_errno(0); + // SAFETY: sysconf will always respond with 0 or -1 for every input cerr(unsafe { libc::sysconf(name) }).ok() } @@ -43,6 +45,7 @@ pub unsafe fn string_from_ptr(ptr: *const libc::c_char) -> String { if ptr.is_null() { String::new() } else { + // SAFETY: the function contract says that CStr::from_ptr is safe let cstr = unsafe { CStr::from_ptr(ptr) }; cstr.to_string_lossy().to_string() } @@ -57,6 +60,7 @@ pub unsafe fn os_string_from_ptr(ptr: *const libc::c_char) -> OsString { if ptr.is_null() { OsString::new() } else { + // SAFETY: the function contract says that CStr::from_ptr is safe let cstr = unsafe { CStr::from_ptr(ptr) }; OsStr::from_bytes(cstr.to_bytes()).to_owned() } @@ -69,13 +73,17 @@ pub fn safe_isatty(fildes: libc::c_int) -> bool { // The Rust standard library doesn't have FileTypeExt on Std{in,out,err}, so we // can't just use FileTypeExt::is_char_device and have to resort to libc::fstat. let mut maybe_stat = std::mem::MaybeUninit::::uninit(); + + // SAFETY: we are passing fstat a pointer to valid memory if unsafe { libc::fstat(fildes, maybe_stat.as_mut_ptr()) } == 0 { + // SAFETY: if `fstat` returned 0, maybe_stat will be initialized let mode = unsafe { maybe_stat.assume_init() }.st_mode; // To complicate matters further, the S_ISCHR macro isn't in libc as well. let is_char_device = (mode & libc::S_IFMT) == libc::S_IFCHR; if is_char_device { + // SAFETY: isatty will return 0 or 1 unsafe { libc::isatty(fildes) != 0 } } else { false diff --git a/src/exec/event.rs b/src/exec/event.rs index e5707fecb..6f186812d 100644 --- a/src/exec/event.rs +++ b/src/exec/event.rs @@ -178,6 +178,9 @@ impl EventRegistry { return Ok(ids); } + // SAFETY: `poll` expects a pointer to an array of file descriptors (first argument), + // the length of which is indicated by the second argument; the third argument being -1 + // denotes an infinite timeout. // FIXME: we should set either a timeout or use ppoll when available. cerr(unsafe { libc::poll(fds.as_mut_ptr(), fds.len() as _, -1) })?; diff --git a/src/pam/converse.rs b/src/pam/converse.rs index 2ca26a6fc..91859f102 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -203,8 +203,13 @@ pub(super) unsafe extern "C" fn converse( messages: Vec::with_capacity(num_msg as usize), }; for i in 0..num_msg as isize { + // SAFETY: the PAM contract ensures that `num_msg` does not exceed the amount + // of messages presented to this function in `msg`, and that it is not being + // written to at the same time as we are reading it. Note that the reference + // we create does not escape this loopy body. let message: &pam_message = unsafe { &**msg.offset(i) }; + // SAFETY: PAM ensures that the messages passed are properly null-terminated let msg = unsafe { string_from_ptr(message.msg) }; let style = if let Some(style) = PamMessageStyle::from_int(message.msg_style) { style @@ -221,6 +226,7 @@ pub(super) unsafe extern "C" fn converse( } // send the conversation of to the Rust part + // SAFETY: appdata_ptr contains the `*mut ConverserData` that is untouched by PAM let app_data = unsafe { &mut *(appdata_ptr as *mut ConverserData) }; if app_data .converser @@ -232,6 +238,8 @@ pub(super) unsafe extern "C" fn converse( // Conversation should now contain response messages // allocate enough memory for the responses, set it to zero + // SAFETY: this will either allocate the required amount of (initialized) bytes, + // or return a null pointer. let temp_resp = unsafe { libc::calloc( num_msg as libc::size_t, @@ -244,6 +252,9 @@ pub(super) unsafe extern "C" fn converse( // Store the responses for (i, msg) in conversation.messages.into_iter().enumerate() { + // SAFETY: `i` will not exceed `num_msg` by the way `conversation_messages` + // is constructed, so `temp_resp` will have allocated-and-initialized data at + // the required offset that only we have a writable pointer to. let response: &mut pam_response = unsafe { &mut *(temp_resp.add(i)) }; if let Some(secbuf) = msg.response { @@ -252,6 +263,7 @@ pub(super) unsafe extern "C" fn converse( } // Set the responses + // SAFETY: PAM contract says that we are passed a valid, non-null, writeable pointer here. unsafe { *response = temp_resp }; PamErrorType::Success @@ -262,6 +274,7 @@ pub(super) unsafe extern "C" fn converse( Ok(r) => r, Err(_) => { // notify caller that a panic has occured + // SAFETY: appdata_ptr contains the `*mut ConverserData` that is untouched by PAM let app_data = unsafe { &mut *(appdata_ptr as *mut ConverserData) }; app_data.panicked = true; diff --git a/src/pam/error.rs b/src/pam/error.rs index ba468cbe5..93a808f51 100644 --- a/src/pam/error.rs +++ b/src/pam/error.rs @@ -154,13 +154,14 @@ impl PamErrorType { } fn get_err_msg(&self) -> String { - // pam_strerror technically takes a pam handle as the first argument, + // SAFETY: pam_strerror technically takes a pam handle as the first argument, // but we do not know of any implementation that actually uses the pamh // argument. See also the netbsd man page for `pam_strerror`. let data = unsafe { pam_strerror(std::ptr::null_mut(), self.as_int()) }; if data.is_null() { String::from("Error unresolved by PAM") } else { + // SAFETY: pam_strerror returns a pointer to a null-terminated string unsafe { string_from_ptr(data) } } } diff --git a/src/pam/mod.rs b/src/pam/mod.rs index 2cef3e4b9..701fc6089 100644 --- a/src/pam/mod.rs +++ b/src/pam/mod.rs @@ -57,6 +57,8 @@ impl PamContextBuilder { })); let mut pamh = std::ptr::null_mut(); + // SAFETY: we are passing the required fields to `pam_start`; in particular, the value + // of `pamh` set above is not used, but will be overwritten by `pam_start`. let res = unsafe { pam_start( c_service_name.as_ptr(), @@ -161,6 +163,7 @@ impl PamContext { flags |= self.silent_flag(); flags |= self.disallow_null_auth_token_flag(); + // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`) pam_err(unsafe { pam_authenticate(self.pamh, flags) })?; if self.has_panicked() { @@ -175,6 +178,7 @@ impl PamContext { flags |= self.silent_flag(); flags |= self.disallow_null_auth_token_flag(); + // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`) pam_err(unsafe { pam_acct_mgmt(self.pamh, flags) }) } @@ -195,6 +199,8 @@ impl PamContext { /// Set the user that will be authenticated. pub fn set_user(&mut self, user: &str) -> PamResult<()> { let c_user = CString::new(user)?; + // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`); furthermore, + // `c_user.as_ptr()` will point to a correct null-terminated string. pam_err(unsafe { pam_set_item(self.pamh, PAM_USER, c_user.as_ptr() as *const libc::c_void) }) @@ -203,14 +209,17 @@ impl PamContext { /// Get the user that is currently active in the PAM handle pub fn get_user(&mut self) -> PamResult { let mut data = std::ptr::null(); + // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`) pam_err(unsafe { pam_get_item(self.pamh, PAM_USER, &mut data) })?; - // safety check to make sure that we do not ready a null ptr into a cstr + // safety check to make sure that we were not passed a null pointer by PAM, + // or that in fact PAM did not write to `data` at all. if data.is_null() { return Err(PamError::InvalidState); } - // unsafe conversion to cstr + // SAFETY: the contract for `pam_get_item` ensures that if `data` was touched by + // `pam_get_item`, it will point to a valid null-terminated string. let cstr = unsafe { CStr::from_ptr(data as *const c_char) }; Ok(cstr.to_str()?.to_owned()) @@ -219,12 +228,16 @@ impl PamContext { /// Set the TTY path for the current TTY that this PAM session started from. pub fn set_tty>(&mut self, tty_path: P) -> PamResult<()> { let data = CString::new(tty_path.as_ref().as_bytes())?; + // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`); furthermore, + // `data.as_ptr()` will point to a correct null-terminated string. pam_err(unsafe { pam_set_item(self.pamh, PAM_TTY, data.as_ptr() as *const libc::c_void) }) } // Set the user that requested the actions in this PAM instance. pub fn set_requesting_user(&mut self, user: &str) -> PamResult<()> { let data = CString::new(user.as_bytes())?; + // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`); furthermore, + // `data.as_ptr()` will point to a correct null-terminated string. pam_err(unsafe { pam_set_item(self.pamh, PAM_RUSER, data.as_ptr() as *const libc::c_void) }) } @@ -238,6 +251,7 @@ impl PamContext { let mut flags = action; flags |= self.silent_flag(); + // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`). pam_err(unsafe { pam_setcred(self.pamh, flags) }) } @@ -252,12 +266,14 @@ impl PamContext { if expired_only { flags |= PAM_CHANGE_EXPIRED_AUTHTOK; } + // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`). pam_err(unsafe { pam_chauthtok(self.pamh, flags) }) } /// Start a user session for the authenticated user. pub fn open_session(&mut self) -> PamResult<()> { if !self.session_started { + // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`). pam_err(unsafe { pam_open_session(self.pamh, self.silent_flag()) })?; self.session_started = true; Ok(()) @@ -269,6 +285,7 @@ impl PamContext { /// End the user session. pub fn close_session(&mut self) -> PamResult<()> { if self.session_started { + // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`). pam_err(unsafe { pam_close_session(self.pamh, self.silent_flag()) })?; self.session_started = false; Ok(()) @@ -280,13 +297,25 @@ impl PamContext { /// Get a full listing of the current PAM environment pub fn env(&mut self) -> PamResult> { let mut res = HashMap::new(); + // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`). + // The man page for pam_getenvlist states that: + // The format of the memory is a malloc()'d array of char pointers, the last element + // of which is set to NULL. Each of the non-NULL entries in this array point to a + // NUL terminated and malloc()'d char string of the form: "name=value". + // + // The pam_getenvlist function returns NULL on failure. let envs = unsafe { pam_getenvlist(self.pamh) }; if envs.is_null() { return Err(PamError::EnvListFailure); } let mut curr_env = envs; + // SAFETY: the loop invariant is as follows: + // - `curr_env` itself is always a valid pointer to an array of valid (possibly NULL) pointers + // - if `curr_env` points to a pointer that is not-null, that data is a c-string allocated by malloc() + // - `curr_env` points to NULL if and only if it is the final element in the array while let Some(curr_str) = NonNull::new(unsafe { curr_env.read() }) { let data = { + // SAFETY: `curr_str` points to a valid null-terminated string per the above let cstr = unsafe { CStr::from_ptr(curr_str.as_ptr()) }; let bytes = cstr.to_bytes(); if let Some(pos) = bytes.iter().position(|b| *b == b'=') { @@ -301,12 +330,16 @@ impl PamContext { res.insert(k, v); } - // free the current string and move to the next one + // SAFETY: curr_str was obtained via libc::malloc() so we are responsible for freeing it. + // At this point, curr_str is also the only remaining pointer/reference to that allocated data + // (the data was copied above), so it can be deallocated without risk of use-after-free errors. unsafe { libc::free(curr_str.as_ptr().cast()) }; + // SAFETY: curr_env was not NULL, so it was not the last element in the list and so PAM + // ensures that the next offset also is a valid pointer, and points to valid data. curr_env = unsafe { curr_env.offset(1) }; } - // free the entire array + // SAFETY: `envs` itself was obtained by malloc(), so we are reponsible for freeing it. unsafe { libc::free(envs.cast()) }; Ok(res) @@ -314,6 +347,7 @@ impl PamContext { /// Check if anything panicked since the last call. pub fn has_panicked(&self) -> bool { + // SAFETY: self.data_ptr was created by Box::into_raw unsafe { (*self.data_ptr).panicked } } } @@ -336,6 +370,7 @@ impl PamContext { impl Drop for PamContext { fn drop(&mut self) { // data_ptr's pointee is de-allocated in this scope + // SAFETY: self.data_ptr was created by Box::into_raw let _data = unsafe { Box::from_raw(self.data_ptr) }; let _ = self.close_session(); @@ -343,6 +378,7 @@ impl Drop for PamContext { // it is unclear what it really does and does not do, other than the vague // documentation description to 'not take the call to seriously' // Also see https://github.com/systemd/systemd/issues/22318 + // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`) unsafe { pam_end( self.pamh, diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index 50327da9e..6325ee44b 100644 --- a/src/pam/rpassword.rs +++ b/src/pam/rpassword.rs @@ -51,6 +51,7 @@ impl HiddenInput { term.c_lflag |= ECHONL; // Save the settings for now. + // SAFETY: we are passing tcsetattr a valid file descriptor and pointer-to-struct cerr(unsafe { tcsetattr(fd, TCSANOW, &term) })?; Ok(Some(HiddenInput { tty, term_orig })) @@ -60,6 +61,7 @@ impl HiddenInput { impl Drop for HiddenInput { fn drop(&mut self) { // Set the the mode back to normal + // SAFETY: we are passing tcsetattr a valid file descriptor and pointer-to-struct unsafe { tcsetattr(self.tty.as_raw_fd(), TCSANOW, &self.term_orig); } @@ -68,7 +70,9 @@ impl Drop for HiddenInput { fn safe_tcgetattr(fd: RawFd) -> io::Result { let mut term = mem::MaybeUninit::::uninit(); + // SAFETY: we are passing tcgetattr a pointer to valid memory cerr(unsafe { ::libc::tcgetattr(fd, term.as_mut_ptr()) })?; + // SAFETY: if the previous call was a success, `tcgetattr` has initialized `term` Ok(unsafe { term.assume_init() }) } diff --git a/src/pam/securemem.rs b/src/pam/securemem.rs index 9772896d6..b4149af20 100644 --- a/src/pam/securemem.rs +++ b/src/pam/securemem.rs @@ -42,6 +42,8 @@ impl PamBuffer { impl Default for PamBuffer { fn default() -> Self { + // SAFETY: `calloc` returns either a cleared, allocated chunk of `SIZE` bytes + // or NULL to indicate that the allocation request failed let res = unsafe { libc::calloc(1, SIZE) }; if let Some(nn) = NonNull::new(res) { PamBuffer(nn.cast()) @@ -55,20 +57,26 @@ impl std::ops::Deref for PamBuffer { type Target = [u8]; fn deref(&self) -> &[u8] { - // make the slice one less in size to guarantee the existence of a terminating NUL + // SAFETY: `self.0.as_ptr()` is non-null, aligned, and initialized, and points to `SIZE` bytes. + // The lifetime of the slice does not exceed that of `self`. + // + // We make the slice one less in size to guarantee the existence of a terminating NUL. unsafe { slice::from_raw_parts(self.0.as_ptr().cast(), SIZE - 1) } } } impl std::ops::DerefMut for PamBuffer { fn deref_mut(&mut self) -> &mut [u8] { + // SAFETY: see above unsafe { slice::from_raw_parts_mut(self.0.as_ptr().cast(), SIZE - 1) } } } impl Drop for PamBuffer { fn drop(&mut self) { + // SAFETY: same as for `deref()` and `deref_mut()` wipe_memory(unsafe { self.0.as_mut() }); + // SAFETY: `self.0.as_ptr()` was obtained via `calloc`, so calling `free` is proper. unsafe { libc::free(self.0.as_ptr().cast()) } } } @@ -80,6 +88,7 @@ fn wipe_memory(memory: &mut [u8]) { let nonsense: u8 = 0x55; for c in memory { + // SAFETY: `c` is safe for writes (it comes from a &mut reference) unsafe { std::ptr::write_volatile(c, nonsense) }; } diff --git a/src/system/file/chown.rs b/src/system/file/chown.rs index 83cce5f3b..bbd9674a7 100644 --- a/src/system/file/chown.rs +++ b/src/system/file/chown.rs @@ -13,6 +13,8 @@ impl Chown for File { fn chown(&self, owner: UserId, group: GroupId) -> io::Result<()> { let fd = self.as_raw_fd(); + // SAFETY: `fchown` is passed a proper file descriptor; and even if the user/group id + // is invalid, it will not cause UB. cerr(unsafe { libc::fchown(fd, owner, group) }).map(|_| ()) } } diff --git a/src/system/file/lock.rs b/src/system/file/lock.rs index aa4ff3e9b..98eade1b4 100644 --- a/src/system/file/lock.rs +++ b/src/system/file/lock.rs @@ -52,6 +52,8 @@ fn flock(fd: RawFd, action: LockOp, nonblocking: bool) -> Result<()> { operation |= libc::LOCK_NB; } + // SAFETY: even if `fd` would not be a valid file descriptor, that would merely + // result in an error condition, not UB cerr(unsafe { libc::flock(fd, operation) })?; Ok(()) } diff --git a/src/system/mod.rs b/src/system/mod.rs index 9dda04053..5ae667999 100644 --- a/src/system/mod.rs +++ b/src/system/mod.rs @@ -49,10 +49,12 @@ pub(crate) fn can_execute>(path: P) -> bool { return false; }; + // SAFETY: we are passing a proper pointer to access unsafe { libc::access(path.as_ptr(), libc::X_OK) == 0 } } pub(crate) fn _exit(status: libc::c_int) -> ! { + // SAFETY: this function is safe to call unsafe { libc::_exit(status) } } @@ -106,7 +108,11 @@ impl FileCloser { fn close_range(min_fd: c_uint, max_fd: c_uint) -> io::Result<()> { if min_fd <= max_fd { - cerr(unsafe { libc::syscall(libc::SYS_close_range, min_fd, max_fd, 0 as c_uint) })?; + // SAFETY: this function is safe to call: + // - any errors while closing a specific fd will be effectively ignored + // - if the provided range or flags are invalid, that will be reported + // as an error but will not cause undefined behaviour + cerr(unsafe { libc::close_range(min_fd, max_fd, 0) })?; } Ok(()) @@ -119,6 +125,10 @@ pub(crate) enum ForkResult { Child, } +/// # Safety +/// +/// In a multithreaded program, only async-signal-safe functions are guaranteed to work in the +/// child process until a call to `execve` or a similar function is done. unsafe fn inner_fork() -> io::Result { let pid = cerr(unsafe { libc::fork() })?; if pid == 0 { @@ -148,6 +158,7 @@ pub(crate) unsafe fn fork() -> io::Result { } pub fn setsid() -> io::Result { + // SAFETY: this function is memory-safe to call cerr(unsafe { libc::setsid() }) } @@ -198,8 +209,10 @@ impl Hostname { let buffer_size = max_hostname_size + 1 /* null byte delimiter */ ; let mut buf = vec![0; buffer_size]; + // SAFETY: we are passing a valid pointer to gethostname match cerr(unsafe { libc::gethostname(buf.as_mut_ptr(), buffer_size) }) { Ok(_) => Self { + // SAFETY: gethostname succeeded, so `buf` will hold a null-terminated C string inner: unsafe { string_from_ptr(buf.as_ptr()) }, }, @@ -218,6 +231,12 @@ pub fn syslog(priority: libc::c_int, facility: libc::c_int, message: &CStr) { Err(_) => panic!("syslog formatting string is not null-terminated"), }; + // SAFETY: + // - "MSG" is a constant expression that is a null-terminated C string that represents "%s"; + // this also means that to achieve safety we MUST pass one more argument to syslog that is a proper + // pointer to a null-terminated C string + // - message.as_ptr() is a pointer to a proper null-terminated C string (message being a &CStr) + // for more info: read the manpage for syslog(2) unsafe { libc::syslog(priority | facility, MSG, message.as_ptr()); } @@ -269,6 +288,7 @@ pub fn killpg(pgid: ProcessId, signal: SignalNumber) -> io::Result<()> { /// Get the process group ID of the current process. pub fn getpgrp() -> ProcessId { + // SAFETY: This function is always safe to call unsafe { libc::getpgrp() } } @@ -280,6 +300,8 @@ pub fn getpgid(pid: ProcessId) -> io::Result { /// Set a process group ID. pub fn setpgid(pid: ProcessId, pgid: ProcessId) -> io::Result<()> { + // SAFETY: This function cannot cause UB even if `pid` or `pgid` are not a valid process IDs: + // https://pubs.opengroup.org/onlinepubs/007904975/functions/setpgid.html cerr(unsafe { libc::setpgid(pid, pgid) }).map(|_| ()) } @@ -292,6 +314,8 @@ pub fn chown>( let uid = uid.into(); let gid = gid.into(); + // SAFETY: path is a valid pointer to a null-terminated C string; chown cannot cause safety + // issues even if uid and/or gid would be invalid identifiers. cerr(unsafe { libc::chown(path, uid, gid) }).map(|_| ()) } @@ -317,6 +341,8 @@ impl User { while { groups_buffer = vec![0; buf_len as usize]; + // SAFETY: getgrouplist is passed valid pointers + // in particular `groups_buffer` is an array of `buf.len()` bytes, as required let result = unsafe { libc::getgrouplist( pwd.pw_name, @@ -356,6 +382,10 @@ impl User { let mut buf = vec![0; max_pw_size as usize]; let mut pwd = MaybeUninit::uninit(); let mut pwd_ptr = std::ptr::null_mut(); + // SAFETY: getpwuid_r is passed valid (although partly uninitialized) pointers to memory, + // in particular `buf` points to an array of `buf.len()` bytes, as required. + // After this call, if `pwd_ptr` is not NULL, `*pwd_ptr` and `pwd` will be aliased; + // but we never dereference `pwd_ptr`. cerr(unsafe { libc::getpwuid_r( uid, @@ -368,24 +398,31 @@ impl User { if pwd_ptr.is_null() { Ok(None) } else { + // SAFETY: pwd_ptr was not null, and getpwuid_r succeeded, so we have assurances that + // the `pwd` structure was written to by getpwuid_r let pwd = unsafe { pwd.assume_init() }; + // SAFETY: `pwd` was obtained by a call to getpwXXX_r, as required. unsafe { Self::from_libc(&pwd).map(Some) } } } pub fn effective_uid() -> UserId { + // SAFETY: this function cannot cause memory safety issues unsafe { libc::geteuid() } } pub fn effective_gid() -> GroupId { + // SAFETY: this function cannot cause memory safety issues unsafe { libc::getegid() } } pub fn real_uid() -> UserId { + // SAFETY: this function cannot cause memory safety issues unsafe { libc::getuid() } } pub fn real_gid() -> GroupId { + // SAFETY: this function cannot cause memory safety issues unsafe { libc::getgid() } } @@ -399,6 +436,7 @@ impl User { let mut pwd = MaybeUninit::uninit(); let mut pwd_ptr = std::ptr::null_mut(); + // SAFETY: analogous to getpwuid_r above cerr(unsafe { libc::getpwnam_r( name_c.as_ptr(), @@ -411,7 +449,10 @@ impl User { if pwd_ptr.is_null() { Ok(None) } else { + // SAFETY: pwd_ptr was not null, and getpwnam_r succeeded, so we have assurances that + // the `pwd` structure was written to by getpwnam_r let pwd = unsafe { pwd.assume_init() }; + // SAFETY: `pwd` was obtained by a call to getpwXXX_r, as required. unsafe { Self::from_libc(&pwd).map(Some) } } } @@ -441,6 +482,7 @@ impl Group { let mut buf = vec![0; max_gr_size as usize]; let mut grp = MaybeUninit::uninit(); let mut grp_ptr = std::ptr::null_mut(); + // SAFETY: analogous to getpwuid_r above cerr(unsafe { libc::getgrgid_r( gid, @@ -453,7 +495,10 @@ impl Group { if grp_ptr.is_null() { Ok(None) } else { + // SAFETY: grp_ptr was not null, and getgrgid_r succeeded, so we have assurances that + // the `grp` structure was written to by getgrgid_r let grp = unsafe { grp.assume_init() }; + // SAFETY: `pwd` was obtained by a call to getgrXXX_r, as required. Ok(Some(unsafe { Group::from_libc(&grp) })) } } @@ -463,6 +508,7 @@ impl Group { let mut buf = vec![0; max_gr_size as usize]; let mut grp = MaybeUninit::uninit(); let mut grp_ptr = std::ptr::null_mut(); + // SAFETY: analogous to getpwuid_r above cerr(unsafe { libc::getgrnam_r( name_c.as_ptr(), @@ -475,7 +521,10 @@ impl Group { if grp_ptr.is_null() { Ok(None) } else { + // SAFETY: grp_ptr was not null, and getgrgid_r succeeded, so we have assurances that + // the `grp` structure was written to by getgrgid_r let grp = unsafe { grp.assume_init() }; + // SAFETY: `pwd` was obtained by a call to getgrXXX_r, as required. Ok(Some(unsafe { Group::from_libc(&grp) })) } } @@ -538,6 +587,8 @@ impl Process { /// Get the session id for the current process pub fn session_id() -> ProcessId { + // SAFETY: this function is explicitly safe to call with argument 0, + // and more generally getsid will never cause memory safety issues. unsafe { libc::getsid(0) } } diff --git a/src/system/signal/stream.rs b/src/system/signal/stream.rs index f9ac67b68..f9236f3a6 100644 --- a/src/system/signal/stream.rs +++ b/src/system/signal/stream.rs @@ -18,12 +18,17 @@ use super::{ static STREAM: OnceLock = OnceLock::new(); +/// # Safety +/// +/// The `info` parameters has to point to a valid instance of SignalInfo pub(super) unsafe fn send_siginfo( _signal: SignalNumber, info: *const SignalInfo, _context: *const libc::c_void, ) { if let Some(tx) = STREAM.get().map(|stream| stream.tx.as_raw_fd()) { + // SAFETY: called ensures that info is a valid pointer; any instance of SignalInfo will + // consists of SignalInfo::SIZE bytes unsafe { libc::send(tx, info.cast(), SignalInfo::SIZE, libc::MSG_DONTWAIT) }; } } diff --git a/src/system/time.rs b/src/system/time.rs index a1de977e5..f8c59ddbb 100644 --- a/src/system/time.rs +++ b/src/system/time.rs @@ -20,6 +20,7 @@ impl SystemTime { pub fn now() -> std::io::Result { let mut spec = MaybeUninit::::uninit(); + // SAFETY: valid pointer is passed to clock_gettime crate::cutils::cerr(unsafe { libc::clock_gettime(libc::CLOCK_BOOTTIME, spec.as_mut_ptr()) })?; diff --git a/src/system/wait.rs b/src/system/wait.rs index ecb081d09..4b8407c18 100644 --- a/src/system/wait.rs +++ b/src/system/wait.rs @@ -27,6 +27,7 @@ impl Wait for ProcessId { fn wait(self, options: WaitOptions) -> Result<(ProcessId, WaitStatus), WaitError> { let mut status: c_int = 0; + // SAFETY: a valid pointer is passed to `waitpid` let pid = cerr(unsafe { libc::waitpid(self, &mut status, options.flags) }) .map_err(WaitError::Io)?;