From d1483535b632065e9e61598edcb7454568aa49b3 Mon Sep 17 00:00:00 2001 From: Facundo Poblete Date: Thu, 19 Dec 2024 01:58:32 -0300 Subject: [PATCH 01/12] Add rmdir support --- Cargo.lock | 2 +- changelog.d/2221.fixed.md | 2 +- mirrord/agent/src/file.rs | 11 +++ mirrord/intproxy/protocol/src/lib.rs | 7 ++ mirrord/intproxy/src/proxies/files.rs | 99 +++++++++++++------------- mirrord/layer/src/file/hooks.rs | 13 ++++ mirrord/layer/src/file/ops.rs | 24 ++++++- mirrord/layer/tests/apps/rmdir/rmdir.c | 21 ++++++ mirrord/layer/tests/common/mod.rs | 23 ++++++ mirrord/layer/tests/mkdir.rs | 4 +- mirrord/layer/tests/rmdir.rs | 31 ++++++++ mirrord/protocol/Cargo.toml | 2 +- mirrord/protocol/src/codec.rs | 2 + mirrord/protocol/src/file.rs | 10 +++ tests/go-e2e-dir/main.go | 6 ++ tests/python-e2e/ops.py | 11 ++- 16 files changed, 209 insertions(+), 59 deletions(-) create mode 100644 mirrord/layer/tests/apps/rmdir/rmdir.c create mode 100644 mirrord/layer/tests/rmdir.rs diff --git a/Cargo.lock b/Cargo.lock index 630f6c223ee..2fa998364dd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4503,7 +4503,7 @@ dependencies = [ [[package]] name = "mirrord-protocol" -version = "1.13.0" +version = "1.14.0" dependencies = [ "actix-codec", "bincode", diff --git a/changelog.d/2221.fixed.md b/changelog.d/2221.fixed.md index 13e0ddd0ae8..bc1359bcb8e 100644 --- a/changelog.d/2221.fixed.md +++ b/changelog.d/2221.fixed.md @@ -1 +1 @@ -Add mkdir support +Add mkdir / mkdirat / rmdir support diff --git a/mirrord/agent/src/file.rs b/mirrord/agent/src/file.rs index 9261b0bcb69..67d984f30f9 100644 --- a/mirrord/agent/src/file.rs +++ b/mirrord/agent/src/file.rs @@ -258,6 +258,9 @@ impl FileManager { pathname, mode, }) => Some(FileResponse::MakeDir(self.mkdirat(dirfd, &pathname, mode))), + FileRequest::RemoveDir(RemoveDirRequest { pathname }) => { + Some(FileResponse::RemoveDir(self.rmdir(&pathname))) + } }) } @@ -520,6 +523,14 @@ impl FileManager { } } + pub(crate) fn rmdir(&mut self, path: &Path) -> RemoteResult<()> { + trace!("FileManager::rmdir -> path {:#?}", path); + + let path = resolve_path(path, &self.root_path)?; + + std::fs::remove_dir(&path.as_path()).map_err(ResponseError::from) + } + pub(crate) fn seek(&mut self, fd: u64, seek_from: SeekFrom) -> RemoteResult { trace!( "FileManager::seek -> fd {:#?} | seek_from {:#?}", diff --git a/mirrord/intproxy/protocol/src/lib.rs b/mirrord/intproxy/protocol/src/lib.rs index 623020718b6..0a922232b6c 100644 --- a/mirrord/intproxy/protocol/src/lib.rs +++ b/mirrord/intproxy/protocol/src/lib.rs @@ -324,6 +324,13 @@ impl_request!( res_path = ProxyToLayerMessage::File => FileResponse::MakeDir, ); +impl_request!( + req = RemoveDirRequest, + res = RemoteResult<()>, + req_path = LayerToProxyMessage::File => FileRequest::RemoveDir, + res_path = ProxyToLayerMessage::File => FileResponse::RemoveDir, +); + impl_request!( req = SeekFileRequest, res = RemoteResult, diff --git a/mirrord/intproxy/src/proxies/files.rs b/mirrord/intproxy/src/proxies/files.rs index 79ac6695575..4cbb33d081c 100644 --- a/mirrord/intproxy/src/proxies/files.rs +++ b/mirrord/intproxy/src/proxies/files.rs @@ -6,7 +6,7 @@ use mirrord_protocol::{ file::{ CloseDirRequest, CloseFileRequest, DirEntryInternal, ReadDirBatchRequest, ReadDirResponse, ReadFileResponse, ReadLimitedFileRequest, SeekFromInternal, MKDIR_VERSION, - READDIR_BATCH_VERSION, READLINK_VERSION, + READDIR_BATCH_VERSION, READLINK_VERSION, RMDIR_VERSION, }, ClientMessage, DaemonMessage, ErrorKindInternal, FileRequest, FileResponse, RemoteIOError, ResponseError, @@ -253,6 +253,23 @@ impl FilesProxy { self.protocol_version.replace(version); } + fn is_request_supported(&self, request: &FileRequest) -> bool { + let protocol_version = self.protocol_version.as_ref(); + + match request { + FileRequest::ReadLink(..) => { + protocol_version.is_some_and(|version| READLINK_VERSION.matches(version)) + } + FileRequest::MakeDir(..) | FileRequest::MakeDirAt(..) => { + protocol_version.is_some_and(|version| MKDIR_VERSION.matches(version)) + } + FileRequest::RemoveDir(..) => { + protocol_version.is_some_and(|version| RMDIR_VERSION.matches(version)) + } + _ => true, + } + } + // #[tracing::instrument(level = Level::TRACE, skip(message_bus))] async fn file_request( &mut self, @@ -262,6 +279,33 @@ impl FilesProxy { message_bus: &mut MessageBus, ) { match request { + // Not supported in old `mirrord-protocol` versions. + r if !self.is_request_supported(&request) => { + let response = match r { + FileRequest::ReadLink(..) => { + FileResponse::ReadLink(Err(ResponseError::NotImplemented)) + } + FileRequest::MakeDir(..) => { + FileResponse::MakeDir(Err(ResponseError::NotImplemented)) + } + FileRequest::MakeDirAt(..) => { + FileResponse::MakeDir(Err(ResponseError::NotImplemented)) + } + FileRequest::RemoveDir(..) => { + FileResponse::RemoveDir(Err(ResponseError::NotImplemented)) + } + _ => unreachable!(), + }; + + message_bus + .send(ToLayer { + message_id, + layer_id, + message: ProxyToLayerMessage::File(response), + }) + .await; + } + // Should trigger remote close only when the fd is closed in all layer instances. FileRequest::Close(close) => { if self.remote_files.remove(layer_id, close.fd) { @@ -454,31 +498,6 @@ impl FilesProxy { } }, - // Not supported in old `mirrord-protocol` versions. - req @ FileRequest::ReadLink(..) => { - let supported = self - .protocol_version - .as_ref() - .is_some_and(|version| READLINK_VERSION.matches(version)); - - if supported { - self.request_queue.push_back(message_id, layer_id); - message_bus - .send(ProxyMessage::ToAgent(ClientMessage::FileRequest(req))) - .await; - } else { - message_bus - .send(ToLayer { - message_id, - message: ProxyToLayerMessage::File(FileResponse::ReadLink(Err( - ResponseError::NotImplemented, - ))), - layer_id, - }) - .await; - } - } - // Should only be sent from intproxy, not from the layer. FileRequest::ReadDirBatch(..) => { unreachable!("ReadDirBatch request is never sent from the layer"); @@ -522,34 +541,12 @@ impl FilesProxy { .await; } - FileRequest::MakeDir(_) | FileRequest::MakeDirAt(_) => { - let supported = self - .protocol_version - .as_ref() - .is_some_and(|version| MKDIR_VERSION.matches(version)); - - if supported { - self.request_queue.push_back(message_id, layer_id); - message_bus - .send(ProxyMessage::ToAgent(ClientMessage::FileRequest(request))) - .await; - } else { - let file_response = FileResponse::MakeDir(Err(ResponseError::NotImplemented)); - - message_bus - .send(ToLayer { - message_id, - message: ProxyToLayerMessage::File(file_response), - layer_id, - }) - .await; - } - } - // Doesn't require any special logic. other => { self.request_queue.push_back(message_id, layer_id); - message_bus.send(ClientMessage::FileRequest(other)).await; + message_bus + .send(ProxyMessage::ToAgent(ClientMessage::FileRequest(other))) + .await; } } } diff --git a/mirrord/layer/src/file/hooks.rs b/mirrord/layer/src/file/hooks.rs index 4de1e73577f..aaab7f64253 100644 --- a/mirrord/layer/src/file/hooks.rs +++ b/mirrord/layer/src/file/hooks.rs @@ -1088,6 +1088,17 @@ pub(crate) unsafe extern "C" fn mkdirat_detour( }) } +/// Hook for `libc::rmdir`. +#[hook_guard_fn] +pub(crate) unsafe extern "C" fn rmdir_detour(pathname: *const c_char) -> c_int { + rmdir(pathname.checked_into()) + .map(|()| 0) + .unwrap_or_bypass_with(|bypass| { + let raw_path = update_ptr_from_bypass(pathname, &bypass); + FN_RMDIR(raw_path) + }) +} + /// Convenience function to setup file hooks (`x_detour`) with `frida_gum`. pub(crate) unsafe fn enable_file_hooks(hook_manager: &mut HookManager) { replace!(hook_manager, "open", open_detour, FnOpen, FN_OPEN); @@ -1172,6 +1183,8 @@ pub(crate) unsafe fn enable_file_hooks(hook_manager: &mut HookManager) { FN_MKDIRAT ); + replace!(hook_manager, "rmdir", rmdir_detour, FnRmdir, FN_RMDIR); + replace!(hook_manager, "lseek", lseek_detour, FnLseek, FN_LSEEK); replace!(hook_manager, "write", write_detour, FnWrite, FN_WRITE); diff --git a/mirrord/layer/src/file/ops.rs b/mirrord/layer/src/file/ops.rs index 580f8eacc8a..424028de02f 100644 --- a/mirrord/layer/src/file/ops.rs +++ b/mirrord/layer/src/file/ops.rs @@ -8,8 +8,8 @@ use libc::{c_int, iovec, unlink, AT_FDCWD}; use mirrord_protocol::{ file::{ MakeDirAtRequest, MakeDirRequest, OpenFileRequest, OpenFileResponse, OpenOptionsInternal, - ReadFileResponse, ReadLinkFileRequest, ReadLinkFileResponse, SeekFileResponse, - WriteFileResponse, XstatFsResponse, XstatResponse, + ReadFileResponse, ReadLinkFileRequest, ReadLinkFileResponse, RemoveDirRequest, + SeekFileResponse, WriteFileResponse, XstatFsResponse, XstatResponse, }, ResponseError, }; @@ -387,6 +387,26 @@ pub(crate) fn mkdirat(dirfd: RawFd, pathname: Detour, mode: u32) -> Det } } +#[mirrord_layer_macro::instrument(level = Level::TRACE, ret)] +pub(crate) fn rmdir(pathname: Detour) -> Detour<()> { + let pathname = pathname?; + + check_relative_paths!(pathname); + + let path = remap_path!(pathname); + + ensure_not_ignored!(path, false); + + let rmdir = RemoveDirRequest { pathname: path }; + + // `NotImplemented` error here means that the protocol doesn't support it. + match common::make_proxy_request_with_response(rmdir)? { + Ok(response) => Detour::Success(response), + Err(ResponseError::NotImplemented) => Detour::Bypass(Bypass::NotImplemented), + Err(fail) => Detour::Error(fail.into()), + } +} + pub(crate) fn pwrite(local_fd: RawFd, buffer: &[u8], offset: u64) -> Detour { let remote_fd = get_remote_fd(local_fd)?; trace!("pwrite: local_fd {local_fd}"); diff --git a/mirrord/layer/tests/apps/rmdir/rmdir.c b/mirrord/layer/tests/apps/rmdir/rmdir.c new file mode 100644 index 00000000000..bffbc5f5fa7 --- /dev/null +++ b/mirrord/layer/tests/apps/rmdir/rmdir.c @@ -0,0 +1,21 @@ +#include +#include +#include +#include +#include + +/// Test `rmdir`. +/// +/// Creates a folder and then removes it. +/// +int main() +{ + char *test_dir = "/test_dir"; + int mkdir_result = mkdir(mkdir_result, 0777); + assert(mkdir_result == 0); + + int rmdir_result = rmdir(test_dir); + assert(rmdir_result == 0); + + return 0; +} diff --git a/mirrord/layer/tests/common/mod.rs b/mirrord/layer/tests/common/mod.rs index 819dce2fe14..951149ed1e1 100644 --- a/mirrord/layer/tests/common/mod.rs +++ b/mirrord/layer/tests/common/mod.rs @@ -487,6 +487,25 @@ impl TestIntProxy { .unwrap(); } + /// Makes a [`FileRequest::RemoveDir`] and answers it. + pub async fn expect_remove_dir(&mut self, expected_dir_name: &str) { + // Expecting `rmdir` call with path. + assert_matches!( + self.recv().await, + ClientMessage::FileRequest(FileRequest::RemoveDir( + mirrord_protocol::file::RemoveDirRequest { pathname } + )) if pathname.to_str().unwrap() == expected_dir_name + ); + + // Answer `rmdir`. + self.codec + .send(DaemonMessage::File( + mirrord_protocol::FileResponse::RemoveDir(Ok(())), + )) + .await + .unwrap(); + } + /// Verify that the passed message (not the next message from self.codec!) is a file read. /// Return buffer size. pub async fn expect_message_file_read(message: ClientMessage, expected_fd: u64) -> u64 { @@ -763,6 +782,7 @@ pub enum Application { Fork, ReadLink, MakeDir, + RemoveDir, OpenFile, CIssue2055, CIssue2178, @@ -817,6 +837,7 @@ impl Application { Application::Fork => String::from("tests/apps/fork/out.c_test_app"), Application::ReadLink => String::from("tests/apps/readlink/out.c_test_app"), Application::MakeDir => String::from("tests/apps/mkdir/out.c_test_app"), + Application::RemoveDir => String::from("tests/apps/rmdir/out.c_test_app"), Application::Realpath => String::from("tests/apps/realpath/out.c_test_app"), Application::NodeHTTP | Application::NodeIssue2283 | Application::NodeIssue2807 => { String::from("node") @@ -1054,6 +1075,7 @@ impl Application { | Application::Fork | Application::ReadLink | Application::MakeDir + | Application::RemoveDir | Application::Realpath | Application::RustFileOps | Application::RustIssue1123 @@ -1131,6 +1153,7 @@ impl Application { | Application::Fork | Application::ReadLink | Application::MakeDir + | Application::RemoveDir | Application::Realpath | Application::Go21Issue834 | Application::Go22Issue834 diff --git a/mirrord/layer/tests/mkdir.rs b/mirrord/layer/tests/mkdir.rs index 5f0fe3301b3..76fa3e8936b 100644 --- a/mirrord/layer/tests/mkdir.rs +++ b/mirrord/layer/tests/mkdir.rs @@ -17,10 +17,10 @@ async fn mkdir(dylib_path: &Path) { .start_process_with_layer(dylib_path, Default::default(), None) .await; - println!("waiting for file request."); + println!("waiting for MakeDirRequest."); intproxy.expect_make_dir("/mkdir_test_path", 0o777).await; - println!("waiting for file request."); + println!("waiting for MakeDirRequest."); intproxy.expect_make_dir("/mkdirat_test_path", 0o777).await; assert_eq!(intproxy.try_recv().await, None); diff --git a/mirrord/layer/tests/rmdir.rs b/mirrord/layer/tests/rmdir.rs new file mode 100644 index 00000000000..910e3d18ff9 --- /dev/null +++ b/mirrord/layer/tests/rmdir.rs @@ -0,0 +1,31 @@ +#![feature(assert_matches)] +use std::{path::Path, time::Duration}; + +use rstest::rstest; + +mod common; +pub use common::*; + +/// Test for the [`libc::rmdir`] function. +#[rstest] +#[tokio::test] +#[timeout(Duration::from_secs(60))] +async fn rmdir(dylib_path: &Path) { + let application = Application::RemoveDir; + + let (mut test_process, mut intproxy) = application + .start_process_with_layer(dylib_path, Default::default(), None) + .await; + + println!("waiting for MakeDirRequest."); + intproxy.expect_make_dir("/test_dir", 0o777).await; + + println!("waiting for RemoveDirRequest."); + intproxy.expect_remove_dir("/test_dir").await; + + assert_eq!(intproxy.try_recv().await, None); + + test_process.wait_assert_success().await; + test_process.assert_no_error_in_stderr().await; + test_process.assert_no_error_in_stdout().await; +} diff --git a/mirrord/protocol/Cargo.toml b/mirrord/protocol/Cargo.toml index 91f44f8bde5..042a4b037c1 100644 --- a/mirrord/protocol/Cargo.toml +++ b/mirrord/protocol/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mirrord-protocol" -version = "1.13.0" +version = "1.14.0" authors.workspace = true description.workspace = true documentation.workspace = true diff --git a/mirrord/protocol/src/codec.rs b/mirrord/protocol/src/codec.rs index 4071018fe6a..43a6d84d3a6 100644 --- a/mirrord/protocol/src/codec.rs +++ b/mirrord/protocol/src/codec.rs @@ -85,6 +85,7 @@ pub enum FileRequest { ReadDirBatch(ReadDirBatchRequest), MakeDir(MakeDirRequest), MakeDirAt(MakeDirAtRequest), + RemoveDir(RemoveDirRequest), } /// Minimal mirrord-protocol version that allows `ClientMessage::ReadyForLogs` message. @@ -130,6 +131,7 @@ pub enum FileResponse { ReadLink(RemoteResult), ReadDirBatch(RemoteResult), MakeDir(RemoteResult<()>), + RemoveDir(RemoteResult<()>), } /// `-agent` --> `-layer` messages. diff --git a/mirrord/protocol/src/file.rs b/mirrord/protocol/src/file.rs index b2e8e3773f8..a0de08655b0 100644 --- a/mirrord/protocol/src/file.rs +++ b/mirrord/protocol/src/file.rs @@ -22,9 +22,14 @@ pub static READLINK_VERSION: LazyLock = pub static READDIR_BATCH_VERSION: LazyLock = LazyLock::new(|| ">=1.9.0".parse().expect("Bad Identifier")); +/// Minimal mirrord-protocol version that allows [`MakeDirRequest`] and [`MakeDirAtRequest`]. pub static MKDIR_VERSION: LazyLock = LazyLock::new(|| ">=1.13.0".parse().expect("Bad Identifier")); +/// Minimal mirrord-protocol version that allows [`RemoveDirRequest`]. +pub static RMDIR_VERSION: LazyLock = + LazyLock::new(|| ">=1.14.0".parse().expect("Bad Identifier")); + /// Internal version of Metadata across operating system (macOS, Linux) /// Only mutual attributes #[derive(Encode, Decode, Debug, PartialEq, Clone, Copy, Eq, Default)] @@ -282,6 +287,11 @@ pub struct MakeDirAtRequest { pub mode: u32, } +#[derive(Encode, Decode, Debug, PartialEq, Eq, Clone)] +pub struct RemoveDirRequest { + pub pathname: PathBuf, +} + #[derive(Encode, Decode, Debug, PartialEq, Eq, Clone)] pub struct ReadLimitedFileRequest { pub remote_fd: u64, diff --git a/tests/go-e2e-dir/main.go b/tests/go-e2e-dir/main.go index b608f01b53b..61aba37232b 100644 --- a/tests/go-e2e-dir/main.go +++ b/tests/go-e2e-dir/main.go @@ -30,6 +30,12 @@ func main() { os.Exit(-1) } + err = os.Remove("/app/test_mkdir") + if err != nil { + fmt.Printf("Rmdir error: %s\n", err) + os.Exit(-1) + } + // let close requests be sent for test time.Sleep(1 * time.Second) os.Exit(0) diff --git a/tests/python-e2e/ops.py b/tests/python-e2e/ops.py index 8e83271628f..6d1b5b5a67f 100644 --- a/tests/python-e2e/ops.py +++ b/tests/python-e2e/ops.py @@ -87,7 +87,16 @@ def test_mkdir_errors(self): os.mkdir("test_mkdir_error_already_exists", dir_fd=dir) os.close(dir) - + + def test_rmdir(self): + """ + Creates a new directory in "/tmp" and removes it using rmdir. + """ + os.mkdir("/tmp/test_rmdir") + self.assertTrue(os.path.isdir("/tmp/test_rmdir")) + os.rmdir("/tmp/test_rmdir") + self.assertFalse(os.path.isdir("/tmp/test_rmdir")) + def _create_new_tmp_file(self): """ Creates a new file in /tmp and returns the path and name of the file. From f568dff067e822b80abe041de8d2bda587301e55 Mon Sep 17 00:00:00 2001 From: Facundo Poblete Date: Thu, 19 Dec 2024 02:10:13 -0300 Subject: [PATCH 02/12] Fix lint --- mirrord/agent/src/file.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mirrord/agent/src/file.rs b/mirrord/agent/src/file.rs index 67d984f30f9..068ff0fe917 100644 --- a/mirrord/agent/src/file.rs +++ b/mirrord/agent/src/file.rs @@ -528,7 +528,7 @@ impl FileManager { let path = resolve_path(path, &self.root_path)?; - std::fs::remove_dir(&path.as_path()).map_err(ResponseError::from) + std::fs::remove_dir(path.as_path()).map_err(ResponseError::from) } pub(crate) fn seek(&mut self, fd: u64, seek_from: SeekFrom) -> RemoteResult { From 804e0eeef300bfe546f26a2a03ee02aa46c1c15e Mon Sep 17 00:00:00 2001 From: Facundo Poblete Date: Thu, 19 Dec 2024 02:25:39 -0300 Subject: [PATCH 03/12] Fix rmdir.c --- mirrord/layer/tests/apps/mkdir/mkdir.c | 1 - mirrord/layer/tests/apps/rmdir/rmdir.c | 5 ++--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/mirrord/layer/tests/apps/mkdir/mkdir.c b/mirrord/layer/tests/apps/mkdir/mkdir.c index 4253b6c089d..8631ae7a26a 100644 --- a/mirrord/layer/tests/apps/mkdir/mkdir.c +++ b/mirrord/layer/tests/apps/mkdir/mkdir.c @@ -1,6 +1,5 @@ #include #include -#include #include #include diff --git a/mirrord/layer/tests/apps/rmdir/rmdir.c b/mirrord/layer/tests/apps/rmdir/rmdir.c index bffbc5f5fa7..f839e5256e7 100644 --- a/mirrord/layer/tests/apps/rmdir/rmdir.c +++ b/mirrord/layer/tests/apps/rmdir/rmdir.c @@ -1,8 +1,7 @@ #include #include -#include +#include #include -#include /// Test `rmdir`. /// @@ -11,7 +10,7 @@ int main() { char *test_dir = "/test_dir"; - int mkdir_result = mkdir(mkdir_result, 0777); + int mkdir_result = mkdir(test_dir, 0777); assert(mkdir_result == 0); int rmdir_result = rmdir(test_dir); From 66ad7b8b95ea2d9ec63bebec46e06956dad95661 Mon Sep 17 00:00:00 2001 From: Facundo Poblete Date: Thu, 19 Dec 2024 03:27:50 -0300 Subject: [PATCH 04/12] Fix rmdir on go --- mirrord/layer/src/go/linux_x64.rs | 1 + mirrord/layer/src/go/mod.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/mirrord/layer/src/go/linux_x64.rs b/mirrord/layer/src/go/linux_x64.rs index dcabdeee4b7..8a8f2fdd994 100644 --- a/mirrord/layer/src/go/linux_x64.rs +++ b/mirrord/layer/src/go/linux_x64.rs @@ -344,6 +344,7 @@ unsafe extern "C" fn c_abi_syscall_handler( #[cfg(all(target_os = "linux", not(target_arch = "aarch64")))] libc::SYS_mkdir => mkdir_detour(param1 as _, param2 as _) as i64, libc::SYS_mkdirat => mkdirat_detour(param1 as _, param2 as _, param3 as _) as i64, + libc::SYS_rmdir => rmdir_detour(param1 as _) as i64, _ => { let (Ok(result) | Err(result)) = syscalls::syscall!( syscalls::Sysno::from(syscall as i32), diff --git a/mirrord/layer/src/go/mod.rs b/mirrord/layer/src/go/mod.rs index 003eed8692c..24627c7184b 100644 --- a/mirrord/layer/src/go/mod.rs +++ b/mirrord/layer/src/go/mod.rs @@ -113,6 +113,7 @@ unsafe extern "C" fn c_abi_syscall6_handler( #[cfg(all(target_os = "linux", not(target_arch = "aarch64")))] libc::SYS_mkdir => mkdir_detour(param1 as _, param2 as _) as i64, libc::SYS_mkdirat => mkdirat_detour(param1 as _, param2 as _, param3 as _) as i64, + libc::SYS_rmdir => rmdir_detour(param1 as _) as i64, _ => { let (Ok(result) | Err(result)) = syscalls::syscall!( syscalls::Sysno::from(syscall as i32), From 8d2c41a7363a3ba845acca4a419bda280d073b12 Mon Sep 17 00:00:00 2001 From: Facundo Poblete Date: Thu, 19 Dec 2024 04:30:32 -0300 Subject: [PATCH 05/12] SYS_rmdir doesn't exist on aarch64 --- mirrord/layer/src/go/linux_x64.rs | 1 + mirrord/layer/src/go/mod.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/mirrord/layer/src/go/linux_x64.rs b/mirrord/layer/src/go/linux_x64.rs index 8a8f2fdd994..1eed665ce90 100644 --- a/mirrord/layer/src/go/linux_x64.rs +++ b/mirrord/layer/src/go/linux_x64.rs @@ -344,6 +344,7 @@ unsafe extern "C" fn c_abi_syscall_handler( #[cfg(all(target_os = "linux", not(target_arch = "aarch64")))] libc::SYS_mkdir => mkdir_detour(param1 as _, param2 as _) as i64, libc::SYS_mkdirat => mkdirat_detour(param1 as _, param2 as _, param3 as _) as i64, + #[cfg(all(target_os = "linux", not(target_arch = "aarch64")))] libc::SYS_rmdir => rmdir_detour(param1 as _) as i64, _ => { let (Ok(result) | Err(result)) = syscalls::syscall!( diff --git a/mirrord/layer/src/go/mod.rs b/mirrord/layer/src/go/mod.rs index 24627c7184b..d3d47bf1d58 100644 --- a/mirrord/layer/src/go/mod.rs +++ b/mirrord/layer/src/go/mod.rs @@ -113,6 +113,7 @@ unsafe extern "C" fn c_abi_syscall6_handler( #[cfg(all(target_os = "linux", not(target_arch = "aarch64")))] libc::SYS_mkdir => mkdir_detour(param1 as _, param2 as _) as i64, libc::SYS_mkdirat => mkdirat_detour(param1 as _, param2 as _, param3 as _) as i64, + #[cfg(all(target_os = "linux", not(target_arch = "aarch64")))] libc::SYS_rmdir => rmdir_detour(param1 as _) as i64, _ => { let (Ok(result) | Err(result)) = syscalls::syscall!( From 8cf0cc26097580761d08aa5158f6a76b22368a36 Mon Sep 17 00:00:00 2001 From: Facundo Poblete Date: Thu, 19 Dec 2024 14:31:34 -0300 Subject: [PATCH 06/12] Add unlink/unlinkat support --- mirrord/agent/src/file.rs | 71 +++++++++++++++++++++++++++- mirrord/intproxy/protocol/src/lib.rs | 14 ++++++ mirrord/layer/src/file/hooks.rs | 36 +++++++++++++- mirrord/layer/src/file/ops.rs | 49 +++++++++++++++++-- mirrord/layer/src/go/linux_x64.rs | 3 ++ mirrord/layer/src/go/mod.rs | 3 ++ mirrord/protocol/src/codec.rs | 3 ++ mirrord/protocol/src/file.rs | 15 +++++- 8 files changed, 188 insertions(+), 6 deletions(-) diff --git a/mirrord/agent/src/file.rs b/mirrord/agent/src/file.rs index 068ff0fe917..4e7efd6313e 100644 --- a/mirrord/agent/src/file.rs +++ b/mirrord/agent/src/file.rs @@ -5,13 +5,17 @@ use std::{ io::{self, prelude::*, BufReader, SeekFrom}, iter::{Enumerate, Peekable}, ops::RangeInclusive, - os::unix::{fs::MetadataExt, prelude::FileExt}, + os::{ + fd::RawFd, + unix::{fs::MetadataExt, prelude::FileExt}, + }, path::{Path, PathBuf}, }; use faccess::{AccessMode, PathExt}; use libc::DT_DIR; use mirrord_protocol::{file::*, FileRequest, FileResponse, RemoteResult, ResponseError}; +use nix::unistd::UnlinkatFlags; use tracing::{error, trace, Level}; use crate::error::Result; @@ -261,6 +265,14 @@ impl FileManager { FileRequest::RemoveDir(RemoveDirRequest { pathname }) => { Some(FileResponse::RemoveDir(self.rmdir(&pathname))) } + FileRequest::Unlink(UnlinkRequest { pathname }) => { + Some(FileResponse::Unlink(self.unlink(&pathname))) + } + FileRequest::UnlinkAt(UnlinkAtRequest { + dirfd, + pathname, + flags, + }) => Some(FileResponse::Unlink(self.unlinkat(dirfd, &pathname, flags))), }) } @@ -531,6 +543,63 @@ impl FileManager { std::fs::remove_dir(path.as_path()).map_err(ResponseError::from) } + pub(crate) fn unlink(&mut self, path: &Path) -> RemoteResult<()> { + trace!("FileManager::unlink -> path {:#?}", path); + + let path = resolve_path(path, &self.root_path)?; + + match nix::unistd::unlink(path.as_path()) { + Ok(_) => Ok(()), + Err(err) => Err(ResponseError::from(std::io::Error::from_raw_os_error( + err as i32, + ))), + } + } + + pub(crate) fn unlinkat( + &mut self, + dirfd: Option, + path: &Path, + flags: u32, + ) -> RemoteResult<()> { + trace!( + "FileManager::unlinkat -> dirfd {:#?} | path {:#?} | flags {:#?}", + dirfd, + path, + flags + ); + + let path = match dirfd { + Some(dirfd) => { + let relative_dir = self + .open_files + .get(&dirfd) + .ok_or(ResponseError::NotFound(dirfd))?; + + if let RemoteFile::Directory(relative_dir) = relative_dir { + relative_dir.join(path) + } else { + return Err(ResponseError::NotDirectory(dirfd)); + } + } + None => resolve_path(path, &self.root_path)?, + }; + + let flags = match flags { + 0 => UnlinkatFlags::RemoveDir, + _ => UnlinkatFlags::NoRemoveDir, + }; + + let fd: Option = dirfd.map(|fd| fd as RawFd); + + match nix::unistd::unlinkat(fd, path.as_path(), flags) { + Ok(_) => Ok(()), + Err(err) => Err(ResponseError::from(std::io::Error::from_raw_os_error( + err as i32, + ))), + } + } + pub(crate) fn seek(&mut self, fd: u64, seek_from: SeekFrom) -> RemoteResult { trace!( "FileManager::seek -> fd {:#?} | seek_from {:#?}", diff --git a/mirrord/intproxy/protocol/src/lib.rs b/mirrord/intproxy/protocol/src/lib.rs index 0a922232b6c..69a551b50bc 100644 --- a/mirrord/intproxy/protocol/src/lib.rs +++ b/mirrord/intproxy/protocol/src/lib.rs @@ -331,6 +331,20 @@ impl_request!( res_path = ProxyToLayerMessage::File => FileResponse::RemoveDir, ); +impl_request!( + req = UnlinkRequest, + res = RemoteResult<()>, + req_path = LayerToProxyMessage::File => FileRequest::Unlink, + res_path = ProxyToLayerMessage::File => FileResponse::Unlink, +); + +impl_request!( + req = UnlinkAtRequest, + res = RemoteResult<()>, + req_path = LayerToProxyMessage::File => FileRequest::UnlinkAt, + res_path = ProxyToLayerMessage::File => FileResponse::Unlink, +); + impl_request!( req = SeekFileRequest, res = RemoteResult, diff --git a/mirrord/layer/src/file/hooks.rs b/mirrord/layer/src/file/hooks.rs index aaab7f64253..7c46165b37a 100644 --- a/mirrord/layer/src/file/hooks.rs +++ b/mirrord/layer/src/file/hooks.rs @@ -1099,6 +1099,32 @@ pub(crate) unsafe extern "C" fn rmdir_detour(pathname: *const c_char) -> c_int { }) } +/// Hook for `libc::unlink`. +#[hook_guard_fn] +pub(crate) unsafe extern "C" fn unlink_detour(pathname: *const c_char) -> c_int { + unlink(pathname.checked_into()) + .map(|()| 0) + .unwrap_or_bypass_with(|bypass| { + let raw_path = update_ptr_from_bypass(pathname, &bypass); + FN_UNLINK(raw_path) + }) +} + +/// Hook for `libc::unlinkat`. +#[hook_guard_fn] +pub(crate) unsafe extern "C" fn unlinkat_detour( + dirfd: c_int, + pathname: *const c_char, + flags: u32, +) -> c_int { + unlinkat(dirfd, pathname.checked_into(), flags) + .map(|()| 0) + .unwrap_or_bypass_with(|bypass| { + let raw_path = update_ptr_from_bypass(pathname, &bypass); + FN_UNLINKAT(dirfd, raw_path, flags) + }) +} + /// Convenience function to setup file hooks (`x_detour`) with `frida_gum`. pub(crate) unsafe fn enable_file_hooks(hook_manager: &mut HookManager) { replace!(hook_manager, "open", open_detour, FnOpen, FN_OPEN); @@ -1174,7 +1200,6 @@ pub(crate) unsafe fn enable_file_hooks(hook_manager: &mut HookManager) { ); replace!(hook_manager, "mkdir", mkdir_detour, FnMkdir, FN_MKDIR); - replace!( hook_manager, "mkdirat", @@ -1185,6 +1210,15 @@ pub(crate) unsafe fn enable_file_hooks(hook_manager: &mut HookManager) { replace!(hook_manager, "rmdir", rmdir_detour, FnRmdir, FN_RMDIR); + replace!(hook_manager, "unlink", unlink_detour, FnUnlink, FN_UNLINK); + replace!( + hook_manager, + "unlinkat", + unlinkat_detour, + FnUnlinkat, + FN_UNLINKAT + ); + replace!(hook_manager, "lseek", lseek_detour, FnLseek, FN_LSEEK); replace!(hook_manager, "write", write_detour, FnWrite, FN_WRITE); diff --git a/mirrord/layer/src/file/ops.rs b/mirrord/layer/src/file/ops.rs index 424028de02f..d43bbaab8d7 100644 --- a/mirrord/layer/src/file/ops.rs +++ b/mirrord/layer/src/file/ops.rs @@ -4,12 +4,12 @@ use std::{env, ffi::CString, io::SeekFrom, os::unix::io::RawFd, path::PathBuf}; #[cfg(target_os = "linux")] use libc::{c_char, statx, statx_timestamp}; -use libc::{c_int, iovec, unlink, AT_FDCWD}; +use libc::{c_int, iovec, AT_FDCWD}; use mirrord_protocol::{ file::{ MakeDirAtRequest, MakeDirRequest, OpenFileRequest, OpenFileResponse, OpenOptionsInternal, ReadFileResponse, ReadLinkFileRequest, ReadLinkFileResponse, RemoveDirRequest, - SeekFileResponse, WriteFileResponse, XstatFsResponse, XstatResponse, + SeekFileResponse, UnlinkAtRequest, WriteFileResponse, XstatFsResponse, XstatResponse, }, ResponseError, }; @@ -157,7 +157,7 @@ fn create_local_fake_file(remote_fd: u64) -> Detour { close_remote_file_on_failure(remote_fd)?; Detour::Error(HookError::LocalFileCreation(remote_fd, error.0)) } else { - unsafe { unlink(file_path_ptr) }; + unsafe { libc::unlink(file_path_ptr) }; Detour::Success(local_file_fd) } } @@ -407,6 +407,49 @@ pub(crate) fn rmdir(pathname: Detour) -> Detour<()> { } } +#[mirrord_layer_macro::instrument(level = "trace")] +pub(crate) fn unlink(pathname: Detour) -> Detour<()> { + let pathname = pathname?; + + check_relative_paths!(pathname); + + let path = remap_path!(pathname); + + ensure_not_ignored!(path, false); + + let unlink = RemoveDirRequest { pathname: path }; + + // `NotImplemented` error here means that the protocol doesn't support it. + match common::make_proxy_request_with_response(unlink)? { + Ok(response) => Detour::Success(response), + Err(ResponseError::NotImplemented) => Detour::Bypass(Bypass::NotImplemented), + Err(fail) => Detour::Error(fail.into()), + } +} + +#[mirrord_layer_macro::instrument(level = "trace")] +pub(crate) fn unlinkat(dirfd: RawFd, pathname: Detour, flags: u32) -> Detour<()> { + let pathname = pathname?; + + let optional_dirfd = match pathname.is_absolute() { + true => None, + false => Some(get_remote_fd(dirfd)?), + }; + + let unlink = UnlinkAtRequest { + dirfd: optional_dirfd, + pathname: pathname.clone(), + flags, + }; + + // `NotImplemented` error here means that the protocol doesn't support it. + match common::make_proxy_request_with_response(unlink)? { + Ok(response) => Detour::Success(response), + Err(ResponseError::NotImplemented) => Detour::Bypass(Bypass::NotImplemented), + Err(fail) => Detour::Error(fail.into()), + } +} + pub(crate) fn pwrite(local_fd: RawFd, buffer: &[u8], offset: u64) -> Detour { let remote_fd = get_remote_fd(local_fd)?; trace!("pwrite: local_fd {local_fd}"); diff --git a/mirrord/layer/src/go/linux_x64.rs b/mirrord/layer/src/go/linux_x64.rs index 1eed665ce90..b2d81bae625 100644 --- a/mirrord/layer/src/go/linux_x64.rs +++ b/mirrord/layer/src/go/linux_x64.rs @@ -346,6 +346,9 @@ unsafe extern "C" fn c_abi_syscall_handler( libc::SYS_mkdirat => mkdirat_detour(param1 as _, param2 as _, param3 as _) as i64, #[cfg(all(target_os = "linux", not(target_arch = "aarch64")))] libc::SYS_rmdir => rmdir_detour(param1 as _) as i64, + #[cfg(all(target_os = "linux", not(target_arch = "aarch64")))] + libc::SYS_unlink => unlink_detour(param1 as _) as i64, + libc::SYS_unlinkat => unlinkat_detour(param1 as _, param2 as _, param3 as _) as i64, _ => { let (Ok(result) | Err(result)) = syscalls::syscall!( syscalls::Sysno::from(syscall as i32), diff --git a/mirrord/layer/src/go/mod.rs b/mirrord/layer/src/go/mod.rs index d3d47bf1d58..df810bbdcf9 100644 --- a/mirrord/layer/src/go/mod.rs +++ b/mirrord/layer/src/go/mod.rs @@ -115,6 +115,9 @@ unsafe extern "C" fn c_abi_syscall6_handler( libc::SYS_mkdirat => mkdirat_detour(param1 as _, param2 as _, param3 as _) as i64, #[cfg(all(target_os = "linux", not(target_arch = "aarch64")))] libc::SYS_rmdir => rmdir_detour(param1 as _) as i64, + #[cfg(all(target_os = "linux", not(target_arch = "aarch64")))] + libc::SYS_unlink => unlink_detour(param1 as _) as i64, + libc::SYS_unlinkat => unlinkat_detour(param1 as _, param2 as _, param3 as _) as i64, _ => { let (Ok(result) | Err(result)) = syscalls::syscall!( syscalls::Sysno::from(syscall as i32), diff --git a/mirrord/protocol/src/codec.rs b/mirrord/protocol/src/codec.rs index 43a6d84d3a6..ce92862b30e 100644 --- a/mirrord/protocol/src/codec.rs +++ b/mirrord/protocol/src/codec.rs @@ -86,6 +86,8 @@ pub enum FileRequest { MakeDir(MakeDirRequest), MakeDirAt(MakeDirAtRequest), RemoveDir(RemoveDirRequest), + Unlink(UnlinkRequest), + UnlinkAt(UnlinkAtRequest), } /// Minimal mirrord-protocol version that allows `ClientMessage::ReadyForLogs` message. @@ -132,6 +134,7 @@ pub enum FileResponse { ReadDirBatch(RemoteResult), MakeDir(RemoteResult<()>), RemoveDir(RemoteResult<()>), + Unlink(RemoteResult<()>), } /// `-agent` --> `-layer` messages. diff --git a/mirrord/protocol/src/file.rs b/mirrord/protocol/src/file.rs index a0de08655b0..32bc5b939f2 100644 --- a/mirrord/protocol/src/file.rs +++ b/mirrord/protocol/src/file.rs @@ -26,7 +26,8 @@ pub static READDIR_BATCH_VERSION: LazyLock = pub static MKDIR_VERSION: LazyLock = LazyLock::new(|| ">=1.13.0".parse().expect("Bad Identifier")); -/// Minimal mirrord-protocol version that allows [`RemoveDirRequest`]. +/// Minimal mirrord-protocol version that allows [`RemoveDirRequest`], [`UnlinkRequest`] and +/// [`UnlinkAtRequest`].. pub static RMDIR_VERSION: LazyLock = LazyLock::new(|| ">=1.14.0".parse().expect("Bad Identifier")); @@ -292,6 +293,18 @@ pub struct RemoveDirRequest { pub pathname: PathBuf, } +#[derive(Encode, Decode, Debug, PartialEq, Eq, Clone)] +pub struct UnlinkRequest { + pub pathname: PathBuf, +} + +#[derive(Encode, Decode, Debug, PartialEq, Eq, Clone)] +pub struct UnlinkAtRequest { + pub dirfd: Option, + pub pathname: PathBuf, + pub flags: u32, +} + #[derive(Encode, Decode, Debug, PartialEq, Eq, Clone)] pub struct ReadLimitedFileRequest { pub remote_fd: u64, From 429d4b8ef171409d75a13d8f7e8ba113ed14cd24 Mon Sep 17 00:00:00 2001 From: Facundo Poblete Date: Thu, 19 Dec 2024 15:07:20 -0300 Subject: [PATCH 07/12] Check protocol supported version on unlink/unlinkat requests --- mirrord/intproxy/src/proxies/files.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/mirrord/intproxy/src/proxies/files.rs b/mirrord/intproxy/src/proxies/files.rs index 4cbb33d081c..489a4d21710 100644 --- a/mirrord/intproxy/src/proxies/files.rs +++ b/mirrord/intproxy/src/proxies/files.rs @@ -263,7 +263,7 @@ impl FilesProxy { FileRequest::MakeDir(..) | FileRequest::MakeDirAt(..) => { protocol_version.is_some_and(|version| MKDIR_VERSION.matches(version)) } - FileRequest::RemoveDir(..) => { + FileRequest::RemoveDir(..) | FileRequest::Unlink(..) | FileRequest::UnlinkAt(..) => { protocol_version.is_some_and(|version| RMDIR_VERSION.matches(version)) } _ => true, @@ -285,15 +285,15 @@ impl FilesProxy { FileRequest::ReadLink(..) => { FileResponse::ReadLink(Err(ResponseError::NotImplemented)) } - FileRequest::MakeDir(..) => { - FileResponse::MakeDir(Err(ResponseError::NotImplemented)) - } - FileRequest::MakeDirAt(..) => { + FileRequest::MakeDir(..) | FileRequest::MakeDirAt(..) => { FileResponse::MakeDir(Err(ResponseError::NotImplemented)) } FileRequest::RemoveDir(..) => { FileResponse::RemoveDir(Err(ResponseError::NotImplemented)) } + FileRequest::Unlink(..) | FileRequest::UnlinkAt(..) => { + FileResponse::Unlink(Err(ResponseError::NotImplemented)) + } _ => unreachable!(), }; From e941c08dfd3fcd4de2e046d115eb97720f670f41 Mon Sep 17 00:00:00 2001 From: Facundo Poblete Date: Thu, 19 Dec 2024 17:39:59 -0300 Subject: [PATCH 08/12] Revert message_bus.send --- mirrord/intproxy/src/proxies/files.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/mirrord/intproxy/src/proxies/files.rs b/mirrord/intproxy/src/proxies/files.rs index 489a4d21710..0ff79b7f8a7 100644 --- a/mirrord/intproxy/src/proxies/files.rs +++ b/mirrord/intproxy/src/proxies/files.rs @@ -544,9 +544,7 @@ impl FilesProxy { // Doesn't require any special logic. other => { self.request_queue.push_back(message_id, layer_id); - message_bus - .send(ProxyMessage::ToAgent(ClientMessage::FileRequest(other))) - .await; + message_bus.send(ClientMessage::FileRequest(other)).await; } } } From 81489b28fd038dcfcc943588a0fa04fb7ebc919c Mon Sep 17 00:00:00 2001 From: Facundo Date: Mon, 6 Jan 2025 12:50:07 -0300 Subject: [PATCH 09/12] PR review --- changelog.d/2221.added.md | 1 + changelog.d/2221.fixed.md | 1 - mirrord/agent/src/file.rs | 12 ++---- mirrord/intproxy/src/proxies/files.rs | 54 ++++++++++++++------------- mirrord/layer/src/file/ops.rs | 4 +- 5 files changed, 35 insertions(+), 37 deletions(-) create mode 100644 changelog.d/2221.added.md delete mode 100644 changelog.d/2221.fixed.md diff --git a/changelog.d/2221.added.md b/changelog.d/2221.added.md new file mode 100644 index 00000000000..fe396a1ac03 --- /dev/null +++ b/changelog.d/2221.added.md @@ -0,0 +1 @@ +Add rmdir / unlink / unlinkat support diff --git a/changelog.d/2221.fixed.md b/changelog.d/2221.fixed.md deleted file mode 100644 index bc1359bcb8e..00000000000 --- a/changelog.d/2221.fixed.md +++ /dev/null @@ -1 +0,0 @@ -Add mkdir / mkdirat / rmdir support diff --git a/mirrord/agent/src/file.rs b/mirrord/agent/src/file.rs index 4e7efd6313e..b7c5fb0fdbc 100644 --- a/mirrord/agent/src/file.rs +++ b/mirrord/agent/src/file.rs @@ -535,16 +535,15 @@ impl FileManager { } } + #[tracing::instrument(level = Level::TRACE, skip(self))] pub(crate) fn rmdir(&mut self, path: &Path) -> RemoteResult<()> { - trace!("FileManager::rmdir -> path {:#?}", path); - let path = resolve_path(path, &self.root_path)?; std::fs::remove_dir(path.as_path()).map_err(ResponseError::from) } + #[tracing::instrument(level = Level::TRACE, skip(self))] pub(crate) fn unlink(&mut self, path: &Path) -> RemoteResult<()> { - trace!("FileManager::unlink -> path {:#?}", path); let path = resolve_path(path, &self.root_path)?; @@ -556,18 +555,13 @@ impl FileManager { } } + #[tracing::instrument(level = Level::TRACE, skip(self))] pub(crate) fn unlinkat( &mut self, dirfd: Option, path: &Path, flags: u32, ) -> RemoteResult<()> { - trace!( - "FileManager::unlinkat -> dirfd {:#?} | path {:#?} | flags {:#?}", - dirfd, - path, - flags - ); let path = match dirfd { Some(dirfd) => { diff --git a/mirrord/intproxy/src/proxies/files.rs b/mirrord/intproxy/src/proxies/files.rs index 0ff79b7f8a7..8a52841ed33 100644 --- a/mirrord/intproxy/src/proxies/files.rs +++ b/mirrord/intproxy/src/proxies/files.rs @@ -253,20 +253,28 @@ impl FilesProxy { self.protocol_version.replace(version); } - fn is_request_supported(&self, request: &FileRequest) -> bool { + /// Checks if the mirrord protocol version supports this [`FileRequest`]. + fn is_request_supported(&self, request: &FileRequest) -> Result<(), FileResponse> { let protocol_version = self.protocol_version.as_ref(); match request { - FileRequest::ReadLink(..) => { - protocol_version.is_some_and(|version| READLINK_VERSION.matches(version)) + FileRequest::ReadLink(..) + if protocol_version.is_some_and(|version| !READLINK_VERSION.matches(version)) => + { + return Err(FileResponse::ReadLink(Err(ResponseError::NotImplemented))); } - FileRequest::MakeDir(..) | FileRequest::MakeDirAt(..) => { - protocol_version.is_some_and(|version| MKDIR_VERSION.matches(version)) + FileRequest::MakeDir(..) | FileRequest::MakeDirAt(..) + if protocol_version.is_some_and(|version| !MKDIR_VERSION.matches(version)) => + { + return Err(FileResponse::MakeDir(Err(ResponseError::NotImplemented))); } - FileRequest::RemoveDir(..) | FileRequest::Unlink(..) | FileRequest::UnlinkAt(..) => { - protocol_version.is_some_and(|version| RMDIR_VERSION.matches(version)) + FileRequest::RemoveDir(..) | FileRequest::Unlink(..) | FileRequest::UnlinkAt(..) + if protocol_version + .is_some_and(|version: &Version| !RMDIR_VERSION.matches(version)) => + { + return Err(FileResponse::RemoveDir(Err(ResponseError::NotImplemented))); } - _ => true, + _ => Ok(()), } } @@ -278,24 +286,20 @@ impl FilesProxy { message_id: MessageId, message_bus: &mut MessageBus, ) { + + // Not supported in old `mirrord-protocol` versions. + if let Err(response) = self.is_request_supported(&request) { + message_bus + .send(ToLayer { + message_id, + layer_id, + message: ProxyToLayerMessage::File(response), + }) + .await; + return; + } + match request { - // Not supported in old `mirrord-protocol` versions. - r if !self.is_request_supported(&request) => { - let response = match r { - FileRequest::ReadLink(..) => { - FileResponse::ReadLink(Err(ResponseError::NotImplemented)) - } - FileRequest::MakeDir(..) | FileRequest::MakeDirAt(..) => { - FileResponse::MakeDir(Err(ResponseError::NotImplemented)) - } - FileRequest::RemoveDir(..) => { - FileResponse::RemoveDir(Err(ResponseError::NotImplemented)) - } - FileRequest::Unlink(..) | FileRequest::UnlinkAt(..) => { - FileResponse::Unlink(Err(ResponseError::NotImplemented)) - } - _ => unreachable!(), - }; message_bus .send(ToLayer { diff --git a/mirrord/layer/src/file/ops.rs b/mirrord/layer/src/file/ops.rs index d43bbaab8d7..a60c3f5d2d8 100644 --- a/mirrord/layer/src/file/ops.rs +++ b/mirrord/layer/src/file/ops.rs @@ -407,7 +407,7 @@ pub(crate) fn rmdir(pathname: Detour) -> Detour<()> { } } -#[mirrord_layer_macro::instrument(level = "trace")] +#[mirrord_layer_macro::instrument(level = Level::TRACE, ret)] pub(crate) fn unlink(pathname: Detour) -> Detour<()> { let pathname = pathname?; @@ -427,7 +427,7 @@ pub(crate) fn unlink(pathname: Detour) -> Detour<()> { } } -#[mirrord_layer_macro::instrument(level = "trace")] +#[mirrord_layer_macro::instrument(level = Level::TRACE, ret)] pub(crate) fn unlinkat(dirfd: RawFd, pathname: Detour, flags: u32) -> Detour<()> { let pathname = pathname?; From e0a9e21f67bfc91eccce4009979624ed504f27b8 Mon Sep 17 00:00:00 2001 From: Facundo Poblete Date: Mon, 6 Jan 2025 12:51:26 -0300 Subject: [PATCH 10/12] Fix && fmt --- mirrord/agent/src/file.rs | 4 +--- mirrord/intproxy/src/proxies/files.rs | 11 ----------- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/mirrord/agent/src/file.rs b/mirrord/agent/src/file.rs index b7c5fb0fdbc..41a314ada0e 100644 --- a/mirrord/agent/src/file.rs +++ b/mirrord/agent/src/file.rs @@ -535,7 +535,7 @@ impl FileManager { } } - #[tracing::instrument(level = Level::TRACE, skip(self))] + #[tracing::instrument(level = Level::TRACE, skip(self))] pub(crate) fn rmdir(&mut self, path: &Path) -> RemoteResult<()> { let path = resolve_path(path, &self.root_path)?; @@ -544,7 +544,6 @@ impl FileManager { #[tracing::instrument(level = Level::TRACE, skip(self))] pub(crate) fn unlink(&mut self, path: &Path) -> RemoteResult<()> { - let path = resolve_path(path, &self.root_path)?; match nix::unistd::unlink(path.as_path()) { @@ -562,7 +561,6 @@ impl FileManager { path: &Path, flags: u32, ) -> RemoteResult<()> { - let path = match dirfd { Some(dirfd) => { let relative_dir = self diff --git a/mirrord/intproxy/src/proxies/files.rs b/mirrord/intproxy/src/proxies/files.rs index 8a52841ed33..59f90dfb099 100644 --- a/mirrord/intproxy/src/proxies/files.rs +++ b/mirrord/intproxy/src/proxies/files.rs @@ -286,7 +286,6 @@ impl FilesProxy { message_id: MessageId, message_bus: &mut MessageBus, ) { - // Not supported in old `mirrord-protocol` versions. if let Err(response) = self.is_request_supported(&request) { message_bus @@ -300,16 +299,6 @@ impl FilesProxy { } match request { - - message_bus - .send(ToLayer { - message_id, - layer_id, - message: ProxyToLayerMessage::File(response), - }) - .await; - } - // Should trigger remote close only when the fd is closed in all layer instances. FileRequest::Close(close) => { if self.remote_files.remove(layer_id, close.fd) { From 778efa0e1a16a5cbed1a75f8535ed2284675f9d8 Mon Sep 17 00:00:00 2001 From: Facundo Poblete Date: Mon, 6 Jan 2025 13:14:10 -0300 Subject: [PATCH 11/12] lint --- mirrord/intproxy/src/proxies/files.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mirrord/intproxy/src/proxies/files.rs b/mirrord/intproxy/src/proxies/files.rs index 59f90dfb099..517c743ce12 100644 --- a/mirrord/intproxy/src/proxies/files.rs +++ b/mirrord/intproxy/src/proxies/files.rs @@ -261,18 +261,18 @@ impl FilesProxy { FileRequest::ReadLink(..) if protocol_version.is_some_and(|version| !READLINK_VERSION.matches(version)) => { - return Err(FileResponse::ReadLink(Err(ResponseError::NotImplemented))); + Err(FileResponse::ReadLink(Err(ResponseError::NotImplemented))) } FileRequest::MakeDir(..) | FileRequest::MakeDirAt(..) if protocol_version.is_some_and(|version| !MKDIR_VERSION.matches(version)) => { - return Err(FileResponse::MakeDir(Err(ResponseError::NotImplemented))); + Err(FileResponse::MakeDir(Err(ResponseError::NotImplemented))) } FileRequest::RemoveDir(..) | FileRequest::Unlink(..) | FileRequest::UnlinkAt(..) if protocol_version .is_some_and(|version: &Version| !RMDIR_VERSION.matches(version)) => { - return Err(FileResponse::RemoveDir(Err(ResponseError::NotImplemented))); + Err(FileResponse::RemoveDir(Err(ResponseError::NotImplemented))) } _ => Ok(()), } From 6a847786f2999e2862b1d23590527b94da8576c7 Mon Sep 17 00:00:00 2001 From: Facundo Poblete Date: Tue, 7 Jan 2025 12:10:43 -0300 Subject: [PATCH 12/12] PR comments --- mirrord/agent/src/file.rs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/mirrord/agent/src/file.rs b/mirrord/agent/src/file.rs index 41a314ada0e..81472385622 100644 --- a/mirrord/agent/src/file.rs +++ b/mirrord/agent/src/file.rs @@ -546,12 +546,8 @@ impl FileManager { pub(crate) fn unlink(&mut self, path: &Path) -> RemoteResult<()> { let path = resolve_path(path, &self.root_path)?; - match nix::unistd::unlink(path.as_path()) { - Ok(_) => Ok(()), - Err(err) => Err(ResponseError::from(std::io::Error::from_raw_os_error( - err as i32, - ))), - } + nix::unistd::unlink(path.as_path()) + .map_err(|error| ResponseError::from(std::io::Error::from_raw_os_error(error as i32))) } #[tracing::instrument(level = Level::TRACE, skip(self))] @@ -584,12 +580,8 @@ impl FileManager { let fd: Option = dirfd.map(|fd| fd as RawFd); - match nix::unistd::unlinkat(fd, path.as_path(), flags) { - Ok(_) => Ok(()), - Err(err) => Err(ResponseError::from(std::io::Error::from_raw_os_error( - err as i32, - ))), - } + nix::unistd::unlinkat(fd, path.as_path(), flags) + .map_err(|error| ResponseError::from(std::io::Error::from_raw_os_error(error as i32))) } pub(crate) fn seek(&mut self, fd: u64, seek_from: SeekFrom) -> RemoteResult {