From 251599c507ce55b39f9f7d1f7e5fb15f7598d130 Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Sat, 30 Mar 2024 13:49:54 +0000 Subject: [PATCH] Use safe `nix` API instead of `libc` (#12315) # Description Where possible, this PR replaces usages of raw `libc` bindings to instead use safe interfaces from the `nix` crate. Where not possible, the `libc` version reexported through `nix` was used instead of having a separate `libc` dependency. --- Cargo.lock | 1 - crates/nu-command/Cargo.toml | 3 +- crates/nu-command/src/debug/info.rs | 12 +-- crates/nu-command/src/filesystem/cd.rs | 55 ++++++------- crates/nu-command/src/filesystem/ls.rs | 9 ++- crates/nu-command/src/filesystem/util.rs | 83 +++++++++----------- crates/nu-command/src/system/run_external.rs | 66 +++++++--------- crates/nu-system/src/foreground.rs | 2 +- 8 files changed, 103 insertions(+), 128 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7f98a77512..4a11f5a6fb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2867,7 +2867,6 @@ dependencies = [ "indexmap", "indicatif", "itertools 0.12.0", - "libc", "log", "lscolors", "md-5", diff --git a/crates/nu-command/Cargo.toml b/crates/nu-command/Cargo.toml index ac550dd819..09201002b7 100644 --- a/crates/nu-command/Cargo.toml +++ b/crates/nu-command/Cargo.toml @@ -106,9 +106,8 @@ winreg = { workspace = true } uucore = { workspace = true, features = ["mode"] } [target.'cfg(unix)'.dependencies] -libc = { workspace = true } umask = { workspace = true } -nix = { workspace = true, default-features = false, features = ["user", "resource"] } +nix = { workspace = true, default-features = false, features = ["user", "resource", "pthread"] } [target.'cfg(any(target_os = "linux", target_os = "android"))'.dependencies] procfs = { workspace = true } diff --git a/crates/nu-command/src/debug/info.rs b/crates/nu-command/src/debug/info.rs index 95f79af4a1..287bcdb5ec 100644 --- a/crates/nu-command/src/debug/info.rs +++ b/crates/nu-command/src/debug/info.rs @@ -71,7 +71,7 @@ impl LazySystemInfoRecord { ) -> Result { let pid = Pid::from(std::process::id() as usize); match column { - "thread_id" => Ok(Value::int(get_thread_id(), self.span)), + "thread_id" => Ok(Value::int(get_thread_id() as i64, self.span)), "pid" => Ok(Value::int(pid.as_u32() as i64, self.span)), "ppid" => { // only get information requested @@ -261,13 +261,13 @@ impl<'a, F: Fn() -> RefreshKind> From<(Option<&'a System>, F)> for SystemOpt<'a> } } -fn get_thread_id() -> i64 { - #[cfg(target_family = "windows")] +fn get_thread_id() -> u64 { + #[cfg(windows)] { - unsafe { windows::Win32::System::Threading::GetCurrentThreadId() as i64 } + unsafe { windows::Win32::System::Threading::GetCurrentThreadId().into() } } - #[cfg(not(target_family = "windows"))] + #[cfg(unix)] { - unsafe { libc::pthread_self() as i64 } + nix::sys::pthread::pthread_self() as u64 } } diff --git a/crates/nu-command/src/filesystem/cd.rs b/crates/nu-command/src/filesystem/cd.rs index 80454d9e99..f46971bace 100644 --- a/crates/nu-command/src/filesystem/cd.rs +++ b/crates/nu-command/src/filesystem/cd.rs @@ -1,16 +1,14 @@ -#[cfg(unix)] -use libc::gid_t; use nu_engine::{command_prelude::*, current_dir}; use std::path::Path; - -// For checking whether we have permission to cd to a directory #[cfg(unix)] -mod file_permissions { - pub type Mode = u32; - pub const USER_EXECUTE: Mode = libc::S_IXUSR as Mode; - pub const GROUP_EXECUTE: Mode = libc::S_IXGRP as Mode; - pub const OTHER_EXECUTE: Mode = libc::S_IXOTH as Mode; -} +use { + crate::filesystem::util::users, + nix::{ + sys::stat::{mode_t, Mode}, + unistd::{Gid, Uid}, + }, + std::os::unix::fs::MetadataExt, +}; // The result of checking whether we have permission to cd to a directory #[derive(Debug)] @@ -170,26 +168,22 @@ fn have_permission(dir: impl AsRef) -> PermissionResult<'static> { #[cfg(unix)] fn have_permission(dir: impl AsRef) -> PermissionResult<'static> { - use crate::filesystem::util::users; - match dir.as_ref().metadata() { Ok(metadata) => { - use std::os::unix::fs::MetadataExt; - let bits = metadata.mode(); - let has_bit = |bit| bits & bit == bit; + let mode = Mode::from_bits_truncate(metadata.mode() as mode_t); let current_user_uid = users::get_current_uid(); - if current_user_uid == 0 { + if current_user_uid.is_root() { return PermissionResult::PermissionOk; } let current_user_gid = users::get_current_gid(); - let owner_user = metadata.uid(); - let owner_group = metadata.gid(); + let owner_user = Uid::from_raw(metadata.uid()); + let owner_group = Gid::from_raw(metadata.gid()); match ( current_user_uid == owner_user, current_user_gid == owner_group, ) { (true, _) => { - if has_bit(file_permissions::USER_EXECUTE) { + if mode.contains(Mode::S_IXUSR) { PermissionResult::PermissionOk } else { PermissionResult::PermissionDenied( @@ -198,7 +192,7 @@ fn have_permission(dir: impl AsRef) -> PermissionResult<'static> { } } (false, true) => { - if has_bit(file_permissions::GROUP_EXECUTE) { + if mode.contains(Mode::S_IXGRP) { PermissionResult::PermissionOk } else { PermissionResult::PermissionDenied( @@ -207,8 +201,8 @@ fn have_permission(dir: impl AsRef) -> PermissionResult<'static> { } } (false, false) => { - if has_bit(file_permissions::OTHER_EXECUTE) - || (has_bit(file_permissions::GROUP_EXECUTE) + if mode.contains(Mode::S_IXOTH) + || (mode.contains(Mode::S_IXGRP) && any_group(current_user_gid, owner_group)) { PermissionResult::PermissionOk @@ -225,24 +219,19 @@ fn have_permission(dir: impl AsRef) -> PermissionResult<'static> { } #[cfg(any(target_os = "linux", target_os = "freebsd", target_os = "android"))] -fn any_group(_current_user_gid: gid_t, owner_group: u32) -> bool { - use crate::filesystem::util::users; - let Some(user_groups) = users::current_user_groups() else { - return false; - }; - user_groups.iter().any(|gid| gid.as_raw() == owner_group) +fn any_group(_current_user_gid: Gid, owner_group: Gid) -> bool { + users::current_user_groups() + .unwrap_or_default() + .contains(&owner_group) } #[cfg(all( unix, not(any(target_os = "linux", target_os = "freebsd", target_os = "android")) ))] -fn any_group(current_user_gid: gid_t, owner_group: u32) -> bool { - use crate::filesystem::util::users; - +fn any_group(current_user_gid: Gid, owner_group: Gid) -> bool { users::get_current_username() .and_then(|name| users::get_user_groups(&name, current_user_gid)) .unwrap_or_default() - .into_iter() - .any(|gid| gid.as_raw() == owner_group) + .contains(&owner_group) } diff --git a/crates/nu-command/src/filesystem/ls.rs b/crates/nu-command/src/filesystem/ls.rs index 10c1f21747..0def2b6fd0 100644 --- a/crates/nu-command/src/filesystem/ls.rs +++ b/crates/nu-command/src/filesystem/ls.rs @@ -511,6 +511,7 @@ pub(crate) fn dir_entry_dict( { use crate::filesystem::util::users; use std::os::unix::fs::MetadataExt; + let mode = md.permissions().mode(); record.push( "mode", @@ -525,19 +526,19 @@ pub(crate) fn dir_entry_dict( record.push( "user", - if let Some(user) = users::get_user_by_uid(md.uid()) { + if let Some(user) = users::get_user_by_uid(md.uid().into()) { Value::string(user.name, span) } else { - Value::int(md.uid() as i64, span) + Value::int(md.uid().into(), span) }, ); record.push( "group", - if let Some(group) = users::get_group_by_gid(md.gid()) { + if let Some(group) = users::get_group_by_gid(md.gid().into()) { Value::string(group.name, span) } else { - Value::int(md.gid() as i64, span) + Value::int(md.gid().into(), span) }, ); } diff --git a/crates/nu-command/src/filesystem/util.rs b/crates/nu-command/src/filesystem/util.rs index 26cfd93724..4491b8ea6d 100644 --- a/crates/nu-command/src/filesystem/util.rs +++ b/crates/nu-command/src/filesystem/util.rs @@ -92,57 +92,40 @@ pub fn is_older(src: &Path, dst: &Path) -> Option { #[cfg(unix)] pub mod users { - use libc::{gid_t, uid_t}; use nix::unistd::{Gid, Group, Uid, User}; - pub fn get_user_by_uid(uid: uid_t) -> Option { - User::from_uid(Uid::from_raw(uid)).ok().flatten() + pub fn get_user_by_uid(uid: Uid) -> Option { + User::from_uid(uid).ok().flatten() } - pub fn get_group_by_gid(gid: gid_t) -> Option { - Group::from_gid(Gid::from_raw(gid)).ok().flatten() + pub fn get_group_by_gid(gid: Gid) -> Option { + Group::from_gid(gid).ok().flatten() } - pub fn get_current_uid() -> uid_t { - Uid::current().as_raw() + pub fn get_current_uid() -> Uid { + Uid::current() } - pub fn get_current_gid() -> gid_t { - Gid::current().as_raw() + pub fn get_current_gid() -> Gid { + Gid::current() } #[cfg(not(any(target_os = "linux", target_os = "freebsd", target_os = "android")))] pub fn get_current_username() -> Option { - User::from_uid(Uid::current()) - .ok() - .flatten() - .map(|user| user.name) + get_user_by_uid(get_current_uid()).map(|user| user.name) } #[cfg(any(target_os = "linux", target_os = "freebsd", target_os = "android"))] pub fn current_user_groups() -> Option> { - // SAFETY: - // if first arg is 0 then it ignores second argument and returns number of groups present for given user. - let ngroups = unsafe { libc::getgroups(0, core::ptr::null:: as *mut _) }; - let mut buff: Vec = vec![0; ngroups as usize]; - - // SAFETY: - // buff is the size of ngroups and getgroups reads max ngroups elements into buff - let found = unsafe { libc::getgroups(ngroups, buff.as_mut_ptr()) }; - - if found < 0 { - None + if let Ok(mut groups) = nix::unistd::getgroups() { + groups.sort_unstable_by_key(|id| id.as_raw()); + groups.dedup(); + Some(groups) } else { - buff.truncate(found as usize); - buff.sort_unstable(); - buff.dedup(); - buff.into_iter() - .filter_map(|i| get_group_by_gid(i as gid_t)) - .map(|group| group.gid) - .collect::>() - .into() + None } } + /// Returns groups for a provided user name and primary group id. /// /// # libc functions used @@ -159,19 +142,19 @@ pub mod users { /// } /// ``` #[cfg(not(any(target_os = "linux", target_os = "freebsd", target_os = "android")))] - pub fn get_user_groups(username: &str, gid: gid_t) -> Option> { + pub fn get_user_groups(username: &str, gid: Gid) -> Option> { + use nix::libc::{c_int, gid_t}; use std::ffi::CString; + // MacOS uses i32 instead of gid_t in getgrouplist for unknown reasons #[cfg(target_os = "macos")] let mut buff: Vec = vec![0; 1024]; #[cfg(not(target_os = "macos"))] let mut buff: Vec = vec![0; 1024]; - let Ok(name) = CString::new(username.as_bytes()) else { - return None; - }; + let name = CString::new(username).ok()?; - let mut count = buff.len() as libc::c_int; + let mut count = buff.len() as c_int; // MacOS uses i32 instead of gid_t in getgrouplist for unknown reasons // SAFETY: @@ -182,11 +165,19 @@ pub mod users { // The capacity for `*groups` is passed in as `*ngroups` which is the buffer max length/capacity (as we initialize with 0) // Following reads from `*groups`/`buff` will only happen after `buff.truncate(*ngroups)` #[cfg(target_os = "macos")] - let res = - unsafe { libc::getgrouplist(name.as_ptr(), gid as i32, buff.as_mut_ptr(), &mut count) }; + let res = unsafe { + nix::libc::getgrouplist( + name.as_ptr(), + gid.as_raw() as i32, + buff.as_mut_ptr(), + &mut count, + ) + }; #[cfg(not(target_os = "macos"))] - let res = unsafe { libc::getgrouplist(name.as_ptr(), gid, buff.as_mut_ptr(), &mut count) }; + let res = unsafe { + nix::libc::getgrouplist(name.as_ptr(), gid.as_raw(), buff.as_mut_ptr(), &mut count) + }; if res < 0 { None @@ -196,11 +187,13 @@ pub mod users { buff.dedup(); // allow trivial cast: on macos i is i32, on linux it's already gid_t #[allow(trivial_numeric_casts)] - buff.into_iter() - .filter_map(|i| get_group_by_gid(i as gid_t)) - .map(|group| group.gid) - .collect::>() - .into() + Some( + buff.into_iter() + .map(|id| Gid::from_raw(id as gid_t)) + .filter_map(get_group_by_gid) + .map(|group| group.gid) + .collect(), + ) } } } diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index 04504166cf..02996f8423 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -533,44 +533,38 @@ impl ExternalCommand { // Create a thread to wait for an exit code. thread::Builder::new() .name("exit code waiter".into()) - .spawn(move || { - match child.as_mut().wait() { - Err(err) => Err(ShellError::ExternalCommand { - label: "External command exited with error".into(), - help: err.to_string(), - span - }), - Ok(x) => { - #[cfg(unix)] - { - use nu_ansi_term::{Color, Style}; - use std::ffi::CStr; - use std::os::unix::process::ExitStatusExt; + .spawn(move || match child.as_mut().wait() { + Err(err) => Err(ShellError::ExternalCommand { + label: "External command exited with error".into(), + help: err.to_string(), + span, + }), + Ok(x) => { + #[cfg(unix)] + { + use nix::sys::signal::Signal; + use nu_ansi_term::{Color, Style}; + use std::os::unix::process::ExitStatusExt; - if x.core_dumped() { - let cause = x.signal().and_then(|sig| unsafe { - // SAFETY: We should be the first to call `char * strsignal(int sig)` - let sigstr_ptr = libc::strsignal(sig); - if sigstr_ptr.is_null() { - return None; - } - - // SAFETY: The pointer points to a valid non-null string - let sigstr = CStr::from_ptr(sigstr_ptr); - sigstr.to_str().map(String::from).ok() - }); - - let cause = cause.as_deref().unwrap_or("Something went wrong"); + if x.core_dumped() { + let cause = x + .signal() + .and_then(|sig| { + Signal::try_from(sig).ok().map(Signal::as_str) + }) + .unwrap_or("Something went wrong"); let style = Style::new().bold().on(Color::Red); - eprintln!( - "{}", - style.paint(format!( - "{cause}: oops, process '{commandname}' core dumped" - )) + let message = format!( + "{cause}: child process '{commandname}' core dumped" ); - let _ = exit_code_tx.send(Value::error ( - ShellError::ExternalCommand { label: "core dumped".to_string(), help: format!("{cause}: child process '{commandname}' core dumped"), span: head }, + eprintln!("{}", style.paint(&message)); + let _ = exit_code_tx.send(Value::error( + ShellError::ExternalCommand { + label: "core dumped".into(), + help: message, + span: head, + }, head, )); return Ok(()); @@ -585,8 +579,8 @@ impl ExternalCommand { } Ok(()) } - } - }).map_err(|e| e.into_spanned(head))?; + }) + .map_err(|e| e.into_spanned(head))?; let exit_code_receiver = ValueReceiver::new(exit_code_rx); diff --git a/crates/nu-system/src/foreground.rs b/crates/nu-system/src/foreground.rs index 56eec154cd..f7d675461e 100644 --- a/crates/nu-system/src/foreground.rs +++ b/crates/nu-system/src/foreground.rs @@ -120,7 +120,7 @@ mod foreground_pgroup { /// Currently only intended to access `tcsetpgrp` and `tcgetpgrp` with the I/O safe `nix` /// interface. pub unsafe fn stdin_fd() -> impl AsFd { - unsafe { BorrowedFd::borrow_raw(libc::STDIN_FILENO) } + unsafe { BorrowedFd::borrow_raw(nix::libc::STDIN_FILENO) } } pub fn prepare_command(external_command: &mut Command, existing_pgrp: u32) {