chore: rebase + adapt previous changes

reubeno · reubeno · commit 62bd2e7ed196 · 2025-03-10T07:39:12.000-07:00
Adjusts previous changes to address earlier PR feedback; adds tests.
diff --git a/brush-core/src/builtins/command.rs b/brush-core/src/builtins/command.rs
@@ -1,12 +1,7 @@
 use clap::Parser;
 use std::{fmt::Display, io::Write, path::Path};
 
-use crate::{builtins, commands, error, shell, sys::fs::PathExt, ExecutionResult};
-
-/// The value for PATH when invoking `command -p`. This is only used when
-/// the Posix.2 `confstr()` returns nothing
-/// The value of this variable is taken from the BASH source code.
-const STANDARD_UTILS_PATH: &[&str] = &["/bin", "/usr/bin", "/sbin", "/usr/sbin", "/etc:/usr/etc"];
+use crate::{builtins, commands, error, shell, sys, sys::fs::PathExt, ExecutionResult};
 
 /// Directly invokes an external command, without going through typical search order.
 #[derive(Parser)]
@@ -67,7 +62,8 @@ impl builtins::Command for CommandCommand {
                     Ok(builtins::ExitCode::Custom(1))
                 }
             } else {
-                self.execute_command(context, command_name).await
+                self.execute_command(context, command_name, self.use_default_path)
+                    .await
             }
         } else {
             Ok(builtins::ExitCode::Success)
@@ -118,34 +114,34 @@ impl CommandCommand {
             }
 
             if use_default_path {
-                let path = confstr_path();
-                // Without an allocation if possible.
-                let path = path.as_ref().map(|p| String::from_utf8_lossy(p));
-                let path = path.as_ref().map_or(
-                    itertools::Either::Right(STANDARD_UTILS_PATH.iter().copied()),
-                    |p| itertools::Either::Left(p.split(':')),
-                );
-
-                return shell
-                    .find_executables_in(path, command_name)
+                let dirs = sys::fs::get_default_standard_utils_paths();
+                shell
+                    .find_executables_in(dirs.iter(), command_name)
                     .first()
-                    .map(|path| FoundCommand::External(path.to_string_lossy().to_string()));
+                    .map(|path| FoundCommand::External(path.to_string_lossy().to_string()))
+            } else {
+                shell
+                    .find_first_executable_in_path_using_cache(command_name)
+                    .map(|path| FoundCommand::External(path.to_string_lossy().to_string()))
             }
-
-            shell
-                .find_first_executable_in_path_using_cache(command_name)
-                .map(|path| FoundCommand::External(path.to_string_lossy().to_string()))
         }
     }
 
     async fn execute_command(
         &self,
         mut context: commands::ExecutionContext<'_>,
         command_name: &str,
+        use_default_path: bool,
     ) -> Result<builtins::ExitCode, error::Error> {
         command_name.clone_into(&mut context.command_name);
         let command_and_args = self.command_and_args.iter().map(|arg| arg.into()).collect();
 
+        let path_dirs = if use_default_path {
+            Some(sys::fs::get_default_standard_utils_paths())
+        } else {
+            None
+        };
+
         // We do not have an existing process group to place this into.
         let mut pgid = None;
 
@@ -156,6 +152,7 @@ impl CommandCommand {
             &mut pgid,
             command_and_args,
             false, /* use functions? */
+            path_dirs,
         )
         .await?
         {
@@ -177,38 +174,3 @@ impl CommandCommand {
         }
     }
 }
-
-/// A wrapper for [`nix::libc::confstr`]. Returns a value for the default PATH variable which
-/// indicates where all the POSIX.2 standard utilities can be found.
-fn confstr_path() -> Option<Vec<u8>> {
-    #[cfg(unix)]
-    {
-        let required_size =
-            unsafe { nix::libc::confstr(nix::libc::_CS_PATH, std::ptr::null_mut(), 0) };
-        if required_size == 0 {
-            return None;
-        }
-        // NOTE: Writing `c_char` (i8 or u8 depending on the platform) into `Vec<u8>` is fine,
-        // as i8 and u8 have compatible representations,
-        // and Rust does not support platforms where `c_char` is not 8-bit wide.
-        let mut buffer = Vec::<u8>::with_capacity(required_size);
-        let final_size = unsafe {
-            nix::libc::confstr(
-                nix::libc::_CS_PATH,
-                buffer.as_mut_ptr().cast(),
-                required_size,
-            )
-        };
-        if final_size == 0 {
-            return None;
-        }
-        // ERANGE
-        if final_size > required_size {
-            return None;
-        }
-        unsafe { buffer.set_len(final_size - 1) }; // The last byte is a null terminator.
-        return Some(buffer);
-    }
-    #[allow(unreachable_code)]
-    None
-}
diff --git a/brush-core/src/commands.rs b/brush-core/src/commands.rs
@@ -273,6 +273,7 @@ pub(crate) async fn execute(
     process_group_id: &mut Option<i32>,
     args: Vec<CommandArg>,
     use_functions: bool,
+    path_dirs: Option<Vec<String>>,
 ) -> Result<CommandSpawnResult, error::Error> {
     if !cmd_context.command_name.contains(std::path::MAIN_SEPARATOR) {
         let builtin = cmd_context
@@ -307,10 +308,19 @@ pub(crate) async fn execute(
             }
         }
 
-        if let Some(path) = cmd_context
-            .shell
-            .find_first_executable_in_path_using_cache(&cmd_context.command_name)
-        {
+        let path = if let Some(path_dirs) = path_dirs {
+            cmd_context
+                .shell
+                .find_executables_in(path_dirs.iter(), &cmd_context.command_name)
+                .first()
+                .cloned()
+        } else {
+            cmd_context
+                .shell
+                .find_first_executable_in_path_using_cache(&cmd_context.command_name)
+        };
+
+        if let Some(path) = path {
             let resolved_path = path.to_string_lossy();
             execute_external_command(
                 cmd_context,
diff --git a/brush-core/src/interp.rs b/brush-core/src/interp.rs
@@ -1116,6 +1116,7 @@ impl ExecuteInPipeline for ast::SimpleCommand {
                 &mut context.process_group_id,
                 args,
                 true, /* use functions? */
+                None,
             )
             .await;
 
diff --git a/brush-core/src/shell.rs b/brush-core/src/shell.rs
@@ -540,12 +540,9 @@ impl Shell {
         // PATH (if not already set)
         #[cfg(unix)]
         if !self.env.is_set("PATH") {
-            self.env.set_global(
-                "PATH",
-                ShellVariable::new(
-                    "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin".into(),
-                ),
-            )?;
+            let default_path_str = sys::fs::get_default_executable_search_paths().join(":");
+            self.env
+                .set_global("PATH", ShellVariable::new(default_path_str.into()))?;
         }
 
         // PIPESTATUS
@@ -1316,10 +1313,8 @@ impl Shell {
         let is_executable = |path: &Path| path.is_file() && path.executable();
 
         let mut executables = vec![];
-
         for dir_str in paths {
             let dir_str = dir_str.as_ref();
-
             let pattern =
                 patterns::Pattern::from(std::format!("{dir_str}/{required_glob_pattern}"))
                     .set_extended_globbing(self.options.extended_globbing)
diff --git a/brush-core/src/sys/stubs/fs.rs b/brush-core/src/sys/stubs/fs.rs
@@ -52,3 +52,11 @@ pub(crate) trait StubMetadataExt {
 }
 
 impl StubMetadataExt for std::fs::Metadata {}
+
+pub(crate) fn get_default_executable_search_paths() -> Vec<String> {
+    vec![]
+}
+
+pub(crate) fn get_default_standard_utils_paths() -> Vec<String> {
+    vec![]
+}
diff --git a/brush-core/src/sys/unix/fs.rs b/brush-core/src/sys/unix/fs.rs
@@ -1,6 +1,19 @@
+use std::os::unix::ffi::OsStringExt;
 use std::os::unix::fs::{FileTypeExt, MetadataExt};
 use std::path::Path;
 
+const DEFAULT_EXECUTABLE_SEARCH_PATHS: &[&str] = &[
+    "/usr/local/sbin",
+    "/usr/local/bin",
+    "/usr/sbin",
+    "/usr/bin",
+    "/sbin",
+    "/bin",
+];
+
+const DEFAULT_STANDARD_UTILS_PATHS: &[&str] =
+    &["/bin", "/usr/bin", "/sbin", "/usr/sbin", "/etc", "/usr/etc"];
+
 impl crate::sys::fs::PathExt for Path {
     fn readable(&self) -> bool {
         nix::unistd::access(self, nix::unistd::AccessFlags::R_OK).is_ok()
@@ -56,3 +69,78 @@ fn try_get_file_type(path: &Path) -> Option<std::fs::FileType> {
 fn try_get_file_mode(path: &Path) -> Option<u32> {
     path.metadata().map(|metadata| metadata.mode()).ok()
 }
+
+pub(crate) fn get_default_executable_search_paths() -> Vec<String> {
+    DEFAULT_EXECUTABLE_SEARCH_PATHS
+        .iter()
+        .map(|s| (*s).to_owned())
+        .collect()
+}
+
+/// Retrieves the platform-specific set of paths that should contain standard system
+/// utilities. Used by `command -p`, for example.
+pub(crate) fn get_default_standard_utils_paths() -> Vec<String> {
+    //
+    // Try to call confstr(_CS_PATH). If that fails, can't find a string value, or
+    // finds an empty string, then we'll fall back to hard-coded defaults.
+    //
+
+    if let Ok(Some(cs_path)) = confstr_cs_path() {
+        if !cs_path.is_empty() {
+            return cs_path.split(':').map(|s| s.to_string()).collect();
+        }
+    }
+
+    DEFAULT_STANDARD_UTILS_PATHS
+        .iter()
+        .map(|s| (*s).to_owned())
+        .collect()
+}
+
+fn confstr_cs_path() -> Result<Option<String>, std::io::Error> {
+    let value = confstr(nix::libc::_CS_PATH)?;
+
+    if let Some(value) = value {
+        let value_str = value
+            .into_string()
+            .map_err(|_err| std::io::Error::new(std::io::ErrorKind::InvalidData, "Invalid data"))?;
+        Ok(Some(value_str))
+    } else {
+        Ok(None)
+    }
+}
+
+/// A wrapper for [`nix::libc::confstr`]. Returns a value for the default PATH variable which
+/// indicates where all the POSIX.2 standard utilities can be found.
+///
+/// N.B. We would strongly prefer to use a safe API exposed (in an idiomatic way) by nix
+/// or similar. Until that exists, we accept the need to make the unsafe call directly.
+fn confstr(name: nix::libc::c_int) -> Result<Option<std::ffi::OsString>, std::io::Error> {
+    let required_size = unsafe { nix::libc::confstr(name, std::ptr::null_mut(), 0) };
+
+    // When confstr returns 0, it either means there's no value associated with _CS_PATH, or
+    // _CS_PATH is considered invalid (and not present) on this platform. In both cases, we
+    // treat it as a non-existent value and return None.
+    if required_size == 0 {
+        return Ok(None);
+    }
+
+    let mut buffer = Vec::<u8>::with_capacity(required_size);
+
+    // NOTE: Writing `c_char` (i8 or u8 depending on the platform) into `Vec<u8>` is fine,
+    // as i8 and u8 have compatible representations,
+    // and Rust does not support platforms where `c_char` is not 8-bit wide.
+    let final_size =
+        unsafe { nix::libc::confstr(name, buffer.as_mut_ptr().cast(), buffer.capacity()) };
+
+    if final_size == 0 {
+        return Err(std::io::Error::last_os_error());
+    }
+
+    unsafe { buffer.set_len(final_size) };
+
+    // The last byte is a null terminator.
+    buffer.pop();
+
+    Ok(Some(std::ffi::OsString::from_vec(buffer)))
+}
diff --git a/brush-shell/tests/cases/builtins/command.yaml b/brush-shell/tests/cases/builtins/command.yaml
@@ -31,6 +31,16 @@ cases:
       echo "[/usr/bin/non-existent]"
       command -v /usr/bin/non-existent || echo "2. Not found"
 
+  - name: "command -v -p"
+    stdin: |
+      unset PATH
+
+      echo "[no -p]"
+      command -v cat
+
+      echo "[-p]"
+      command -v -p cat
+
   - name: "command -v with full paths"
     skip: true # TODO: investigate why this fails on arch linux
     stdin: |
@@ -57,3 +67,9 @@ cases:
   - name: "command with --help"
     stdin: |
       command ls --help
+
+  - name: "command with -p"
+    stdin: |
+      unset PATH
+
+      command -p -- ls

Original file line number	Diff line number	Diff line change
`@@ -1116,6 +1116,7 @@ impl ExecuteInPipeline for ast::SimpleCommand {`
`1116`	`1116`	`&mut context.process_group_id,`
`1117`	`1117`	`args,`
`1118`	`1118`	`true, /* use functions? */`
	`1119`	`+ None,`
`1119`	`1120`	`)`
`1120`	`1121`	`.await;`
`1121`	`1122`