From 1776ab28f0fb934d399361e6012945c70dcd996f Mon Sep 17 00:00:00 2001 From: Nicolas Lacasse Date: Tue, 2 Apr 2019 17:27:30 -0700 Subject: [PATCH] Add test that symlinking over a directory returns EEXIST. Also remove comments in InodeOperations that required that implementation of some Create* operations ensure that the name does not already exist, since these checks are all centralized in the Dirent. PiperOrigin-RevId: 241637335 Change-Id: Id098dc6063ff7c38347af29d1369075ad1e89a58 --- pkg/sentry/fs/inode_operations.go | 6 +----- pkg/sentry/fs/ramfs/dir.go | 4 ---- test/syscalls/linux/symlink.cc | 24 ++++++++++-------------- 3 files changed, 11 insertions(+), 23 deletions(-) diff --git a/pkg/sentry/fs/inode_operations.go b/pkg/sentry/fs/inode_operations.go index 548f1eb8b..e8b9ab96b 100644 --- a/pkg/sentry/fs/inode_operations.go +++ b/pkg/sentry/fs/inode_operations.go @@ -104,15 +104,12 @@ type InodeOperations interface { CreateLink(ctx context.Context, dir *Inode, oldname string, newname string) error // CreateHardLink creates a hard link under dir between the target - // Inode and name. Implementations must ensure that name does not - // already exist. + // Inode and name. // // The caller must ensure this operation is permitted. CreateHardLink(ctx context.Context, dir *Inode, target *Inode, name string) error // CreateFifo creates a new named pipe under dir at name. - // Implementations must ensure that an Inode at name does not - // already exist. // // The caller must ensure that this operation is permitted. CreateFifo(ctx context.Context, dir *Inode, name string, perm FilePermissions) error @@ -144,7 +141,6 @@ type InodeOperations interface { Rename(ctx context.Context, oldParent *Inode, oldName string, newParent *Inode, newName string, replacement bool) error // Bind binds a new socket under dir at the given name. - // Implementations must ensure that name does not already exist. // // The caller must ensure that this operation is permitted. Bind(ctx context.Context, dir *Inode, name string, data transport.BoundEndpoint, perm FilePermissions) (*Dirent, error) diff --git a/pkg/sentry/fs/ramfs/dir.go b/pkg/sentry/fs/ramfs/dir.go index b60dab243..05d716afb 100644 --- a/pkg/sentry/fs/ramfs/dir.go +++ b/pkg/sentry/fs/ramfs/dir.go @@ -268,10 +268,6 @@ func (d *Dir) createInodeOperationsCommon(ctx context.Context, name string, make d.mu.Lock() defer d.mu.Unlock() - if _, ok := d.children[name]; ok { - return nil, syscall.EEXIST - } - inode, err := makeInodeOperations() if err != nil { return nil, err diff --git a/test/syscalls/linux/symlink.cc b/test/syscalls/linux/symlink.cc index cfc87bc8f..ea6baf76a 100644 --- a/test/syscalls/linux/symlink.cc +++ b/test/syscalls/linux/symlink.cc @@ -113,23 +113,19 @@ TEST(SymlinkTest, CannotCreateSymlinkInReadOnlyDir) { } TEST(SymlinkTest, CannotSymlinkOverExistingFile) { - const std::string oldname = NewTempAbsPath(); - const std::string newname = NewTempAbsPath(); + const auto oldfile = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile()); + const auto newfile = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile()); - int oldfd; - int newfd; - ASSERT_THAT(oldfd = open(oldname.c_str(), O_CREAT | O_RDWR, 0666), - SyscallSucceeds()); - EXPECT_THAT(close(oldfd), SyscallSucceeds()); - ASSERT_THAT(newfd = open(newname.c_str(), O_CREAT | O_RDWR, 0666), - SyscallSucceeds()); - EXPECT_THAT(close(newfd), SyscallSucceeds()); - - EXPECT_THAT(symlink(oldname.c_str(), newname.c_str()), + EXPECT_THAT(symlink(oldfile.path().c_str(), newfile.path().c_str()), SyscallFailsWithErrno(EEXIST)); +} - EXPECT_THAT(unlink(oldname.c_str()), SyscallSucceeds()); - EXPECT_THAT(unlink(newname.c_str()), SyscallSucceeds()); +TEST(SymlinkTest, CannotSymlinkOverExistingDir) { + const auto oldfile = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile()); + const auto newdir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir()); + + EXPECT_THAT(symlink(oldfile.path().c_str(), newdir.path().c_str()), + SyscallFailsWithErrno(EEXIST)); } TEST(SymlinkTest, OldnameIsEmpty) {