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 <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>

diff --git a/src/internal/poll/splice_linux.go b/src/internal/poll/splice_linux.go
index 96cbe4a3122704be3db057a481c95838c783d74c..ae7e42d0e6542adf17b13f29445b394c57d19690 100644 (file)
--- 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 7e8024028e98e852d7787845e8a94732f88e8cd4..c67407cf66ed6b83c18f0ee1d202701f3774ad34 100644 (file)
--- 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 c4990713409c577b576543d357cb1f9b0c4c2be1..70dccab8d19eaffac917616c6fb83eb0dab373a1 100644 (file)
--- 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)