Repository: metalbear-co/mirrord (language: rust)

Issue:
Wrong `ensure_not_ignored!` calls
Some of the calls to `ensure_not_ignored!` pass `false` for write accesses, which I think is wrong.
Let's go over all the calls to `ensure_not_ignored!` and make sure they're all correct.

Some examples:

https://github.com/metalbear-co/mirrord/blob/971c6caa0d2e6b28433b08ddddaf2aaadec365b7/mirrord/layer/src/file/ops.rs#L350-L358


https://github.com/metalbear-co/mirrord/blob/971c6caa0d2e6b28433b08ddddaf2aaadec365b7/mirrord/layer/src/file/ops.rs#L400-L408




Discussion / hints:
<p><a href="https://linear.app/metalbear/issue/MBE-719/wrong-ensure-not-ignored-calls">MBE-719 Wrong `ensure_not_ignored!` calls</a></p>

Fix patch:
diff --git a/changelog.d/3083.fixed.md b/changelog.d/3083.fixed.md
new file mode 100644
index 00000000000..1cf29341f87
--- /dev/null
+++ b/changelog.d/3083.fixed.md
@@ -0,0 +1,1 @@
+Some FS libc calls could be carried out remotely instead of locally in some cases.
diff --git a/mirrord/layer/src/file/filter.rs b/mirrord/layer/src/file/filter.rs
index 8191522d17a..e02c11e6e59 100644
--- a/mirrord/layer/src/file/filter.rs
+++ b/mirrord/layer/src/file/filter.rs
@@ -126,7 +126,7 @@ impl FileFilter {
     }
 
     /// Checks if `text` matches the regex held by the initialized variant of `FileFilter`,
-    /// and the whether the path is queried for write converting the result a `Detour`.
+    /// and whether the path is queried for write converting the result a `Detour`.
     ///
     /// `op` is used to lazily initialize a `Bypass` case.
     pub fn continue_or_bypass_with<F>(&self, text: &str, write: bool, op: F) -> Detour<()>
diff --git a/mirrord/layer/src/file/hooks.rs b/mirrord/layer/src/file/hooks.rs
index bc30f54a6b2..3a75e9e2153 100644
--- a/mirrord/layer/src/file/hooks.rs
+++ b/mirrord/layer/src/file/hooks.rs
@@ -662,7 +662,7 @@ pub(crate) unsafe extern "C" fn _write_nocancel_detour(
 
 /// Implementation of access_detour, used in access_detour and faccessat_detour
 unsafe fn access_logic(raw_path: *const c_char, mode: c_int) -> c_int {
-    access(raw_path.checked_into(), mode as u8).unwrap_or_bypass_with(|bypass| {
+    access(raw_path.checked_into(), mode).unwrap_or_bypass_with(|bypass| {
         let raw_path = update_ptr_from_bypass(raw_path, &bypass);
         FN_ACCESS(raw_path, mode)
     })
diff --git a/mirrord/layer/src/file/ops.rs b/mirrord/layer/src/file/ops.rs
index d894fcd5e45..11650051dfe 100644
--- a/mirrord/layer/src/file/ops.rs
+++ b/mirrord/layer/src/file/ops.rs
@@ -355,7 +355,7 @@ pub(crate) fn mkdir(pathname: Detour<PathBuf>, mode: u32) -> Detour<()> {
 
     let path = remap_path!(pathname);
 
-    ensure_not_ignored!(path, false);
+    ensure_not_ignored!(path, true);
 
     let mkdir = MakeDirRequest {
         pathname: path,
@@ -376,6 +376,9 @@ pub(crate) fn mkdirat(dirfd: RawFd, pathname: Detour<PathBuf>, mode: u32) -> Det
 
     if pathname.is_absolute() || dirfd == AT_FDCWD {
         let path = remap_path!(pathname);
+        check_relative_paths!(path);
+        ensure_not_ignored!(path, true);
+
         mkdir(Detour::Success(path), mode)
     } else {
         // Relative path requires special handling, we must identify the relative part (relative to
@@ -405,7 +408,7 @@ pub(crate) fn rmdir(pathname: Detour<PathBuf>) -> Detour<()> {
 
     let path = remap_path!(pathname);
 
-    ensure_not_ignored!(path, false);
+    ensure_not_ignored!(path, true);
 
     let rmdir = RemoveDirRequest { pathname: path };
 
@@ -425,7 +428,7 @@ pub(crate) fn unlink(pathname: Detour<PathBuf>) -> Detour<()> {
 
     let path = remap_path!(pathname);
 
-    ensure_not_ignored!(path, false);
+    ensure_not_ignored!(path, true);
 
     let unlink = RemoveDirRequest { pathname: path };
 
@@ -441,15 +444,23 @@ pub(crate) fn unlink(pathname: Detour<PathBuf>) -> Detour<()> {
 pub(crate) fn unlinkat(dirfd: RawFd, pathname: Detour<PathBuf>, flags: u32) -> Detour<()> {
     let pathname = pathname?;
 
-    let optional_dirfd = match pathname.is_absolute() {
-        true => None,
-        false => Some(get_remote_fd(dirfd)?),
-    };
+    let unlink = if pathname.is_absolute() || dirfd == AT_FDCWD {
+        let path = remap_path!(pathname);
+        check_relative_paths!(path);
+        ensure_not_ignored!(path, true);
+        UnlinkAtRequest {
+            dirfd: None,
+            pathname: path,
+            flags,
+        }
+    } else {
+        let remote_fd = get_remote_fd(dirfd)?;
 
-    let unlink = UnlinkAtRequest {
-        dirfd: optional_dirfd,
-        pathname: pathname.clone(),
-        flags,
+        UnlinkAtRequest {
+            dirfd: Some(remote_fd),
+            pathname: pathname.clone(),
+            flags,
+        }
     };
 
     // `NotImplemented` error here means that the protocol doesn't support it.
@@ -518,18 +529,25 @@ pub(crate) fn write(local_fd: RawFd, write_bytes: Option<Vec<u8>>) -> Detour<isi
 }
 
 #[mirrord_layer_macro::instrument(level = "trace")]
-pub(crate) fn access(path: Detour<PathBuf>, mode: u8) -> Detour<c_int> {
+pub(crate) fn access(path: Detour<PathBuf>, mode: c_int) -> Detour<c_int> {
     let path = path?;
 
     check_relative_paths!(path);
 
     let path = remap_path!(path);
 
-    ensure_not_ignored!(path, false);
+    // Is the caller asking about write access to the file?
+    let is_write = (mode & libc::W_OK) != 0;
+
+    // Even though `access` is never a write operation (even if mode is write), we take the mode
+    // into account when deciding whether to ignore, because when a caller is asking whether they
+    // have write access to a file and then write to it, we want the test and the actual write to
+    // happen with the same file.
+    ensure_not_ignored!(path, is_write);
 
     let access = AccessFileRequest {
         pathname: path,
-        mode,
+        mode: mode as u8,
     };
 
     let _ = common::make_proxy_request_with_response(access)??;


Test patch:
diff --git a/mirrord/layer/tests/apps/faccessat_go/main.go b/mirrord/layer/tests/apps/faccessat_go/main.go
index 5c8b34a5e6d..312c6832dfd 100644
--- a/mirrord/layer/tests/apps/faccessat_go/main.go
+++ b/mirrord/layer/tests/apps/faccessat_go/main.go
@@ -3,11 +3,21 @@ package main
 import (
 	"C"
 	"syscall"
+	"os"
 )
 
-const TEXT = "Pineapples."
-
 func main() {
+
+	// First make a call that should be bypassed.
+	localHomeDir, homeDirErr := os.UserHomeDir()
+	if homeDirErr != nil {
+		panic(homeDirErr)
+	}
+	localErr := syscall.Access(localHomeDir, syscall.O_RDWR)
+	if localErr != nil {
+		panic(localErr)
+	}
+
 	// Tests: SYS_faccessat
 	// Access calls Faccess with _AT_FDCWD and flags set to 0
 	err := syscall.Access("/app/test.txt", syscall.O_RDWR)
diff --git a/mirrord/layer/tests/apps/mkdir/mkdir.c b/mirrord/layer/tests/apps/mkdir/mkdir.c
deleted file mode 100644
index 8631ae7a26a..00000000000
--- a/mirrord/layer/tests/apps/mkdir/mkdir.c
+++ /dev/null
@@ -1,21 +0,0 @@
-#include <assert.h>
-#include <stdio.h>
-#include <sys/stat.h>
-#include <sys/fcntl.h>
-
-/// Test `mkdir`.
-///
-/// Creates a folder in the current directory.
-///
-int main()
-{
-  char *mkdir_test_path = "/mkdir_test_path";
-  int mkdir_result = mkdir(mkdir_test_path, 0777);
-  assert(mkdir_result == 0);
-
-  char *mkdirat_test_path = "/mkdirat_test_path";
-  int mkdirat_result = mkdirat(AT_FDCWD, mkdirat_test_path, 0777);
-  assert(mkdirat_result == 0);
-
-  return 0;
-}
diff --git a/mirrord/layer/tests/apps/mkdir_rmdir/mkdir_rmdir.c b/mirrord/layer/tests/apps/mkdir_rmdir/mkdir_rmdir.c
new file mode 100644
index 00000000000..59d382c9410
--- /dev/null
+++ b/mirrord/layer/tests/apps/mkdir_rmdir/mkdir_rmdir.c
@@ -0,0 +1,63 @@
+#include <assert.h>
+#include <stdio.h>
+#include <sys/fcntl.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+
+/// Test `mkdir`, `mkdirat` and `rmdir`.
+///
+/// For each of `mkdir` and `mkdirat`:
+/// - creates a folder in a path that mirrord should ignore (bypass and do locally).
+/// - deletes that folder (should happen locally as well).
+/// - creates a folder in a path that mirrord should handle remotely.
+/// - deletes that folder (should happen remotely as well).
+///
+int main()
+{
+  // First create and delete a dir that should be created and deleted locally and therefore
+  // bypassed in the layer.
+  // This should not result in any client message.
+  char *ignored_dir = "/tmp/rmdir_test_dir";
+
+  // CREATE dir (should happen locally)
+  int local_mkdir_result = mkdir(ignored_dir, 0777);
+  assert(local_mkdir_result == 0);
+
+  // DELETE dir (should happen locally)
+  int local_rmdir_result = rmdir(ignored_dir);
+  assert(local_rmdir_result == 0);
+
+  char *ignored_mkdirat_test_path = "/tmp/mkdirat_test_path";
+
+  // CREATE dir (should happen locally)
+  int local_mkdirat_result = mkdirat(AT_FDCWD, ignored_mkdirat_test_path, 0777);
+  assert(local_mkdirat_result == 0);
+
+  // DELETE dir (should happen locally)
+  int local_rmdir_mkdirat_result = rmdir(ignored_mkdirat_test_path);
+  assert(local_rmdir_mkdirat_result == 0);
+
+  // This should result in client messages.
+  char *test_dir = "/mkdir_rmdir_test_dir";
+
+  // CREATE dir (should happen remotely)
+  int mkdir_result = mkdir(test_dir, 0777);
+  assert(mkdir_result == 0);
+
+  // DELETE dir (should happen remotely)
+  int rmdir_result = rmdir(test_dir);
+  assert(rmdir_result == 0);
+
+  char *mkdirat_test_path = "/mkdirat_rmdir_test_dir";
+
+  // CREATE dir (should happen remotely)
+  int mkdirat_result = mkdirat(AT_FDCWD, mkdirat_test_path, 0777);
+  assert(mkdirat_result == 0);
+
+  // DELETE dir (should happen remotely)
+  int rmdir_mkdirat_result = rmdir(mkdirat_test_path);
+  assert(rmdir_mkdirat_result == 0);
+
+  return 0;
+}
diff --git a/mirrord/layer/tests/apps/rmdir/rmdir.c b/mirrord/layer/tests/apps/rmdir/rmdir.c
deleted file mode 100644
index f839e5256e7..00000000000
--- a/mirrord/layer/tests/apps/rmdir/rmdir.c
+++ /dev/null
@@ -1,20 +0,0 @@
-#include <assert.h>
-#include <stdio.h>
-#include <unistd.h>
-#include <sys/stat.h>
-
-/// Test `rmdir`.
-///
-/// Creates a folder and then removes it.
-///
-int main()
-{
-  char *test_dir = "/test_dir";
-  int mkdir_result = mkdir(test_dir, 0777);
-  assert(mkdir_result == 0);
-
-  int rmdir_result = rmdir(test_dir);
-  assert(rmdir_result == 0);
-
-  return 0;
-}
diff --git a/mirrord/layer/tests/apps/statfs_fstatfs/statfs_fstatfs.c b/mirrord/layer/tests/apps/statfs_fstatfs/statfs_fstatfs.c
index 6c474cce4e3..0cb103d5242 100644
--- a/mirrord/layer/tests/apps/statfs_fstatfs/statfs_fstatfs.c
+++ b/mirrord/layer/tests/apps/statfs_fstatfs/statfs_fstatfs.c
@@ -20,7 +20,6 @@
 int main()
 {
   char *tmp_test_path = "/statfs_fstatfs_test_path";
-  mkdir(tmp_test_path, 0777);
 
   // statfs
   struct statfs statfs_buf;
diff --git a/mirrord/layer/tests/common/mod.rs b/mirrord/layer/tests/common/mod.rs
index ac47dbe5f51..2d20eb7682c 100644
--- a/mirrord/layer/tests/common/mod.rs
+++ b/mirrord/layer/tests/common/mod.rs
@@ -826,9 +826,8 @@ pub enum Application {
     RustListenPorts,
     Fork,
     ReadLink,
-    MakeDir,
     StatfsFstatfs,
-    RemoveDir,
+    MkdirRmdir,
     OpenFile,
     CIssue2055,
     CIssue2178,
@@ -884,9 +883,8 @@ impl Application {
             Application::PythonFastApiHTTP | Application::PythonIssue864 => String::from("uvicorn"),
             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::StatfsFstatfs => String::from("tests/apps/statfs_fstatfs/out.c_test_app"),
-            Application::RemoveDir => String::from("tests/apps/rmdir/out.c_test_app"),
+            Application::MkdirRmdir => String::from("tests/apps/mkdir_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")
@@ -1124,9 +1122,8 @@ impl Application {
             | Application::Go23FAccessAt
             | Application::Fork
             | Application::ReadLink
-            | Application::MakeDir
             | Application::StatfsFstatfs
-            | Application::RemoveDir
+            | Application::MkdirRmdir
             | Application::Realpath
             | Application::RustFileOps
             | Application::RustIssue1123
@@ -1204,9 +1201,8 @@ impl Application {
             | Application::BashShebang
             | Application::Fork
             | Application::ReadLink
-            | Application::MakeDir
             | Application::StatfsFstatfs
-            | Application::RemoveDir
+            | Application::MkdirRmdir
             | Application::Realpath
             | Application::Go21Issue834
             | Application::Go22Issue834
diff --git a/mirrord/layer/tests/configs/fs.json b/mirrord/layer/tests/configs/fs.json
new file mode 100644
index 00000000000..f6455e78356
--- /dev/null
+++ b/mirrord/layer/tests/configs/fs.json
@@ -0,0 +1,10 @@
+{
+  "feature": {
+    "fs": {
+      "read_write": [
+        "^/mkdir_rmdir_test_dir$",
+        "^/mkdirat_rmdir_test_dir$"
+      ]
+    }
+  }
+}
\ No newline at end of file
diff --git a/mirrord/layer/tests/mkdir.rs b/mirrord/layer/tests/mkdir.rs
deleted file mode 100644
index 76fa3e8936b..00000000000
--- a/mirrord/layer/tests/mkdir.rs
+++ /dev/null
@@ -1,31 +0,0 @@
-#![feature(assert_matches)]
-use std::{path::Path, time::Duration};
-
-use rstest::rstest;
-
-mod common;
-pub use common::*;
-
-/// Test for the [`libc::mkdir`] function.
-#[rstest]
-#[tokio::test]
-#[timeout(Duration::from_secs(60))]
-async fn mkdir(dylib_path: &Path) {
-    let application = Application::MakeDir;
-
-    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("/mkdir_test_path", 0o777).await;
-
-    println!("waiting for MakeDirRequest.");
-    intproxy.expect_make_dir("/mkdirat_test_path", 0o777).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/layer/tests/mkdir_rmdir.rs b/mirrord/layer/tests/mkdir_rmdir.rs
new file mode 100644
index 00000000000..8c0cf2e2483
--- /dev/null
+++ b/mirrord/layer/tests/mkdir_rmdir.rs
@@ -0,0 +1,45 @@
+#![feature(assert_matches)]
+use std::{path::Path, time::Duration};
+
+use rstest::rstest;
+
+mod common;
+pub use common::*;
+
+/// Test for the [`libc::mkdir`], [`libc::mkdirat`] and [`libc::rmdir`] hooks.
+#[rstest]
+#[tokio::test]
+#[timeout(Duration::from_secs(60))]
+async fn mkdir_rmdir(dylib_path: &Path, config_dir: &Path) {
+    let _tracing = init_tracing();
+    let application = Application::MkdirRmdir;
+    let mut config_path = config_dir.to_path_buf();
+    config_path.push("fs.json");
+    let config_path = Some(config_path.to_str().unwrap());
+
+    let (mut test_process, mut intproxy) = application
+        .start_process_with_layer(dylib_path, Default::default(), config_path)
+        .await;
+
+    println!("waiting for MakeDirRequest.");
+    intproxy
+        .expect_make_dir("/mkdir_rmdir_test_dir", 0o777)
+        .await;
+
+    println!("waiting for RemoveDirRequest.");
+    intproxy.expect_remove_dir("/mkdir_rmdir_test_dir").await;
+
+    println!("waiting for MakeDirRequest.");
+    intproxy
+        .expect_make_dir("/mkdirat_rmdir_test_dir", 0o777)
+        .await;
+
+    println!("waiting for RemoveDirRequest.");
+    intproxy.expect_remove_dir("/mkdirat_rmdir_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/layer/tests/rmdir.rs b/mirrord/layer/tests/rmdir.rs
deleted file mode 100644
index 910e3d18ff9..00000000000
--- a/mirrord/layer/tests/rmdir.rs
+++ /dev/null
@@ -1,31 +0,0 @@
-#![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/layer/tests/statfs_fstatfs.rs b/mirrord/layer/tests/statfs_fstatfs.rs
index 38f48c8495f..54700cdda1b 100644
--- a/mirrord/layer/tests/statfs_fstatfs.rs
+++ b/mirrord/layer/tests/statfs_fstatfs.rs
@@ -10,18 +10,13 @@ pub use common::*;
 #[rstest]
 #[tokio::test]
 #[timeout(Duration::from_secs(60))]
-async fn mkdir(dylib_path: &Path) {
+async fn statfs(dylib_path: &Path) {
     let application = Application::StatfsFstatfs;
 
     let (mut test_process, mut intproxy) = application
         .start_process_with_layer(dylib_path, Default::default(), None)
         .await;
 
-    println!("waiting for file request (mkdir).");
-    intproxy
-        .expect_make_dir("/statfs_fstatfs_test_path", 0o777)
-        .await;
-
     println!("waiting for file request (statfs).");
     intproxy.expect_statfs("/statfs_fstatfs_test_path").await;
 
diff --git a/tests/src/file_ops.rs b/tests/src/file_ops.rs
index 69512f9ffa7..a213e59889b 100644
--- a/tests/src/file_ops.rs
+++ b/tests/src/file_ops.rs
@@ -181,9 +181,9 @@ mod file_ops_tests {
         assert!(res.success());
     }
 
-    /// On Linux: Test our getdents64 Go syscall hook, for `os.ReadDir` on go.
+    /// Test our getdents64 Go syscall hook, for `os.ReadDir` on go, and mkdir and rmdir.
     /// This is an E2E test and not an integration test in order to test the agent side of the
-    /// detour.
+    /// detours.
     #[cfg_attr(not(any(feature = "ephemeral", feature = "job")), ignore)]
     #[rstest]
     #[trace]
@@ -198,7 +198,7 @@ mod file_ops_tests {
         let service = service.await;
         let command = ops.command();
 
-        let mut args = vec!["--fs-mode", "read"];
+        let mut args = Vec::new();
 
         if cfg!(feature = "ephemeral") {
             args.extend(["-e"].into_iter());
@@ -209,7 +209,10 @@ mod file_ops_tests {
             &service.pod_container_target(),
             Some(&service.namespace),
             Some(args),
-            None,
+            Some(vec![(
+                "MIRRORD_FILE_READ_WRITE_PATTERN",
+                "^/app/test_mkdir$",
+            )]),
         )
         .await;
         let res = process.wait().await;