Change Strings in SIP crate to PathBufs/OsStrings

changelog.d/2198.internal.md

@@ -0,0 +1 @@
+Remove Strings from SIP crate where types should be OsString or PathBuf.

mirrord/cli/src/execution.rs

@@ -1,6 +1,8 @@
+#[cfg(target_os = "macos")]
 use std::{
     collections::{HashMap, HashSet},
     net::SocketAddr,
+    path::Path,
     time::Duration,
 };
 
@@ -272,17 +274,19 @@ impl MirrordExecution {
         let patched_path = executable
             .and_then(|exe| {
                 sip_patch(
-                    exe,
-                    &config
+                    Path::new(exe),
+                    config
                         .sip_binaries
                         .clone()
-                        .map(|x| x.to_vec())
-                        .unwrap_or_default(),
+                        .map(|x| x.to_vec().iter().map(|y| y.into()).collect::<Vec<_>>())
+                        .unwrap_or_default()
+                        .as_ref(),
                 )
                 .transpose() // We transpose twice to propagate a possible error out of this
                              // closure.
             })
-            .transpose()?;
+            .transpose()?
+            .map(|x| x.to_string_lossy().to_string());
 
         #[cfg(not(target_os = "macos"))]
         let patched_path = None;

mirrord/layer/src/common.rs

@@ -1,9 +1,13 @@
 //! Shared place for a few types and functions that are used everywhere by the layer.
+#[cfg(target_os = "macos")]
+use std::path::Path;
 use std::{ffi::CStr, fmt::Debug, ops::Not, path::PathBuf};
 
 use libc::c_char;
 use mirrord_intproxy_protocol::{IsLayerRequest, IsLayerRequestWithResponse, MessageId};
 use mirrord_protocol::file::OpenOptionsInternal;
+#[cfg(target_os = "macos")]
+use mirrord_sip::{MIRRORD_TEMP_BIN_DIR_CANONIC_PATHBUF, MIRRORD_TEMP_BIN_DIR_PATH_BUF};
 use null_terminated::Nul;
 use tracing::warn;
 
@@ -84,15 +88,15 @@ impl CheckedInto<String> for *const c_char {
 }
 
 #[cfg(target_os = "macos")]
-pub fn strip_mirrord_path(path_str: &str) -> Option<&str> {
-    use mirrord_sip::MIRRORD_PATCH_DIR;
-
-    // SAFETY: We only slice after we find the string in the path
-    // so it must be valid
-    #[allow(clippy::indexing_slicing)]
+pub fn strip_mirrord_path(path_str: &Path) -> Option<&Path> {
     path_str
-        .find(MIRRORD_PATCH_DIR)
-        .map(|index| &path_str[(MIRRORD_PATCH_DIR.len() + index)..])
+        .strip_prefix(MIRRORD_TEMP_BIN_DIR_PATH_BUF.to_owned())
+        .ok()
+        .or_else(|| {
+            path_str
+                .strip_prefix(MIRRORD_TEMP_BIN_DIR_CANONIC_PATHBUF.to_owned())
+                .ok()
+        })
 }
 
 impl CheckedInto<PathBuf> for *const c_char {
@@ -102,15 +106,18 @@ impl CheckedInto<PathBuf> for *const c_char {
         let str_det = CheckedInto::<&str>::checked_into(self);
         #[cfg(target_os = "macos")]
         let str_det = str_det.and_then(|path_str| {
-            let optional_stripped_path = strip_mirrord_path(path_str);
+            let optional_stripped_path =
+                strip_mirrord_path(Path::new(path_str)).map(|x| Path::new("/").join(x));
             if let Some(stripped_path) = optional_stripped_path {
                 // actually stripped, so bypass and provide a pointer to after the temp dir.
                 // `stripped_path` is a reference to a later character in the same string as
                 // `path_str`, `stripped_path.as_ptr()` returns a pointer to a later index
                 // in the same string owned by the caller (the hooked program).
-                Detour::Bypass(Bypass::FileOperationInMirrordBinTempDir(
-                    stripped_path.as_ptr() as _,
-                ))
+                let path_len = stripped_path.to_string_lossy().len();
+                let prefix_len = path_str.len() - path_len;
+                Detour::Bypass(Bypass::FileOperationInMirrordBinTempDir((
+                    prefix_len, path_len,
+                )))
             } else {
                 Detour::Success(path_str) // strip is None, path not in temp dir.
             }

mirrord/layer/src/detour.rs

@@ -13,9 +13,6 @@ use std::{
     cell::RefCell, ffi::CString, ops::Deref, os::unix::prelude::*, path::PathBuf, sync::OnceLock,
 };
 
-#[cfg(target_os = "macos")]
-use libc::c_char;
-
 use crate::error::HookError;
 
 thread_local!(
@@ -147,9 +144,11 @@ pub(crate) enum Bypass {
     CStrConversion,
 
     /// We hooked a file operation on a path in mirrord's bin directory. So do the operation
-    /// locally, but on the original path, not the one in mirrord's dir.
+    /// locally, but on the original path, not the one in mirrord's dir. The offset from the full
+    /// path to the original path is needed for pointer arithmetic, as well as the length of the
+    /// stripped path.
     #[cfg(target_os = "macos")]
-    FileOperationInMirrordBinTempDir(*const c_char),
+    FileOperationInMirrordBinTempDir((usize, usize)),
 
     /// File [`PathBuf`] should be ignored (used for tests).
     IgnoredFile(CString),

mirrord/layer/src/exec_utils.rs

@@ -2,13 +2,15 @@
 use std::{
     env,
     ffi::{c_void, CStr, CString},
-    path::PathBuf,
+    path::{Path, PathBuf},
     sync::OnceLock,
 };
 
 use libc::{c_char, c_int, pid_t};
 use mirrord_layer_macro::{hook_fn, hook_guard_fn};
-use mirrord_sip::{sip_patch, SipError, MIRRORD_PATCH_DIR};
+use mirrord_sip::{
+    sip_patch, SipError, MIRRORD_TEMP_BIN_DIR_CANONIC_PATHBUF, MIRRORD_TEMP_BIN_DIR_PATH_BUF,
+};
 use null_terminated::Nul;
 use tracing::{trace, warn};
 
@@ -61,19 +63,26 @@ pub(crate) unsafe fn enable_macos_hooks(
 /// Check if the file that is to be executed has SIP and patch it if it does.
 #[mirrord_layer_macro::instrument(level = "trace")]
 pub(super) fn patch_if_sip(path: &str) -> Detour<String> {
-    let patch_binaries = PATCH_BINARIES.get().expect("patch binaries not set");
-    match sip_patch(path, patch_binaries) {
+    let patch_binaries: Vec<_> = PATCH_BINARIES
+        .get()
+        .expect("patch binaries not set")
+        .iter()
+        .map(|y| y.into())
+        .collect();
+    match sip_patch(Path::new(path), patch_binaries.as_ref()) {
         Ok(None) => Bypass(NoSipDetected(path.to_string())),
-        Ok(Some(new_path)) => Success(new_path),
+        Ok(Some(new_path)) => Success(new_path.to_string_lossy().to_string()),
         Err(SipError::FileNotFound(non_existing_bin)) => {
             trace!(
-                "The application wants to execute {}, SIP check got FileNotFound for {}. \
+                "The application wants to execute {}, SIP check got FileNotFound for {:?}. \
                 If the file actually exists and should have been found, make sure it is excluded \
                 from FS ops.",
                 path,
                 non_existing_bin
             );
-            Bypass(ExecOnNonExistingFile(non_existing_bin))
+            Bypass(ExecOnNonExistingFile(
+                non_existing_bin.to_string_lossy().to_string(),
+            ))
         }
         Err(sip_error) => {
             warn!(
@@ -107,27 +116,27 @@ fn intercept_tmp_dir(argv_arr: &Nul<*const c_char>) -> Detour<Argv> {
         }
         let arg_str: &str = arg.checked_into()?;
         trace!("exec arg: {arg_str}");
+        let arg_path = Path::new(arg_str);
 
-        // SAFETY: We only slice after we find the string in the path
-        // so it must be valid
-        #[allow(clippy::indexing_slicing)]
-        let stripped = arg_str
-            .find(MIRRORD_PATCH_DIR)
-            .map(|index| &arg_str[(MIRRORD_PATCH_DIR.len() + index)..])
+        let leading_slash = Path::new("/");
+        let stripped: PathBuf = arg_path
+            .strip_prefix(MIRRORD_TEMP_BIN_DIR_PATH_BUF.to_owned()).map(|x| leading_slash.join(x))
+            // If /var/folders... not a prefix, check /private/var/folers...
+            .or(arg_path.strip_prefix(MIRRORD_TEMP_BIN_DIR_CANONIC_PATHBUF.to_owned()).map(|x| leading_slash.join(x)))
             .inspect(|original_path| {
                 trace!(
-                    "Intercepted mirrord's temp dir in argv: {}. Replacing with original path: {}.",
+                    "Intercepted mirrord's temp dir in argv: {}. Replacing with original path: {:?}.",
                     arg_str,
                     original_path
                 );
             })
-            .unwrap_or(arg_str) // No temp-dir prefix found, use arg as is.
-            // As the string slice we get here is a slice of memory allocated and managed by
+            .unwrap_or(arg_path.to_owned()) // No temp-dir prefix found, use arg as is.
+            // As the path we get here is a slice of memory allocated and managed by
             // the user app, we copy the data and create new CStrings out of the copy
             // without consuming the original data.
             .to_owned();
 
-        c_string_vec.push(CString::new(stripped)?)
+        c_string_vec.push(CString::new(stripped.to_string_lossy().to_string())?);
     }
     Success(c_string_vec)
 }
@@ -174,16 +183,18 @@ pub(crate) unsafe fn patch_sip_for_new_process(
         .unwrap_or_default();
     trace!("Executable {} called execve/posix_spawn", calling_exe);
 
-    let path_str = path.checked_into()?;
+    let mut path_str: String = path.checked_into()?;
     // If an application is trying to run an executable from our tmp dir, strip our tmp dir from the
     // path. The file might not even exist in our tmp dir, and the application is expecting it there
     // only because it somehow found out about its own patched location in our tmp dir.
     // If original path is SIP, and actually exists in our dir that patched executable will be used.
-    let path_str = strip_mirrord_path(path_str).unwrap_or(path_str);
-    let path_c_string = patch_if_sip(path_str)
+    if let Some(s) = strip_mirrord_path(Path::new(&path_str)).map(|x| Path::new("/").join(x)) {
+        path_str = s.to_string_lossy().to_string();
+    };
+    let path_c_string = patch_if_sip(&path_str)
         .and_then(|new_path| Success(CString::new(new_path)?))
         // Continue also on error, use original path, don't bypass yet, try cleaning argv.
-        .unwrap_or(CString::new(path_str.to_string())?);
+        .unwrap_or(CString::new(path_str)?);
 
     let argv_arr = Nul::new_unchecked(argv);
     let envp_arr = Nul::new_unchecked(envp);
@@ -238,15 +249,14 @@ pub(crate) unsafe extern "C" fn _nsget_executable_path_detour(
     let res = FN__NSGET_EXECUTABLE_PATH(path, buflen);
     if res == 0 {
         let path_buf_detour = CheckedInto::<PathBuf>::checked_into(path as *const c_char);
-        if let Bypass(FileOperationInMirrordBinTempDir(later_ptr)) = path_buf_detour {
+        if let Bypass(FileOperationInMirrordBinTempDir((prefix_len, stripped_len))) =
+            path_buf_detour
+        {
             // SAFETY: If we're here, the original function was passed this pointer and was
             //         successful, so this pointer must be valid.
-            let old_len = *buflen;
 
             // SAFETY:  `later_ptr` is a pointer to a later char in the same buffer.
-            let prefix_len = later_ptr.offset_from(path);
-
-            let stripped_len = old_len - prefix_len as u32;
+            let later_ptr = path.wrapping_add(prefix_len);
 
             // SAFETY:
             // - can read `stripped_len` bytes from `path_cstring` because it's its length.
@@ -258,7 +268,7 @@ pub(crate) unsafe extern "C" fn _nsget_executable_path_detour(
             // SAFETY:
             // - We call the original function before this, so if it's not a valid pointer we should
             //   not get back 0, and then this code is not executed.
-            *buflen = stripped_len;
+            *buflen = stripped_len as u32;
 
             // If the buffer is long enough for the path, it is long enough for the stripped
             // path.
@@ -281,9 +291,9 @@ pub(crate) unsafe extern "C" fn dlopen_detour(
     // we hold the guard manually for tracing/internal code
     let guard = crate::detour::DetourGuard::new();
     let detour: Detour<PathBuf> = raw_path.checked_into();
-    let raw_path = if let Bypass(FileOperationInMirrordBinTempDir(ptr)) = detour {
+    let raw_path = if let Bypass(FileOperationInMirrordBinTempDir((prefix_len, _))) = detour {
         trace!("dlopen called with a path inside our patch dir, switching with fixed pointer.");
-        ptr
+        raw_path.wrapping_add(prefix_len)
     } else {
         trace!("dlopen called on path {detour:?}.");
         raw_path

mirrord/layer/src/file/hooks.rs

@@ -59,8 +59,10 @@
         // inside mirrord's temp bin dir. The detour has returned us the original path of the file
         // (stripped mirrord's dir path), so now we carry out the operation locally, on the stripped
         // path.
-        #[cfg(target_os = "macos")]
-        Bypass::FileOperationInMirrordBinTempDir(stripped_ptr) => *stripped_ptr,
+        Bypass::FileOperationInMirrordBinTempDir((prefix_len, _)) => {
+            // add length of path to strip to ptr to obtain new pointer
+            ptr.wrapping_add(*prefix_len)
+        }
         Bypass::RelativePath(path) | Bypass::IgnoredFile(path) => path.as_ptr(),
         _ => ptr,
     }

mirrord/layer/src/lib.rs

@@ -75,6 +75,8 @@ use std::{
     sync::OnceLock,
     time::Duration,
 };
+#[cfg(target_os = "macos")]
+use std::{ffi::OsString, path::Path};
 
 use ctor::ctor;
 use error::{LayerError, Result};
@@ -176,10 +178,10 @@ fn layer_pre_initialization() -> Result<(), LayerError> {
     let config = LayerConfig::from_env()?;
 
     #[cfg(target_os = "macos")]
-    let patch_binaries = config
+    let patch_binaries: Vec<OsString> = config
         .sip_binaries
         .clone()
-        .map(|x| x.to_vec())
+        .map(|x| x.to_vec().iter().map(|y| y.into()).collect())
         .unwrap_or_default();
 
     // SIP Patch the process' binary then re-execute it. Needed
@@ -189,7 +191,7 @@ fn layer_pre_initialization() -> Result<(), LayerError> {
         let path = EXECUTABLE_PATH
             .get()
             .expect("EXECUTABLE_PATH needs to be set!");
-        if let Ok(Some(binary)) = mirrord_sip::sip_patch(path, &patch_binaries) {
+        if let Ok(Some(binary)) = mirrord_sip::sip_patch(Path::new(path), &patch_binaries) {
             let err = exec::execvp(
                 binary,
                 EXECUTABLE_ARGS
@@ -206,7 +208,13 @@ fn layer_pre_initialization() -> Result<(), LayerError> {
     match given_process.load_type(&config) {
         LoadType::Full => layer_start(config),
         #[cfg(target_os = "macos")]
-        LoadType::SIPOnly => sip_only_layer_start(config, patch_binaries),
+        LoadType::SIPOnly => sip_only_layer_start(
+            config,
+            patch_binaries
+                .iter()
+                .map(|x| x.to_string_lossy().to_string())
+                .collect(),
+        ),
         LoadType::Skip => load_only_layer_start(&config),
     }
 

mirrord/layer/tests/bash.rs

@@ -1,7 +1,10 @@
 #![feature(assert_matches)]
 #![warn(clippy::indexing_slicing)]
 
-use std::{path::Path, time::Duration};
+use std::{
+    path::{Path, PathBuf},
+    time::Duration,
+};
 
 #[cfg(not(target_os = "macos"))]
 use mirrord_protocol::{
@@ -30,8 +33,8 @@ async fn bash_script(dylib_path: &Path, config_dir: &Path) {
     // to ignore those paths.
     config_path.push("bash_script.json");
     let application = Application::EnvBashCat;
-    let executable = application.get_executable().await; // Own it.
-    println!("Using executable: {}", &executable);
+    let executable = PathBuf::from(application.get_executable().await.to_owned()); // Own it.
+    println!("Using executable: {:?}", &executable);
     let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
     let addr = listener.local_addr().unwrap().to_string();
     println!("Listening for messages from the layer on {addr}");
@@ -43,7 +46,12 @@ async fn bash_script(dylib_path: &Path, config_dir: &Path) {
     );
     #[cfg(target_os = "macos")]
     let executable = sip_patch(&executable, &Vec::new()).unwrap().unwrap();
-    let test_process = TestProcess::start_process(executable, application.get_args(), env).await;
+    let test_process = TestProcess::start_process(
+        executable.to_string_lossy().to_string(),
+        application.get_args(),
+        env,
+    )
+    .await;
 
     let mut intproxy = TestIntProxy::new(listener).await;