From 1adedad81c0ddc68526d616ad31daf2af0135f47 Mon Sep 17 00:00:00 2001 From: Tiwei Bie Date: Thu, 10 Sep 2020 21:47:17 +0800 Subject: Fix proc.(*fdDir).IterDirents for VFS2 Currently the returned offset is an index, and we can't use it to find the next fd to serialize, because getdents should iterate correctly despite mutation of fds. Instead, we can return the next fd to serialize plus 2 (which accounts for "." and "..") as the offset. Fixes: #3894 Signed-off-by: Tiwei Bie --- pkg/sentry/fsimpl/proc/task_fds.go | 13 +++++++++---- test/syscalls/linux/proc.cc | 24 ++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/pkg/sentry/fsimpl/proc/task_fds.go b/pkg/sentry/fsimpl/proc/task_fds.go index 3f0d78461..94ec2ff69 100644 --- a/pkg/sentry/fsimpl/proc/task_fds.go +++ b/pkg/sentry/fsimpl/proc/task_fds.go @@ -86,14 +86,19 @@ func (i *fdDir) IterDirents(ctx context.Context, cb vfs.IterDirentsCallback, off Name: strconv.FormatUint(uint64(fd), 10), Type: typ, Ino: i.fs.NextIno(), - NextOff: offset + 1, + NextOff: int64(fd) + 3, } if err := cb.Handle(dirent); err != nil { - return offset, err + // Getdents should iterate correctly despite mutation + // of fds, so we return the next fd to serialize plus + // 2 (which accounts for the "." and ".." tracked by + // kernfs) as the offset. + return int64(fd) + 2, err } - offset++ } - return offset, nil + // We serialized them all. Next offset should be higher than last + // serialized fd. + return int64(fds[len(fds)-1]) + 3, nil } // fdDirInode represents the inode for /proc/[pid]/fd directory. diff --git a/test/syscalls/linux/proc.cc b/test/syscalls/linux/proc.cc index b73189e55..f3f80a12a 100644 --- a/test/syscalls/linux/proc.cc +++ b/test/syscalls/linux/proc.cc @@ -694,6 +694,30 @@ TEST(ProcSelfFd, OpenFd) { ASSERT_THAT(close(pipe_fds[1]), SyscallSucceeds()); } +static void CheckFdDirGetdentsDuplicates(const std::string& path) { + const FileDescriptor fd = + ASSERT_NO_ERRNO_AND_VALUE(Open(path.c_str(), O_RDONLY | O_DIRECTORY)); + // Open a FD whose value is supposed to be much larger than + // the number of FDs opened by current process. + auto newfd = fcntl(fd.get(), F_DUPFD, 1024); + EXPECT_GE(newfd, 1024); + auto fd_closer = Cleanup([newfd]() { close(newfd); }); + auto fd_files = ASSERT_NO_ERRNO_AND_VALUE(ListDir(path.c_str(), false)); + std::unordered_set fd_files_dedup(fd_files.begin(), + fd_files.end()); + EXPECT_EQ(fd_files.size(), fd_files_dedup.size()); +} + +// This is a regression test for gvisor.dev/issues/3894 +TEST(ProcSelfFd, GetdentsDuplicates) { + CheckFdDirGetdentsDuplicates("/proc/self/fd"); +} + +// This is a regression test for gvisor.dev/issues/3894 +TEST(ProcSelfFdInfo, GetdentsDuplicates) { + CheckFdDirGetdentsDuplicates("/proc/self/fdinfo"); +} + TEST(ProcSelfFdInfo, CorrectFds) { // Make sure there is at least one open file. auto f = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile()); -- cgit v1.2.3