diff --git a/changelog.d/2614.fixed.md b/changelog.d/2614.fixed.md new file mode 100644 index 00000000000..24e8259bbf5 --- /dev/null +++ b/changelog.d/2614.fixed.md @@ -0,0 +1 @@ +Fixed a bug where file mode was ignored when Go applications were creating local files. \ No newline at end of file diff --git a/mirrord/layer/src/file/hooks.rs b/mirrord/layer/src/file/hooks.rs index 0867109fb9d..30217589d08 100644 --- a/mirrord/layer/src/file/hooks.rs +++ b/mirrord/layer/src/file/hooks.rs @@ -303,18 +303,26 @@ pub(crate) unsafe extern "C" fn dirfd_detour(dirp: *mut DIR) -> c_int { /// If `fd == AT_FDCWD`, the current working directory is used, and the behavior is the same as /// `open_detour`. /// `fd` for a file descriptor with the `O_DIRECTORY` flag. -#[hook_guard_fn] +#[hook_fn] pub(crate) unsafe extern "C" fn openat_detour( fd: RawFd, raw_path: *const c_char, open_flags: c_int, + mut args: ... ) -> RawFd { - let open_options = OpenOptionsInternalExt::from_flags(open_flags); + let mode: c_int = args.arg(); - openat(fd, raw_path.checked_into(), open_options).unwrap_or_bypass_with(|bypass| { - let raw_path = update_ptr_from_bypass(raw_path, &bypass); - FN_OPENAT(fd, raw_path, open_flags) - }) + let guard = DetourGuard::new(); + if guard.is_none() { + FN_OPENAT(fd, raw_path, open_flags, mode) + } else { + let open_options = OpenOptionsInternalExt::from_flags(open_flags); + + openat(fd, raw_path.checked_into(), open_options).unwrap_or_bypass_with(|bypass| { + let raw_path = update_ptr_from_bypass(raw_path, &bypass); + FN_OPENAT(fd, raw_path, open_flags, mode) + }) + } } /// Equivalent to `open_detour`, **except** when `raw_path` specifies a relative path. diff --git a/mirrord/layer/src/go/mod.rs b/mirrord/layer/src/go/mod.rs index e329431f420..63d468c3f6b 100644 --- a/mirrord/layer/src/go/mod.rs +++ b/mirrord/layer/src/go/mod.rs @@ -103,7 +103,10 @@ unsafe extern "C" fn c_abi_syscall6_handler( libc::SYS_fstat => fstat_detour(param1 as _, param2 as _) as i64, libc::SYS_fsync => fsync_detour(param1 as _) as i64, libc::SYS_fdatasync => fsync_detour(param1 as _) as i64, - libc::SYS_openat => openat_detour(param1 as _, param2 as _, param3 as _) as i64, + libc::SYS_openat => { + openat_detour(param1 as _, param2 as _, param3 as _, param4 as libc::c_int) + as i64 + } libc::SYS_getdents64 => { getdents64_detour(param1 as _, param2 as _, param3 as _) as i64 } diff --git a/mirrord/layer/tests/apps/open_go/go.mod b/mirrord/layer/tests/apps/open_go/go.mod new file mode 100644 index 00000000000..9a8c942d54a --- /dev/null +++ b/mirrord/layer/tests/apps/open_go/go.mod @@ -0,0 +1,3 @@ +module open_go + +go 1.23 diff --git a/mirrord/layer/tests/apps/open_go/main.go b/mirrord/layer/tests/apps/open_go/main.go new file mode 100644 index 00000000000..effce4323aa --- /dev/null +++ b/mirrord/layer/tests/apps/open_go/main.go @@ -0,0 +1,25 @@ +package main + +import ( + "flag" + "io/fs" + _ "net" // to assert dynamic linking + "os" +) + +func main() { + var path string + var flags int + var mode uint + + flag.StringVar(&path, "p", "/tmp/testfile", "Path to the file") + flag.IntVar(&flags, "f", os.O_RDONLY, "Flags to use when opening the file (as raw integer)") + flag.UintVar(&mode, "m", 0444, "Mode to use when opening the file (as raw integer)") + flag.Parse() + + fd, err := os.OpenFile(path, flags, fs.FileMode(mode)) + if err != nil { + panic(err) + } + fd.Close() +} diff --git a/mirrord/layer/tests/common/mod.rs b/mirrord/layer/tests/common/mod.rs index cde99e61c7d..5e5e05a639d 100644 --- a/mirrord/layer/tests/common/mod.rs +++ b/mirrord/layer/tests/common/mod.rs @@ -752,6 +752,15 @@ pub enum Application { RustIssue2438, NodeIssue2807, RustRebind0, + /// Go application that simply opens a file. + Go23Open { + /// Path to the file, accepted as `-p` param. + path: String, + /// Flags to use when opening the file, accepted as `-f` param. + flags: i32, + /// Mode to use when opening the file, accepted as `-m` param. + mode: u32, + }, // For running applications with the executable and arguments determined at runtime. DynamicApp(String, Vec), } @@ -923,6 +932,7 @@ impl Application { ), Application::RustIssue2058 => String::from("tests/apps/issue2058/target/issue2058"), Application::RustIssue2204 => String::from("tests/apps/issue2204/target/issue2204"), + Application::Go23Open { .. } => String::from("tests/apps/open_go/23.go_test_app"), Application::DynamicApp(exe, _) => exe.clone(), } } @@ -1051,6 +1061,16 @@ impl Application { .into_iter() .map(Into::into) .collect(), + Application::Go23Open { path, flags, mode } => { + vec![ + "-p".to_string(), + path.clone(), + "-f".to_string(), + flags.to_string(), + "-m".to_string(), + mode.to_string(), + ] + } Application::DynamicApp(_, args) => args.to_owned(), } } @@ -1123,6 +1143,7 @@ impl Application { | Application::RustIssue2438 | Application::NodeIssue2807 | Application::RustRebind0 + | Application::Go23Open { .. } | Application::DynamicApp(..) => unimplemented!("shouldn't get here"), Application::PythonSelfConnect => 1337, Application::RustIssue2058 => 1234, diff --git a/mirrord/layer/tests/issue2614.rs b/mirrord/layer/tests/issue2614.rs new file mode 100644 index 00000000000..7f863451cbc --- /dev/null +++ b/mirrord/layer/tests/issue2614.rs @@ -0,0 +1,46 @@ +#![cfg(target_os = "linux")] +#![feature(assert_matches)] +#![warn(clippy::indexing_slicing)] + +use std::{os::unix::fs::PermissionsExt, path::Path, time::Duration}; + +use rand::{thread_rng, Rng}; +use rstest::rstest; + +mod common; + +pub use common::*; + +/// Verify that issue [#2614](https://github.com/metalbear-co/mirrord/issues/2614) is fixed +/// and the file open mode is honoured on bypass. +#[rstest] +#[tokio::test] +#[timeout(Duration::from_secs(60))] +async fn test_issue2614(dylib_path: &Path) { + let tmpdir = tempfile::tempdir().unwrap(); + let file_path = tmpdir + .path() + .join(format!("testfile-{}", thread_rng().gen::(),)); + let application = Application::Go23Open { + path: file_path.to_str().unwrap().into(), + flags: libc::O_CREAT | libc::O_RDWR, + mode: libc::S_IRUSR | libc::S_IRGRP | libc::S_IROTH, + }; + let (mut test_process, mut intproxy) = application + .start_process_with_layer(dylib_path, vec![("MIRRORD_FILE_MODE", "local")], None) + .await; + + let message = intproxy.try_recv().await; + assert!( + message.is_none(), + "received an unexpected message: {message:?}" + ); + test_process.wait_assert_success().await; + + let permissions = tokio::fs::metadata(file_path).await.unwrap().permissions(); + assert_eq!( + permissions.mode() & 0b111111111, + libc::S_IRUSR | libc::S_IRGRP | libc::S_IROTH, + "test app created file with unexpected permissions" + ) +}