From 8a499ae65f361fb01c2e4be03122f69910a8ba4a Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Mon, 18 Mar 2019 18:39:08 -0700 Subject: Remove references to replaced child in Rename in ramfs/agentfs In the case of a rename replacing an existing destination inode, ramfs Rename failed to first remove the replaced inode. This caused: 1. A leak of a reference to the inode (making it live indefinitely). 2. For directories, a leak of the replaced directory's .. link to the parent. This would cause the parent's link count to incorrectly increase. (2) is much simpler to test than (1), so that's what I've done. agentfs has a similar bug with link count only, so the Dirent layer informs the Inode if this is a replacing rename. Fixes #133 PiperOrigin-RevId: 239105698 Change-Id: I4450af2462d8ae3339def812287213d2cbeebde0 --- pkg/sentry/fs/ramfs/dir.go | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) (limited to 'pkg/sentry/fs/ramfs') diff --git a/pkg/sentry/fs/ramfs/dir.go b/pkg/sentry/fs/ramfs/dir.go index 4da876ebd..b60dab243 100644 --- a/pkg/sentry/fs/ramfs/dir.go +++ b/pkg/sentry/fs/ramfs/dir.go @@ -15,6 +15,7 @@ package ramfs import ( + "fmt" "sync" "syscall" @@ -383,8 +384,8 @@ func (d *Dir) GetFile(ctx context.Context, dirent *fs.Dirent, flags fs.FileFlags } // Rename implements fs.InodeOperations.Rename. -func (*Dir) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string) error { - return Rename(ctx, oldParent.InodeOperations, oldName, newParent.InodeOperations, newName) +func (*Dir) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { + return Rename(ctx, oldParent.InodeOperations, oldName, newParent.InodeOperations, newName, replacement) } // dirFileOperations implements fs.FileOperations for a ramfs directory. @@ -456,7 +457,7 @@ func hasChildren(ctx context.Context, inode *fs.Inode) (bool, error) { } // Rename renames from a *ramfs.Dir to another *ramfs.Dir. -func Rename(ctx context.Context, oldParent fs.InodeOperations, oldName string, newParent fs.InodeOperations, newName string) error { +func Rename(ctx context.Context, oldParent fs.InodeOperations, oldName string, newParent fs.InodeOperations, newName string, replacement bool) error { op, ok := oldParent.(*Dir) if !ok { return syserror.EXDEV @@ -469,8 +470,14 @@ func Rename(ctx context.Context, oldParent fs.InodeOperations, oldName string, n np.mu.Lock() defer np.mu.Unlock() - // Check whether the ramfs entry to be replaced is a non-empty directory. - if replaced, ok := np.children[newName]; ok { + // Is this is an overwriting rename? + if replacement { + replaced, ok := np.children[newName] + if !ok { + panic(fmt.Sprintf("Dirent claims rename is replacement, but %q is missing from %+v", newName, np)) + } + + // Non-empty directories cannot be replaced. if fs.IsDir(replaced.StableAttr) { if ok, err := hasChildren(ctx, replaced); err != nil { return err @@ -478,6 +485,13 @@ func Rename(ctx context.Context, oldParent fs.InodeOperations, oldName string, n return syserror.ENOTEMPTY } } + + // Remove the replaced child and drop our reference on it. + inode, err := np.removeChildLocked(ctx, newName) + if err != nil { + return err + } + inode.DecRef() } // Be careful, we may have already grabbed this mutex above. -- cgit v1.2.3