From 704543b8f338ad2a18a3ec5c89918f1016ad8b7c Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Mon, 20 Nov 2023 12:01:59 -0800 Subject: [PATCH] Ensure empty VFSPipeFD.SpliceToNonPipe(/dev/null) returns ErrWouldBlock. When grep's stdout is /dev/null (so printed matches are discarded), its outcome is only observable in its exit code, which is binary (0 for matches, 1 for no matches). When grep's stdin is additionally a pipe, GNU grep optimizes for this specific case by switching from reading input to splicing it directly to stdout after the first match: ``` if (exit_on_match | dev_null_output) list_files = LISTFILES_NONE; ... if (list_files == LISTFILES_NONE) finalize_input (desc, &st, ineof); ... static bool drain_input (int fd, struct stat const *st) { ssize_t nbytes; if (S_ISFIFO (st->st_mode) && dev_null_output) { #ifdef SPLICE_F_MOVE /* Should be faster, since it need not copy data to user space. */ nbytes = splice (fd, NULL, STDOUT_FILENO, NULL, INITIAL_BUFSIZE, SPLICE_F_MOVE); ``` This triggers a bug in our splice implementation: since memdev.nullFD.Write() never calls back into pipe.Pipe.peekLocked() to get ErrWouldBlock, this is never propagated up to syscalls/linux.Splice(). Consequently, splice() returns 0 instead of blocking; grep interprets this as EOF from the pipe and exits. We can't fix this by calling src.CopyInTo() in memdev.nullFD.Write() because this would have the wrong behavior for `write(/dev/null, unmapped addr)`, which should succeed because `drivers/char/mem.c:null_write()` also ignores the application-provided pointer. Instead, handle this in VFSPipeFD.SpliceToNonPipe(). (Linux instead avoids this problem by distinguishing file_operations::write and file_operations::splice_write, which we would prefer to avoid if possible.) Fixes #9736 PiperOrigin-RevId: 584091971 --- g3doc/user_guide/compatibility.md | 2 +- pkg/sentry/kernel/pipe/vfs.go | 7 +++++++ test/syscalls/linux/splice.cc | 13 +++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/g3doc/user_guide/compatibility.md b/g3doc/user_guide/compatibility.md index ef50c0147..a10f5c940 100644 --- a/g3doc/user_guide/compatibility.md +++ b/g3doc/user_guide/compatibility.md @@ -67,7 +67,7 @@ Most common utilities work. Note that: | gcore | Working. | | gdb | Working. | | gosu | Working. | -| grep | Working (unless stdin is a pipe and stdout is /dev/null). | +| grep | Working. | | ifconfig | Works partially, like ip. Full support [in progress](https://gvisor.dev/issue/578). | | ip | Some subcommands work (e.g. addr, route). Full support [in progress](https://gvisor.dev/issue/578). | | less | Working. | diff --git a/pkg/sentry/kernel/pipe/vfs.go b/pkg/sentry/kernel/pipe/vfs.go index 3d58249b7..32527e045 100644 --- a/pkg/sentry/kernel/pipe/vfs.go +++ b/pkg/sentry/kernel/pipe/vfs.go @@ -277,6 +277,13 @@ func (fd *VFSPipeFD) SpliceToNonPipe(ctx context.Context, out *vfs.FileDescripti fd.pipe.consumeLocked(n) } + // Implementations of out.[P]Write() that ignore written data (e.g. + // /dev/null) may skip calling src.CopyIn[To]() and therefore miss getting + // ErrWouldBlock from Pipe.peekLocked(). + if n == 0 && err == nil && fd.pipe.size == 0 && fd.pipe.HasWriters() { + err = linuxerr.ErrWouldBlock + } + fd.pipe.mu.Unlock() if n > 0 { diff --git a/test/syscalls/linux/splice.cc b/test/syscalls/linux/splice.cc index f49ea4a7a..4d8e6cc73 100644 --- a/test/syscalls/linux/splice.cc +++ b/test/syscalls/linux/splice.cc @@ -921,6 +921,19 @@ TEST(SpliceTest, FromPipeWithConcurrentIo) { } } +// Regression test for #9736. +TEST(SpliceTest, FromEmptyPipeWithWriterToDevNull) { + int fds[2]; + ASSERT_THAT(pipe(fds), SyscallSucceeds()); + const FileDescriptor rfd(fds[0]); + const FileDescriptor wfd(fds[1]); + const FileDescriptor out_fd = + ASSERT_NO_ERRNO_AND_VALUE(Open("/dev/null", O_WRONLY)); + ASSERT_THAT( + splice(rfd.get(), nullptr, out_fd.get(), nullptr, 1, SPLICE_F_NONBLOCK), + SyscallFailsWithErrno(EAGAIN)); +} + } // namespace } // namespace testing