Skip to content

fs/macos: preserve the inode a rename overwrites #700

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
acdtrx wants to merge 1 commit into
base: main
Choose a base branch
from
+61 −2
Open

fs/macos: preserve the inode a rename overwrites #700

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 61 additions & 2 deletions src/devices/src/virtio/fs/macos/passthrough.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,7 +825,7 @@ impl PassthroughFs {
Ok(fd)
}

fn store_unlinked_fd(&self, unlinked_fd: RawFd) -> io::Result<()> {
fn store_unlinked_fd(&self, unlinked_fd: RawFd) -> io::Result<bool> {
let st = fstat(unlinked_fd, true)?;
let altkey = InodeAltKey {
ino: st.st_ino,
Expand All @@ -834,8 +834,13 @@ impl PassthroughFs {
if let Some(data) = self.inodes.read().unwrap().get_alt(&altkey).cloned() {
data.unlinked_fd
.store(unlinked_fd as i64, Ordering::Release);
// The tracked inode now owns `unlinked_fd` (closed in `forget_one`).
Ok(true)
Comment on lines 835 to +838
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

1. Missed Update in do_unlink (Critical Bug)

Changing store_unlinked_fd to return io::Result<bool> requires all callers to handle the bool return value to avoid leaking the file descriptor when there is no tracked inode. While the rename path was updated correctly, do_unlink (around line 892) was missed and still ignores the return value:

if let Err(err) = self.store_unlinked_fd(unlinked_fd) {
    unsafe { libc::close(unlinked_fd) };
    warn!("Couldn't store unlinked fd \"{}\": {err}", unlinked_fd);
}

Since Ok(false) is not an Err, the file descriptor is leaked in the unlink path when there is no tracked inode. Please update do_unlink to handle the return value as follows:

match self.store_unlinked_fd(unlinked_fd) {
    Ok(true) => {}
    Ok(false) => {
        unsafe { libc::close(unlinked_fd) };
    }
    Err(err) => {
        unsafe { libc::close(unlinked_fd) };
        warn!("Couldn't store unlinked fd \"{}\": {err}", unlinked_fd);
    }
}

2. Preventing Double-Store Leak

If store_unlinked_fd is called multiple times for the same tracked inode (e.g., when multiple hard links are unlinked or overwritten), the previous file descriptor stored in unlinked_fd will be silently overwritten and leaked. Using swap instead of store allows us to retrieve and safely close any previously stored file descriptor.

            let old_fd = data.unlinked_fd.swap(unlinked_fd as i64, Ordering::AcqRel);
            if old_fd >= 0 {
                unsafe { libc::close(old_fd as RawFd) };
            }
            // The tracked inode now owns unlinked_fd (closed in forget_one).
            Ok(true)

} else {
// No tracked inode for this (dev, ino): the caller keeps ownership
// of `unlinked_fd` and must close it to avoid a leak.
Ok(false)
}
Ok(())
}

fn do_unlink(
Expand Down Expand Up @@ -1659,8 +1664,58 @@ impl FileSystem for PassthroughFs {
let old_cpath = self.name_to_path(olddir, oldname)?;
let new_cpath = self.name_to_path(newdir, newname)?;

// macOS addresses inodes by their volfs path ("/.vol/{dev}/{ino}"),
// which only resolves while the inode still has a directory entry. A
// rename that REPLACES an existing target drops that target's last
// link, so any inode the guest still holds open there would afterwards
// resolve to a dangling volfs path and fail path-based ops
// (getattr/open/setattr/...) with ENOENT (e.g. apt/dpkg's atomic
// rewrite of /var/lib/dpkg/status, surfaced as
// "close (2: No such file or directory)"). `do_unlink` already guards
// the unlink case by stashing an fd to the doomed inode in
// `InodeData.unlinked_fd`; mirror that for the overwritten target. Grab
// it *before* the rename, while its entry still exists. RENAME_SWAP
// keeps both inodes linked and RENAME_EXCL never overwrites, so skip
// those; best-effort otherwise (a non-overwriting rename finds nothing).
let doomed_fd = if (flags as i32)
& (bindings::LINUX_RENAME_EXCHANGE | bindings::LINUX_RENAME_NOREPLACE)
== 0
{
match self.inode_to_handle(newdir, true) {
Ok(InodeHandle::Path(parent_cpath)) => {
let parent_fd = unsafe {
libc::open(parent_cpath.as_ptr(), libc::O_NOFOLLOW | libc::O_CLOEXEC)
};
if parent_fd < 0 {
None
} else {
let grabbed = self.grab_unlinked_fd(parent_fd, newname).ok();
unsafe { libc::close(parent_fd) };
grabbed
}
}
Ok(InodeHandle::Fd(parent_fd)) => self.grab_unlinked_fd(parent_fd, newname).ok(),
Err(_) => None,
}
} else {
None
};

let res = unsafe { libc::renamex_np(old_cpath.as_ptr(), new_cpath.as_ptr(), mflags) };
if res == 0 {
// If the rename overwrote a tracked inode, hand its preserved fd to
// the inode store so later ops resolve by fd, not the vanished path.
// `store_unlinked_fd` takes ownership only when that inode is
// tracked; close the fd ourselves otherwise so it is never leaked.
if let Some(fd) = doomed_fd {
match self.store_unlinked_fd(fd) {
Ok(true) => {}
Ok(false) | Err(_) => unsafe {
libc::close(fd);
},
}
}

if ((flags as i32) & bindings::LINUX_RENAME_WHITEOUT) != 0 {
let fd = unsafe {
libc::open(
Expand Down Expand Up @@ -1688,6 +1743,10 @@ impl FileSystem for PassthroughFs {

Ok(())
} else {
if let Some(fd) = doomed_fd {
// The rename failed; nothing was overwritten. Drop the fd.
unsafe { libc::close(fd) };
}
Err(linux_error(io::Error::last_os_error()))
}
}
Expand Down
Loading