From 980de72deba4100e7f73f1b633ed8b04dc8448ea Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Thu, 16 Nov 2023 13:34:50 -0800 Subject: [PATCH] Call FileDescription.OnClose() for newfd being replaced in dup2 and dup3. dup(2) man page specifies: If the file descriptor newfd was previously open, it is closed before being reused; the close is performed silently (i.e., any errors during the close are not reported by dup2()). Even though we were DecRef-ing and hence releasing the replaced FD, we were not calling OnClose(). Compare fs/file.c:do_dup2() -> filp_close(tofree), which in turn calls filp_flush(). In gVisor, FileDescription.OnClose() analogously does such flush operations. in turn PiperOrigin-RevId: 583147682 --- pkg/sentry/fdimport/fdimport.go | 7 ++++- pkg/sentry/kernel/fd_table.go | 40 ++++++++++++--------------- pkg/sentry/kernel/fd_table_test.go | 20 +++++++++----- pkg/sentry/kernel/task.go | 2 +- pkg/sentry/syscalls/linux/sys_file.go | 9 +++++- 5 files changed, 46 insertions(+), 32 deletions(-) diff --git a/pkg/sentry/fdimport/fdimport.go b/pkg/sentry/fdimport/fdimport.go index 926077dbf..718182352 100644 --- a/pkg/sentry/fdimport/fdimport.go +++ b/pkg/sentry/fdimport/fdimport.go @@ -93,9 +93,14 @@ func Import(ctx context.Context, fdTable *kernel.FDTable, console bool, uid auth hostFD.Release() // FD is transferred to host FD. } - if err := fdTable.NewFDAt(ctx, int32(appFD), appFile, fdFlags[hostFD]); err != nil { + df, err := fdTable.NewFDAt(ctx, int32(appFD), appFile, fdFlags[hostFD]) + if err != nil { return nil, err } + if df != nil { + df.DecRef(ctx) + return nil, fmt.Errorf("app FD %d displaced while importing FDs", appFD) + } } if ttyFile == nil { return nil, nil diff --git a/pkg/sentry/kernel/fd_table.go b/pkg/sentry/kernel/fd_table.go index ee79d8d18..2d29d94b3 100644 --- a/pkg/sentry/kernel/fd_table.go +++ b/pkg/sentry/kernel/fd_table.go @@ -123,18 +123,14 @@ func (f *FDTable) loadDescriptorTable(m map[int32]descriptor) { } } -// drop drops the table reference. -func (f *FDTable) drop(ctx context.Context, file *vfs.FileDescription) { - // Release any POSIX lock possibly held by the FDTable. +// Release any POSIX lock possibly held by the FDTable. +func (f *FDTable) fileUnlock(ctx context.Context, file *vfs.FileDescription) { if file.SupportsLocks() { err := file.UnlockPOSIX(ctx, f, lock.LockRange{0, lock.LockEOF}) if err != nil && !linuxerr.Equals(linuxerr.ENOLCK, err) { panic(fmt.Sprintf("UnlockPOSIX failed: %v", err)) } } - - // Drop the table's reference. - file.DecRef(ctx) } // NewFDTable allocates a new FDTable that may be used by tasks in k. @@ -312,23 +308,24 @@ func (f *FDTable) NewFD(ctx context.Context, minFD int32, file *vfs.FileDescript } // NewFDAt sets the file reference for the given FD. If there is an existing -// file description for that FD, the table reference for that file description -// is dropped. +// file description for that FD, it is returned. +// +// N.B. Callers are required to use DecRef on the returned file when they are done. // // Precondition: file != nil. -func (f *FDTable) NewFDAt(ctx context.Context, fd int32, file *vfs.FileDescription, flags FDFlags) error { +func (f *FDTable) NewFDAt(ctx context.Context, fd int32, file *vfs.FileDescription, flags FDFlags) (*vfs.FileDescription, error) { if fd < 0 { // Don't accept negative FDs. - return unix.EBADF + return nil, unix.EBADF } if fd >= f.k.MaxFDLimit.Load() { - return unix.EMFILE + return nil, unix.EMFILE } // Check the limit for the provided file. if limitSet := limits.FromContext(ctx); limitSet != nil { if lim := limitSet.Get(limits.NumberOfFiles); lim.Cur != limits.Infinity && uint64(fd) >= lim.Cur { - return unix.EMFILE + return nil, unix.EMFILE } } @@ -342,9 +339,10 @@ func (f *FDTable) NewFDAt(ctx context.Context, fd int32, file *vfs.FileDescripti f.mu.Unlock() if df != nil { - f.drop(ctx, df) + f.fileUnlock(ctx, df) + // Table's reference on df is transferred to caller, so don't DecRef. } - return nil + return df, nil } // SetFlags sets the flags for the given file descriptor. @@ -470,14 +468,13 @@ func (f *FDTable) Remove(ctx context.Context, fd int32) *vfs.FileDescription { f.mu.Lock() df := f.set(fd, nil, FDFlags{}) // Zap entry. if df != nil { - // Add reference for caller. - df.IncRef() f.fdBitmap.Remove(uint32(fd)) } f.mu.Unlock() if df != nil { - f.drop(ctx, df) + f.fileUnlock(ctx, df) + // Table's reference on df is transferred to caller, so don't DecRef. } return df } @@ -499,7 +496,8 @@ func (f *FDTable) RemoveIf(ctx context.Context, cond func(*vfs.FileDescription, f.mu.Unlock() for _, file := range files { - f.drop(ctx, file) + f.fileUnlock(ctx, file) + file.DecRef(ctx) // Drop the table's reference. } } @@ -513,7 +511,6 @@ func (f *FDTable) RemoveNextInRange(ctx context.Context, startFd int32, endFd in } f.mu.Lock() - fdUint, err := f.fdBitmap.FirstOne(uint32(startFd)) fd := int32(fdUint) if err != nil || fd > endFd { @@ -522,14 +519,13 @@ func (f *FDTable) RemoveNextInRange(ctx context.Context, startFd int32, endFd in } df := f.set(fd, nil, FDFlags{}) // Zap entry. if df != nil { - // Add reference for caller. - df.IncRef() f.fdBitmap.Remove(uint32(fd)) } f.mu.Unlock() if df != nil { - f.drop(ctx, df) + f.fileUnlock(ctx, df) + // Table's reference on df is transferred to caller, so don't DecRef. } return fd, df } diff --git a/pkg/sentry/kernel/fd_table_test.go b/pkg/sentry/kernel/fd_table_test.go index 1ed7e8d31..575e06e41 100644 --- a/pkg/sentry/kernel/fd_table_test.go +++ b/pkg/sentry/kernel/fd_table_test.go @@ -92,8 +92,10 @@ func TestFDTableMany(t *testing.T) { t.Fatalf("fdTable.NewFDs(0, r) in full map: got nil, wanted error") } - if err := fdTable.NewFDAt(ctx, 1, fd, FDFlags{}); err != nil { + if df, err := fdTable.NewFDAt(ctx, 1, fd, FDFlags{}); err != nil { t.Fatalf("fdTable.NewFDAt(1, r, FDFlags{}): got %v, wanted nil", err) + } else if df != nil { + t.Fatalf("fdTable.NewFDAt(1, r, FDFlags{}) displaced FD") } i := int32(2) @@ -159,11 +161,13 @@ func TestFDTable(t *testing.T) { t.Fatalf("Added an FD to a resized map: got %v, want {1}", fds) } - if err := fdTable.NewFDAt(ctx, 1, fd, FDFlags{}); err != nil { + if df, err := fdTable.NewFDAt(ctx, 1, fd, FDFlags{}); err != nil { t.Fatalf("Replacing FD 1 via fdTable.NewFDAt(1, r, FDFlags{}): got %v, wanted nil", err) + } else if df != nil { + t.Fatalf("fdTable.NewFDAt(1, r, FDFlags{}) displaced FD") } - if err := fdTable.NewFDAt(ctx, maxFD+1, fd, FDFlags{}); err == nil { + if _, err := fdTable.NewFDAt(ctx, maxFD+1, fd, FDFlags{}); err == nil { t.Fatalf("Using an FD that was too large via fdTable.NewFDAt(%v, r, FDFlags{}): got nil, wanted an error", maxFD+1) } @@ -189,8 +193,10 @@ func TestFDTable(t *testing.T) { func TestDescriptorFlags(t *testing.T) { runTest(t, func(ctx context.Context, fdTable *FDTable, fd *vfs.FileDescription, _ *limits.LimitSet) { - if err := fdTable.NewFDAt(ctx, 2, fd, FDFlags{CloseOnExec: true}); err != nil { + if df, err := fdTable.NewFDAt(ctx, 2, fd, FDFlags{CloseOnExec: true}); err != nil { t.Fatalf("fdTable.NewFDAt(2, r, FDFlags{}): got %v, wanted nil", err) + } else if df != nil { + t.Fatalf("fdTable.NewFDAt(2, r, FDFlags{}) displaced FD") } newFile, flags := fdTable.Get(2) @@ -231,7 +237,7 @@ func BenchmarkNewFDAt(b *testing.B) { b.StartTimer() // Benchmark. for i := 0; i < b.N; i++ { - err := fdTable.NewFDAt(ctx, int32(i%maxLimit), fd, FDFlags{}) + _, err := fdTable.NewFDAt(ctx, int32(i%maxLimit), fd, FDFlags{}) if err != nil { b.Fatalf("fdTable.NewFDAt: got %v, wanted nil", err) } @@ -244,7 +250,7 @@ func BenchmarkFork(b *testing.B) { runTest(b, func(ctx context.Context, fdTable *FDTable, fd *vfs.FileDescription, limitSet *limits.LimitSet) { for i := 0; i < maxFD; i++ { - err := fdTable.NewFDAt(ctx, int32(i), fd, FDFlags{}) + _, err := fdTable.NewFDAt(ctx, int32(i), fd, FDFlags{}) if err != nil { b.Fatalf("fdTable.NewFDs: got %v, wanted nil", err) } @@ -267,7 +273,7 @@ func BenchmarkCreateWithMaxFD(b *testing.B) { for i := 0; i < b.N; i++ { fdTable := new(FDTable) fdTable.init() - err := fdTable.NewFDAt(ctx, maxLimit-1, fd, FDFlags{}) + _, err := fdTable.NewFDAt(ctx, maxLimit-1, fd, FDFlags{}) if err != nil { b.Fatalf("fdTable.NewFDs: got %v, wanted nil", err) } diff --git a/pkg/sentry/kernel/task.go b/pkg/sentry/kernel/task.go index c0c9c0298..261601304 100644 --- a/pkg/sentry/kernel/task.go +++ b/pkg/sentry/kernel/task.go @@ -774,7 +774,7 @@ func (t *Task) NewFDFrom(minFD int32, file *vfs.FileDescription, flags FDFlags) // This automatically passes the task as the context. // // Precondition: same as FDTable. -func (t *Task) NewFDAt(fd int32, file *vfs.FileDescription, flags FDFlags) error { +func (t *Task) NewFDAt(fd int32, file *vfs.FileDescription, flags FDFlags) (*vfs.FileDescription, error) { return t.fdTable.NewFDAt(t, fd, file, flags) } diff --git a/pkg/sentry/syscalls/linux/sys_file.go b/pkg/sentry/syscalls/linux/sys_file.go index ded2c8f50..3d8665726 100644 --- a/pkg/sentry/syscalls/linux/sys_file.go +++ b/pkg/sentry/syscalls/linux/sys_file.go @@ -562,7 +562,7 @@ func dup3(t *kernel.Task, oldfd, newfd int32, flags uint32) (uintptr, *kernel.Sy } defer file.DecRef(t) - err := t.NewFDAt(newfd, file, kernel.FDFlags{ + df, err := t.NewFDAt(newfd, file, kernel.FDFlags{ CloseOnExec: flags&linux.O_CLOEXEC != 0, }) if linuxerr.Equals(linuxerr.EMFILE, err) { @@ -571,6 +571,13 @@ func dup3(t *kernel.Task, oldfd, newfd int32, flags uint32) (uintptr, *kernel.Sy if err != nil { return 0, nil, err } + if df != nil { + // "If the file descriptor newfd was previously open, it is closed + // before being reused; the close is performed silently (i.e., any + // errors during the close are not reported by dup2())." - dup(2) + _ = df.OnClose(t) + df.DecRef(t) + } return uintptr(newfd), nil, nil }