From: Ian Lance Taylor Date: Tue, 14 Mar 2023 21:49:16 +0000 (-0700) Subject: os, internal/poll: don't use splice with tty X-Git-Tag: go1.21rc1~1259 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=be27fcfd2bfeda927213f334811df794d6a45872;p=gostls13.git os, internal/poll: don't use splice with tty Also don't try to wait for a non-pollable FD. Fixes #59041 Change-Id: Ife469d8738f2cc27c0beba223bdc8f8bc757b2a7 Reviewed-on: https://go-review.googlesource.com/c/go/+/476335 Run-TryBot: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Gopher Robot Auto-Submit: Ian Lance Taylor Reviewed-by: Bryan Mills Reviewed-by: Ian Lance Taylor --- diff --git a/src/internal/poll/splice_linux.go b/src/internal/poll/splice_linux.go index 96cbe4a312..ae7e42d0e6 100644 --- a/src/internal/poll/splice_linux.go +++ b/src/internal/poll/splice_linux.go @@ -12,9 +12,6 @@ import ( ) const ( - // spliceNonblock makes calls to splice(2) non-blocking. - spliceNonblock = 0x2 - // maxSpliceSize is the maximum amount of data Splice asks // the kernel to move in a single call to splice(2). // We use 1MB as Splice writes data through a pipe, and 1MB is the default maximum pipe buffer size, @@ -91,15 +88,17 @@ func spliceDrain(pipefd int, sock *FD, max int) (int, error) { return 0, err } for { - n, err := splice(pipefd, sock.Sysfd, max, spliceNonblock) + n, err := splice(pipefd, sock.Sysfd, max, 0) if err == syscall.EINTR { continue } if err != syscall.EAGAIN { return n, err } - if err := sock.pd.waitRead(sock.isFile); err != nil { - return n, err + if sock.pd.pollable() { + if err := sock.pd.waitRead(sock.isFile); err != nil { + return n, err + } } } } @@ -127,7 +126,7 @@ func splicePump(sock *FD, pipefd int, inPipe int) (int, error) { } written := 0 for inPipe > 0 { - n, err := splice(sock.Sysfd, pipefd, inPipe, spliceNonblock) + n, err := splice(sock.Sysfd, pipefd, inPipe, 0) // Here, the condition n == 0 && err == nil should never be // observed, since Splice controls the write side of the pipe. if n > 0 { @@ -138,8 +137,10 @@ func splicePump(sock *FD, pipefd int, inPipe int) (int, error) { if err != syscall.EAGAIN { return written, err } - if err := sock.pd.waitWrite(sock.isFile); err != nil { - return written, err + if sock.pd.pollable() { + if err := sock.pd.waitWrite(sock.isFile); err != nil { + return written, err + } } } return written, nil diff --git a/src/os/readfrom_linux.go b/src/os/readfrom_linux.go index 7e8024028e..c67407cf66 100644 --- a/src/os/readfrom_linux.go +++ b/src/os/readfrom_linux.go @@ -33,6 +33,19 @@ func (f *File) readFrom(r io.Reader) (written int64, handled bool, err error) { } func (f *File) spliceToFile(r io.Reader) (written int64, handled bool, err error) { + // At least as of kernel 5.19.11, splice to a tty fails. + // poll.Splice will do the wrong thing if it can splice from r + // but can't splice to f: it will read data from r, which is + // not what we want if r is a pipe or socket. + // So we have to check now whether f is a tty. + fi, err := f.Stat() + if err != nil { + return 0, false, err + } + if fi.Mode()&ModeCharDevice != 0 { + return 0, false, nil + } + var ( remain int64 lr *io.LimitedReader diff --git a/src/os/readfrom_linux_test.go b/src/os/readfrom_linux_test.go index c499071340..70dccab8d1 100644 --- a/src/os/readfrom_linux_test.go +++ b/src/os/readfrom_linux_test.go @@ -6,7 +6,9 @@ package os_test import ( "bytes" + "errors" "internal/poll" + "internal/testpty" "io" "math/rand" "net" @@ -16,6 +18,7 @@ import ( "runtime" "strconv" "strings" + "sync" "syscall" "testing" "time" @@ -279,6 +282,12 @@ func TestSpliceFile(t *testing.T) { }) } }) + t.Run("TCP-To-TTY", func(t *testing.T) { + testSpliceToTTY(t, "tcp", 32768) + }) + t.Run("Unix-To-TTY", func(t *testing.T) { + testSpliceToTTY(t, "unix", 32768) + }) t.Run("Limited", func(t *testing.T) { t.Run("OneLess-TCP", func(t *testing.T) { for _, size := range sizes { @@ -396,6 +405,81 @@ func testSpliceFile(t *testing.T, proto string, size, limit int64) { } } +// Issue #59041. +func testSpliceToTTY(t *testing.T, proto string, size int64) { + var wg sync.WaitGroup + + // Call wg.Wait as the final deferred function, + // because the goroutines may block until some of + // the deferred Close calls. + defer wg.Wait() + + pty, ttyName, err := testpty.Open() + if err != nil { + t.Skipf("skipping test because pty open failed: %v", err) + } + defer pty.Close() + + // Open the tty directly, rather than via OpenFile. + // This bypasses the non-blocking support and is required + // to recreate the problem in the issue (#59041). + ttyFD, err := syscall.Open(ttyName, syscall.O_RDWR, 0) + if err != nil { + t.Skipf("skipping test becaused failed to open tty: %v", err) + } + defer syscall.Close(ttyFD) + + tty := NewFile(uintptr(ttyFD), "tty") + defer tty.Close() + + client, server := createSocketPair(t, proto) + + data := bytes.Repeat([]byte{'a'}, int(size)) + + wg.Add(1) + go func() { + defer wg.Done() + // The problem (issue #59041) occurs when writing + // a series of blocks of data. It does not occur + // when all the data is written at once. + for i := 0; i < len(data); i += 1024 { + if _, err := client.Write(data[i : i+1024]); err != nil { + // If we get here because the client was + // closed, skip the error. + if !errors.Is(err, net.ErrClosed) { + t.Errorf("error writing to socket: %v", err) + } + return + } + } + client.Close() + }() + + wg.Add(1) + go func() { + defer wg.Done() + buf := make([]byte, 32) + for { + if _, err := pty.Read(buf); err != nil { + if err != io.EOF && !errors.Is(err, ErrClosed) { + // An error here doesn't matter for + // our test. + t.Logf("error reading from pty: %v", err) + } + return + } + } + }() + + // Close Client to wake up the writing goroutine if necessary. + defer client.Close() + + _, err = io.Copy(tty, server) + if err != nil { + t.Fatal(err) + } +} + func testCopyFileRange(t *testing.T, size int64, limit int64) { dst, src, data, hook := newCopyFileRangeTest(t, size)