Merge branch 'main' into codex/viyatb/recreate-bwrap-symlink-aliases

Co-authored-by: Codex noreply@openai.com

codex-rs/linux-sandbox/src/bwrap.rs

@@ -221,12 +221,14 @@ fn create_filesystem_args(
         .collect::<Vec<_>>();
     let unreadable_roots = file_system_sandbox_policy.get_unreadable_roots_with_cwd(cwd);
 
+    let mut logical_readable_roots = Vec::new();
     let mut args = if file_system_sandbox_policy.has_full_disk_read_access() {
         // Read-only root, then mount a minimal device tree.
         // In bubblewrap (`bubblewrap.c`, `SETUP_MOUNT_DEV`), `--dev /dev`
         // creates the standard minimal nodes: null, zero, full, random,
         // urandom, and tty. `/dev` must be mounted before writable roots so
         // explicit `/dev/*` writable binds remain visible.
+        logical_readable_roots.push(PathBuf::from("/"));
         vec![
             "--ro-bind".to_string(),
             "/".to_string(),
@@ -262,6 +264,7 @@ fn create_filesystem_args(
         // the broad read baseline. Explicit unreadable carveouts are
         // re-applied later.
         if readable_roots.iter().any(|root| root == Path::new("/")) {
+            logical_readable_roots.push(PathBuf::from("/"));
             args = vec![
                 "--ro-bind".to_string(),
                 "/".to_string(),
@@ -278,10 +281,13 @@ fn create_filesystem_args(
                 // for their restricted-read bootstrap mount. Plain read-only
                 // roots must stay logical because callers may execute those
                 // paths inside bwrap, such as Bazel runfiles helper binaries.
-                let mount_root = if writable_roots
+                let read_root_bootstraps_writable_root = writable_roots
                     .iter()
-                    .any(|writable_root| root.starts_with(writable_root.root.as_path()))
-                {
+                    .any(|writable_root| root.starts_with(writable_root.root.as_path()));
+                if !read_root_bootstraps_writable_root {
+                    logical_readable_roots.push(root.clone());
+                }
+                let mount_root = if read_root_bootstraps_writable_root {
                     canonical_target_if_symlinked_path(&root).unwrap_or(root)
                 } else {
                     root
@@ -307,6 +313,7 @@ fn create_filesystem_args(
         .iter()
         .map(|path| path.as_path().to_path_buf())
         .collect();
+    let mut emitted_logical_aliases = BTreeSet::new();
     let mut sorted_writable_roots = writable_roots;
     sorted_writable_roots.sort_by_key(|writable_root| path_depth(writable_root.root.as_path()));
     // Mask only the unreadable ancestors that sit outside every writable root.
@@ -333,27 +340,35 @@ fn create_filesystem_args(
             &mut preserved_files,
             unreadable_root,
             &allowed_write_paths,
+            &mut emitted_logical_aliases,
         )?;
     }
 
     for writable_root in &sorted_writable_roots {
         let root = writable_root.root.as_path();
         let symlink_target = canonical_target_if_symlinked_path(root);
+        let mount_root = symlink_target.as_deref().unwrap_or(root);
         // If a denied ancestor was already masked, recreate any missing mount
-        // target parents before binding the narrower writable descendant.
+        // target parents before binding the narrower writable descendant. Use
+        // the actual bwrap destination, not a logical symlink alias path.
         if let Some(masking_root) = unreadable_roots
             .iter()
             .map(AbsolutePathBuf::as_path)
-            .filter(|unreadable_root| root.starts_with(unreadable_root))
+            .filter(|unreadable_root| mount_root.starts_with(unreadable_root))
             .max_by_key(|unreadable_root| path_depth(unreadable_root))
         {
-            append_mount_target_parent_dir_args(&mut args, root, masking_root);
+            append_mount_target_parent_dir_args(&mut args, mount_root, masking_root);
         }
 
-        let mount_root = symlink_target.as_deref().unwrap_or(root);
         args.push("--bind".to_string());
         args.push(path_to_string(mount_root));
         args.push(path_to_string(mount_root));
+        append_logical_symlink_alias_args(
+            &mut args,
+            root,
+            &logical_readable_roots,
+            &mut emitted_logical_aliases,
+        );
 
         let mut read_only_subpaths: Vec<PathBuf> = writable_root
             .read_only_subpaths
@@ -384,6 +399,7 @@ fn create_filesystem_args(
                 &mut preserved_files,
                 &unreadable_root,
                 &allowed_write_paths,
+                &mut emitted_logical_aliases,
             )?;
         }
     }
@@ -405,6 +421,7 @@ fn create_filesystem_args(
             &mut preserved_files,
             &unreadable_root,
             &allowed_write_paths,
+            &mut emitted_logical_aliases,
         )?;
     }
 
@@ -423,6 +440,16 @@ fn path_depth(path: &Path) -> usize {
 }
 
 fn canonical_target_if_symlinked_path(path: &Path) -> Option<PathBuf> {
+    let _ = first_symlink_alias_in_path(path)?;
+    let target = fs::canonicalize(path).ok()?;
+    if target.as_path() == path {
+        None
+    } else {
+        Some(target)
+    }
+}
+
+fn first_symlink_alias_in_path(path: &Path) -> Option<(PathBuf, PathBuf)> {
     let mut current = PathBuf::new();
     for component in path.components() {
         use std::path::Component;
@@ -445,11 +472,11 @@ fn canonical_target_if_symlinked_path(path: &Path) -> Option<PathBuf> {
             Err(_) => return None,
         };
         if metadata.file_type().is_symlink() {
-            let target = fs::canonicalize(path).ok()?;
-            if target.as_path() == path {
+            let target = fs::canonicalize(&current).ok()?;
+            if target.as_path() == current {
                 return None;
             }
-            return Some(target);
+            return Some((current, target));
         }
     }
     None
@@ -474,6 +501,64 @@ fn normalize_command_cwd_for_bwrap(command_cwd: &Path) -> PathBuf {
         .unwrap_or_else(|_| command_cwd.to_path_buf())
 }
 
+fn append_logical_symlink_alias_args(
+    args: &mut Vec<String>,
+    writable_root: &Path,
+    logical_readable_roots: &[PathBuf],
+    emitted_logical_aliases: &mut BTreeSet<PathBuf>,
+) {
+    let Some((alias_path, target)) = first_symlink_alias_in_path(writable_root) else {
+        return;
+    };
+    if logical_readable_roots
+        .iter()
+        .any(|root| alias_path.starts_with(root))
+    {
+        return;
+    }
+
+    append_symlink_alias_args(
+        args,
+        alias_path,
+        target,
+        Path::new("/"),
+        emitted_logical_aliases,
+    );
+}
+
+fn append_symlink_alias_args(
+    args: &mut Vec<String>,
+    alias_path: PathBuf,
+    target: PathBuf,
+    parent_anchor: &Path,
+    emitted_logical_aliases: &mut BTreeSet<PathBuf>,
+) {
+    if !emitted_logical_aliases.insert(alias_path.clone()) {
+        return;
+    }
+
+    append_parent_dir_args_for_path(args, &alias_path, parent_anchor);
+    args.push("--symlink".to_string());
+    args.push(path_to_string(&target));
+    args.push(path_to_string(&alias_path));
+}
+
+fn append_parent_dir_args_for_path(args: &mut Vec<String>, path: &Path, anchor: &Path) {
+    let Some(parent) = path.parent() else {
+        return;
+    };
+    let mut parent_dirs: Vec<PathBuf> = parent
+        .ancestors()
+        .take_while(|path| *path != anchor)
+        .map(Path::to_path_buf)
+        .collect();
+    parent_dirs.reverse();
+    for parent_dir in parent_dirs {
+        args.push("--dir".to_string());
+        args.push(path_to_string(&parent_dir));
+    }
+}
+
 fn append_mount_target_parent_dir_args(args: &mut Vec<String>, mount_target: &Path, anchor: &Path) {
     let mount_target_dir = if mount_target.is_dir() {
         mount_target
@@ -534,6 +619,7 @@ fn append_unreadable_root_args(
     preserved_files: &mut Vec<File>,
     unreadable_root: &Path,
     allowed_write_paths: &[PathBuf],
+    emitted_logical_aliases: &mut BTreeSet<PathBuf>,
 ) -> Result<()> {
     if let Some(target) = canonical_target_for_symlink_in_path(unreadable_root, allowed_write_paths)
     {
@@ -544,6 +630,7 @@ fn append_unreadable_root_args(
             preserved_files,
             &target,
             allowed_write_paths,
+            emitted_logical_aliases,
         );
     }
 
@@ -563,6 +650,7 @@ fn append_unreadable_root_args(
         preserved_files,
         unreadable_root,
         allowed_write_paths,
+        emitted_logical_aliases,
     )
 }
 
@@ -571,6 +659,7 @@ fn append_existing_unreadable_path_args(
     preserved_files: &mut Vec<File>,
     unreadable_root: &Path,
     allowed_write_paths: &[PathBuf],
+    emitted_logical_aliases: &mut BTreeSet<PathBuf>,
 ) -> Result<()> {
     if unreadable_root.is_dir() {
         let mut writable_descendants: Vec<&Path> = allowed_write_paths
@@ -595,6 +684,17 @@ fn append_existing_unreadable_path_args(
         // nested mount targets after the parent has been frozen.
         writable_descendants.sort_by_key(|path| path_depth(path));
         for writable_descendant in writable_descendants {
+            // If the writable descendant is addressed through a symlink inside
+            // this tmpfs, create the symlink now. Creating directories on the
+            // logical path would block the later symlink alias with EEXIST.
+            if append_symlink_alias_for_masked_writable_descendant(
+                args,
+                writable_descendant,
+                unreadable_root,
+                emitted_logical_aliases,
+            ) {
+                continue;
+            }
             append_mount_target_parent_dir_args(args, writable_descendant, unreadable_root);
         }
         args.push("--remount-ro".to_string());
@@ -614,6 +714,30 @@ fn append_existing_unreadable_path_args(
     Ok(())
 }
 
+fn append_symlink_alias_for_masked_writable_descendant(
+    args: &mut Vec<String>,
+    writable_descendant: &Path,
+    unreadable_root: &Path,
+    emitted_logical_aliases: &mut BTreeSet<PathBuf>,
+) -> bool {
+    let Some((alias_path, target)) = first_symlink_alias_in_path(writable_descendant) else {
+        return false;
+    };
+    // Only this unreadable tmpfs can safely create aliases it contains.
+    if !alias_path.starts_with(unreadable_root) {
+        return false;
+    }
+
+    append_symlink_alias_args(
+        args,
+        alias_path,
+        target,
+        unreadable_root,
+        emitted_logical_aliases,
+    );
+    true
+}
+
 /// Returns true when `path` is under any allowed writable root.
 fn is_within_allowed_write_paths(path: &Path, allowed_write_paths: &[PathBuf]) -> bool {
     allowed_write_paths
@@ -922,6 +1046,243 @@ mod tests {
         }));
     }
 
+    #[cfg(unix)]
+    #[test]
+    fn symlinked_writable_roots_recreate_logical_alias_when_parent_is_not_readable() {
+        let temp_dir = TempDir::new().expect("temp dir");
+        let temp_root = temp_dir.path().canonicalize().expect("canonical temp dir");
+        let real_root = temp_root.join("real");
+        let link_root = temp_root.join("link");
+        std::fs::create_dir_all(&real_root).expect("create real root");
+        std::os::unix::fs::symlink(&real_root, &link_root).expect("create symlinked root");
+
+        let link_root =
+            AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root");
+        let real_root_str = path_to_string(&real_root);
+        let link_root_str = path_to_string(link_root.as_path());
+        let policy = FileSystemSandboxPolicy::restricted(vec![
+            FileSystemSandboxEntry {
+                path: FileSystemPath::Special {
+                    value: FileSystemSpecialPath::Minimal,
+                },
+                access: FileSystemAccessMode::Read,
+            },
+            FileSystemSandboxEntry {
+                path: FileSystemPath::Path { path: link_root },
+                access: FileSystemAccessMode::Write,
+            },
+        ]);
+
+        let args = create_filesystem_args(&policy, temp_dir.path()).expect("filesystem args");
+        let bind_index = args
+            .args
+            .windows(3)
+            .position(|window| window == ["--bind", real_root_str.as_str(), real_root_str.as_str()])
+            .expect("writable root should bind real target");
+        let symlink_index = args
+            .args
+            .windows(3)
+            .position(|window| {
+                window == ["--symlink", real_root_str.as_str(), link_root_str.as_str()]
+            })
+            .expect("logical symlink alias should be recreated");
+
+        assert!(
+            bind_index < symlink_index,
+            "logical alias must point at the mounted writable target: {:#?}",
+            args.args
+        );
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn symlinked_writable_roots_deduplicate_shared_logical_alias() {
+        let temp_dir = TempDir::new().expect("temp dir");
+        let temp_root = temp_dir.path().canonicalize().expect("canonical temp dir");
+        let real_root = temp_root.join("real");
+        let real_a = real_root.join("a");
+        let real_b = real_root.join("b");
+        let link_root = temp_root.join("link");
+        let link_a = link_root.join("a");
+        let link_b = link_root.join("b");
+        std::fs::create_dir_all(&real_a).expect("create real a");
+        std::fs::create_dir_all(&real_b).expect("create real b");
+        std::os::unix::fs::symlink(&real_root, &link_root).expect("create symlinked root");
+
+        let link_a = AbsolutePathBuf::from_absolute_path(&link_a).expect("absolute link a");
+        let link_b = AbsolutePathBuf::from_absolute_path(&link_b).expect("absolute link b");
+        let real_root_str = path_to_string(&real_root);
+        let link_root_str = path_to_string(&link_root);
+        let policy = FileSystemSandboxPolicy::restricted(vec![
+            FileSystemSandboxEntry {
+                path: FileSystemPath::Special {
+                    value: FileSystemSpecialPath::Minimal,
+                },
+                access: FileSystemAccessMode::Read,
+            },
+            FileSystemSandboxEntry {
+                path: FileSystemPath::Path { path: link_a },
+                access: FileSystemAccessMode::Write,
+            },
+            FileSystemSandboxEntry {
+                path: FileSystemPath::Path { path: link_b },
+                access: FileSystemAccessMode::Write,
+            },
+        ]);
+
+        let args = create_filesystem_args(&policy, temp_dir.path()).expect("filesystem args");
+        let symlink_count = args
+            .args
+            .windows(3)
+            .filter(|window| {
+                *window == ["--symlink", real_root_str.as_str(), link_root_str.as_str()]
+            })
+            .count();
+
+        assert_eq!(
+            symlink_count, 1,
+            "logical symlink aliases should be emitted once: {:#?}",
+            args.args
+        );
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn symlinked_writable_root_under_unreadable_parent_recreates_alias_before_remount() {
+        let temp_dir = TempDir::new().expect("temp dir");
+        let temp_root = temp_dir.path().canonicalize().expect("canonical temp dir");
+        let blocked = temp_root.join("blocked");
+        let real_root = temp_root.join("real");
+        let real_allowed = real_root.join("allowed");
+        let link_root = blocked.join("link");
+        let link_allowed = link_root.join("allowed");
+        std::fs::create_dir_all(&blocked).expect("create blocked dir");
+        std::fs::create_dir_all(&real_allowed).expect("create real allowed dir");
+        std::os::unix::fs::symlink(&real_root, &link_root).expect("create symlinked root");
+
+        let blocked = AbsolutePathBuf::from_absolute_path(&blocked).expect("absolute blocked");
+        let link_allowed =
+            AbsolutePathBuf::from_absolute_path(&link_allowed).expect("absolute link allowed");
+        let blocked_str = path_to_string(blocked.as_path());
+        let real_root_str = path_to_string(&real_root);
+        let real_allowed_str = path_to_string(&real_allowed);
+        let link_root_str = path_to_string(&link_root);
+        let link_allowed_str = path_to_string(link_allowed.as_path());
+        let policy = FileSystemSandboxPolicy::restricted(vec![
+            FileSystemSandboxEntry {
+                path: FileSystemPath::Special {
+                    value: FileSystemSpecialPath::Minimal,
+                },
+                access: FileSystemAccessMode::Read,
+            },
+            FileSystemSandboxEntry {
+                path: FileSystemPath::Path { path: blocked },
+                access: FileSystemAccessMode::None,
+            },
+            FileSystemSandboxEntry {
+                path: FileSystemPath::Path { path: link_allowed },
+                access: FileSystemAccessMode::Write,
+            },
+        ]);
+
+        let args = create_filesystem_args(&policy, temp_dir.path()).expect("filesystem args");
+        let tmpfs_index = args
+            .args
+            .windows(4)
+            .position(|window| window == ["--perms", "111", "--tmpfs", blocked_str.as_str()])
+            .expect("unreadable parent should become an executable tmpfs");
+        let symlink_index = args
+            .args
+            .windows(3)
+            .position(|window| {
+                window == ["--symlink", real_root_str.as_str(), link_root_str.as_str()]
+            })
+            .expect("logical symlink alias should be created inside tmpfs");
+        let remount_index = args
+            .args
+            .windows(2)
+            .position(|window| window == ["--remount-ro", blocked_str.as_str()])
+            .expect("unreadable parent should be remounted read-only");
+
+        assert!(
+            tmpfs_index < symlink_index && symlink_index < remount_index,
+            "logical alias must be created before freezing the unreadable parent: {:#?}",
+            args.args
+        );
+        assert!(args.args.windows(3).any(|window| {
+            window
+                == [
+                    "--bind",
+                    real_allowed_str.as_str(),
+                    real_allowed_str.as_str(),
+                ]
+        }));
+        assert!(
+            !args
+                .args
+                .windows(2)
+                .any(|window| window == ["--dir", link_root_str.as_str()]),
+            "logical alias path must not be pre-created as a directory: {:#?}",
+            args.args
+        );
+        assert!(
+            !args
+                .args
+                .windows(2)
+                .any(|window| window == ["--dir", link_allowed_str.as_str()]),
+            "writable child under the alias must not be pre-created as a directory: {:#?}",
+            args.args
+        );
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn symlinked_writable_roots_skip_logical_alias_when_parent_is_readable() {
+        let temp_dir = TempDir::new().expect("temp dir");
+        let temp_root = temp_dir.path().canonicalize().expect("canonical temp dir");
+        let parent = temp_root.join("parent");
+        let real_root = temp_root.join("real");
+        let link_root = parent.join("link");
+        std::fs::create_dir_all(&parent).expect("create parent");
+        std::fs::create_dir_all(&real_root).expect("create real root");
+        std::os::unix::fs::symlink(&real_root, &link_root).expect("create symlinked root");
+
+        let parent = AbsolutePathBuf::from_absolute_path(&parent).expect("absolute parent");
+        let link_root =
+            AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root");
+        let parent_str = path_to_string(parent.as_path());
+        let real_root_str = path_to_string(&real_root);
+        let link_root_str = path_to_string(link_root.as_path());
+        let policy = FileSystemSandboxPolicy::restricted(vec![
+            FileSystemSandboxEntry {
+                path: FileSystemPath::Path { path: parent },
+                access: FileSystemAccessMode::Read,
+            },
+            FileSystemSandboxEntry {
+                path: FileSystemPath::Path { path: link_root },
+                access: FileSystemAccessMode::Write,
+            },
+        ]);
+
+        let args = create_filesystem_args(&policy, temp_dir.path()).expect("filesystem args");
+
+        assert!(
+            args.args.windows(3).any(|window| {
+                window == ["--ro-bind", parent_str.as_str(), parent_str.as_str()]
+            })
+        );
+        assert!(args.args.windows(3).any(|window| {
+            window == ["--bind", real_root_str.as_str(), real_root_str.as_str()]
+        }));
+        assert!(
+            !args.args.windows(3).any(|window| {
+                window == ["--symlink", real_root_str.as_str(), link_root_str.as_str()]
+            }),
+            "readable parent already exposes the logical symlink: {:#?}",
+            args.args
+        );
+    }
+
     #[cfg(unix)]
     #[test]
     fn protected_symlinked_directory_subpaths_bind_target_read_only() {