Skip to content

Commit

Permalink
Change pointer in Bypass variant to PathBuf to fix dangling pointer bug
Browse files Browse the repository at this point in the history
  • Loading branch information
gememma committed Jun 6, 2024
1 parent 677f1d2 commit 5bbce18
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 14 deletions.
7 changes: 1 addition & 6 deletions mirrord/layer/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,7 @@ impl CheckedInto<PathBuf> for *const c_char {
// `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(
// TODO: dangling pointer bug - old code relied on fact that string was owned
// elsewhere now the result of to_string_lossy is
// short-lived
stripped_path.as_os_str().as_encoded_bytes().as_ptr() as _,
))
Detour::Bypass(Bypass::FileOperationInMirrordBinTempDir(stripped_path))
} else {
Detour::Success(path_str) // strip is None, path not in temp dir.
}
Expand Down
5 changes: 1 addition & 4 deletions mirrord/layer/src/detour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,6 @@ use core::{
};
use std::{cell::RefCell, ops::Deref, os::unix::prelude::*, path::PathBuf, sync::OnceLock};

#[cfg(target_os = "macos")]
use libc::c_char;

use crate::error::HookError;

thread_local!(
Expand Down Expand Up @@ -147,7 +144,7 @@ pub(crate) enum Bypass {
/// 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.
#[cfg(target_os = "macos")]
FileOperationInMirrordBinTempDir(*const c_char),
FileOperationInMirrordBinTempDir(PathBuf),

/// File [`PathBuf`] should be ignored (used for tests).
IgnoredFile(PathBuf),
Expand Down
7 changes: 4 additions & 3 deletions mirrord/layer/src/exec_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,8 @@ 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(path_buf)) = path_buf_detour {
let later_ptr: *const i8 = path_buf.as_os_str().as_encoded_bytes().as_ptr() as _;
// 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;
Expand Down Expand Up @@ -325,9 +326,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(path_buf)) = detour {
trace!("dlopen called with a path inside our patch dir, switching with fixed pointer.");
ptr
path_buf.as_os_str().as_encoded_bytes().as_ptr() as _
} else {
trace!("dlopen called on path {detour:?}.");
raw_path
Expand Down
4 changes: 3 additions & 1 deletion mirrord/layer/src/file/hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ fn update_ptr_from_bypass(ptr: *const c_char, bypass: Bypass) -> *const c_char {
// 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.
Bypass::FileOperationInMirrordBinTempDir(stripped_ptr) => stripped_ptr,
Bypass::FileOperationInMirrordBinTempDir(path_buf) => {
path_buf.as_os_str().as_encoded_bytes().as_ptr() as _
}
_ => ptr,
}
}
Expand Down

0 comments on commit 5bbce18

Please sign in to comment.