From da3eb80271d6f23ac4830fefd200c809936e4639 Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Mon, 26 Feb 2024 13:53:35 -0800 Subject: [PATCH] Fix #10046 See updated comment in sentry/kernel/pipe/vfs.go. PiperOrigin-RevId: 610516821 --- pkg/sentry/kernel/pipe/pipe.go | 21 +++++++++++++-------- pkg/sentry/kernel/pipe/pipe_util.go | 2 +- pkg/sentry/kernel/pipe/vfs.go | 17 +++++++++-------- test/syscalls/linux/splice.cc | 16 ++++++++++++++-- 4 files changed, 37 insertions(+), 19 deletions(-) diff --git a/pkg/sentry/kernel/pipe/pipe.go b/pkg/sentry/kernel/pipe/pipe.go index 422140352..48b44e74a 100644 --- a/pkg/sentry/kernel/pipe/pipe.go +++ b/pkg/sentry/kernel/pipe/pipe.go @@ -179,9 +179,9 @@ func initPipe(pipe *Pipe, isNamed bool, sizeBytes int64) { pipe.max = sizeBytes } -// peekLocked passes the first count bytes in the pipe to f and returns its -// result. If fewer than count bytes are available, the safemem.BlockSeq passed -// to f will be less than count bytes in length. +// peekLocked passes the first count bytes in the pipe, starting at offset off, +// to f and returns its result. If fewer than count bytes are available, the +// safemem.BlockSeq passed to f will be less than count bytes in length. // // peekLocked does not mutate the pipe; if the read consumes bytes from the // pipe, then the caller is responsible for calling p.consumeLocked() and @@ -191,25 +191,30 @@ func initPipe(pipe *Pipe, isNamed bool, sizeBytes int64) { // Preconditions: // - p.mu must be locked. // - This pipe must have readers. -func (p *Pipe) peekLocked(count int64, f func(safemem.BlockSeq) (uint64, error)) (int64, error) { +// - off <= p.size. +func (p *Pipe) peekLocked(off, count int64, f func(safemem.BlockSeq) (uint64, error)) (int64, error) { // Don't block for a zero-length read even if the pipe is empty. if count == 0 { return 0, nil } // Limit the amount of data read to the amount of data in the pipe. - if count > p.size { - if p.size == 0 { + if rem := p.size - off; count > rem { + if rem == 0 { if !p.HasWriters() { return 0, io.EOF } return 0, linuxerr.ErrWouldBlock } - count = p.size + count = rem } // Prepare the view of the data to be read. - bs := p.bufBlockSeq.DropFirst64(uint64(p.off)).TakeFirst64(uint64(count)) + pipeOff := p.off + off + if max := int64(len(p.buf)); pipeOff >= max { + pipeOff -= max + } + bs := p.bufBlockSeq.DropFirst64(uint64(pipeOff)).TakeFirst64(uint64(count)) // Perform the read. done, err := f(bs) diff --git a/pkg/sentry/kernel/pipe/pipe_util.go b/pkg/sentry/kernel/pipe/pipe_util.go index ebafd3959..807e9c469 100644 --- a/pkg/sentry/kernel/pipe/pipe_util.go +++ b/pkg/sentry/kernel/pipe/pipe_util.go @@ -66,7 +66,7 @@ func (p *Pipe) Read(ctx context.Context, dst usermem.IOSequence) (int64, error) func (p *Pipe) read(count int64, f func(srcs safemem.BlockSeq) (uint64, error), removeFromSrc bool) (int64, error) { p.mu.Lock() defer p.mu.Unlock() - n, err := p.peekLocked(count, f) + n, err := p.peekLocked(0, count, f) if n > 0 && removeFromSrc { p.consumeLocked(n) } diff --git a/pkg/sentry/kernel/pipe/vfs.go b/pkg/sentry/kernel/pipe/vfs.go index fca043ac8..b4efd6ee1 100644 --- a/pkg/sentry/kernel/pipe/vfs.go +++ b/pkg/sentry/kernel/pipe/vfs.go @@ -279,10 +279,13 @@ func (fd *VFSPipeFD) SpliceToNonPipe(ctx context.Context, out *vfs.FileDescripti } else { n, err = out.PWrite(ctx, src, off, vfs.WriteOptions{}) } - // 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(). + // /dev/null) may skip calling src.CopyIn[To](), so: + // + // - We must call Pipe.consumeLocked() here rather than in fd.CopyIn[To](). + // + // - We must check if Pipe.peekLocked() would have returned ErrWouldBlock. + fd.pipe.consumeLocked(n) if n == 0 && err == nil && fd.pipe.size == 0 && fd.pipe.HasWriters() { err = linuxerr.ErrWouldBlock } @@ -331,10 +334,9 @@ func (fd *VFSPipeFD) CopyIn(ctx context.Context, addr hostarch.Addr, dst []byte, log.Traceback("Non-sequential VFSPipeFD.CopyIn: lastAddr=%#x addr=%#x", fd.lastAddr, addr) return 0, linuxerr.EINVAL } - n, err := fd.pipe.peekLocked(int64(len(dst)), func(srcs safemem.BlockSeq) (uint64, error) { + n, err := fd.pipe.peekLocked(int64(addr), int64(len(dst)), func(srcs safemem.BlockSeq) (uint64, error) { return safemem.CopySeq(safemem.BlockSeqOf(safemem.BlockFromSafeSlice(dst)), srcs) }) - fd.pipe.consumeLocked(n) fd.lastAddr = addr + hostarch.Addr(n) return int(n), err } @@ -384,10 +386,9 @@ func (fd *VFSPipeFD) CopyInTo(ctx context.Context, ars hostarch.AddrRangeSeq, ds log.Traceback("Non-sequential VFSPipeFD.CopyInTo: lastAddr=%#x addr=%#x", fd.lastAddr, ar.Start) return total, linuxerr.EINVAL } - n, err := fd.pipe.peekLocked(int64(ar.Length()), func(srcs safemem.BlockSeq) (uint64, error) { + n, err := fd.pipe.peekLocked(int64(ar.Start), int64(ar.Length()), func(srcs safemem.BlockSeq) (uint64, error) { return dst.WriteFromBlocks(srcs) }) - fd.pipe.consumeLocked(n) fd.lastAddr = ar.Start + hostarch.Addr(n) total += n if err != nil { @@ -464,7 +465,7 @@ func spliceOrTee(ctx context.Context, dst, src *VFSPipeFD, count int64, removeFr firstLocked, secondLocked := lockTwoPipes(dst.pipe, src.pipe) n, err := dst.pipe.writeLocked(count, func(dsts safemem.BlockSeq) (uint64, error) { - n, err := src.pipe.peekLocked(int64(dsts.NumBytes()), func(srcs safemem.BlockSeq) (uint64, error) { + n, err := src.pipe.peekLocked(0, int64(dsts.NumBytes()), func(srcs safemem.BlockSeq) (uint64, error) { return safemem.CopySeq(dsts, srcs) }) if n > 0 && removeFromSrc { diff --git a/test/syscalls/linux/splice.cc b/test/syscalls/linux/splice.cc index 4d8e6cc73..338259361 100644 --- a/test/syscalls/linux/splice.cc +++ b/test/syscalls/linux/splice.cc @@ -921,14 +921,26 @@ TEST(SpliceTest, FromPipeWithConcurrentIo) { } } -// Regression test for #9736. -TEST(SpliceTest, FromEmptyPipeWithWriterToDevNull) { +// Regression test for #9736 and #10046. +TEST(SpliceTest, FromPipeWithWriterToDevNull) { 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)); + + // Write one byte to the pipe and expect it to be successfully spliced to + // /dev/null. + constexpr char kSplicedByte = '.'; + ASSERT_THAT(WriteFd(wfd.get(), &kSplicedByte, 1), + SyscallSucceedsWithValue(1)); + ASSERT_THAT( + splice(rfd.get(), nullptr, out_fd.get(), nullptr, 1, SPLICE_F_NONBLOCK), + SyscallSucceedsWithValue(1)); + + // Expect that splicing from the pipe again fails with EAGAIN since the pipe + // is now empty. ASSERT_THAT( splice(rfd.get(), nullptr, out_fd.get(), nullptr, 1, SPLICE_F_NONBLOCK), SyscallFailsWithErrno(EAGAIN));