internal/poll: use more fine-grained locking in Splice

The previous code acquired a read lock on src and a write lock on
dst for the entire duration of Splice. This resulted in deadlock,
in a situation akin to the following:

Splice is blocking, waiting to read from src.

The caller tries to close dst from another goroutine, but Close on
dst blocks in runtime.semacquire, because Splice is still holding a
write lock on it, and the Close didn't unblock any I/O.

The caller cannot unblock the read side of Splice through other means,
because they are stuck waiting in dst.Close().

Use more fine-grained locking instead: acquire the read lock on src
just before trying to splice from the source socket to the pipe,
and acquire the write lock on dst just before trying to splice from
the pipe to the destination socket.

Fixes #25985

Change-Id: I264c91c7a69bb6c5e28610e2bd801244804cf86d
Reviewed-on: https://go-review.googlesource.com/120317
Run-TryBot: Aram Hăvărneanu <aram@mgk.ro>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

diff --git a/src/internal/poll/splice_linux.go b/src/internal/poll/splice_linux.go
index 5874f79a564fdf130f6fc13abd6b0f1a95f3da9a..aa237e587adabf45945f8841c78966a755053f9c 100644 (file)
--- a/src/internal/poll/splice_linux.go
+++ b/src/internal/poll/splice_linux.go
@@ -34,20 +34,6 @@ func Splice(dst, src *FD, remain int64) (written int64, handled bool, sc string,
        defer destroyTempPipe(prfd, pwfd)
        // From here on, the operation should be considered handled,
        // even if Splice doesn't transfer any data.
-       if err := src.readLock(); err != nil {
-               return 0, true, "splice", err
-       }
-       defer src.readUnlock()
-       if err := dst.writeLock(); err != nil {
-               return 0, true, "splice", err
-       }
-       defer dst.writeUnlock()
-       if err := src.pd.prepareRead(src.isFile); err != nil {
-               return 0, true, "splice", err
-       }
-       if err := dst.pd.prepareWrite(dst.isFile); err != nil {
-               return 0, true, "splice", err
-       }
        var inPipe, n int
        for err == nil && remain > 0 {
                max := maxSpliceSize
@@ -84,6 +70,13 @@ func Splice(dst, src *FD, remain int64) (written int64, handled bool, sc string,
 //
 // If spliceDrain returns (0, nil), src is at EOF.
 func spliceDrain(pipefd int, sock *FD, max int) (int, error) {
+       if err := sock.readLock(); err != nil {
+               return 0, err
+       }
+       defer sock.readUnlock()
+       if err := sock.pd.prepareRead(sock.isFile); err != nil {
+               return 0, err
+       }
        for {
                n, err := splice(pipefd, sock.Sysfd, max, spliceNonblock)
                if err != syscall.EAGAIN {
@@ -109,6 +102,13 @@ func spliceDrain(pipefd int, sock *FD, max int) (int, error) {
 // all of it to the socket. This behavior is similar to the Write
 // step of an io.Copy in userspace.
 func splicePump(sock *FD, pipefd int, inPipe int) (int, error) {
+       if err := sock.writeLock(); err != nil {
+               return 0, err
+       }
+       defer sock.writeUnlock()
+       if err := sock.pd.prepareWrite(sock.isFile); err != nil {
+               return 0, err
+       }
        written := 0
        for inPipe > 0 {
                n, err := splice(sock.Sysfd, pipefd, inPipe, spliceNonblock)

diff --git a/src/net/splice_test.go b/src/net/splice_test.go
index 483a9e555f0d2233ce4173c37c22d5cffa7f943a..2f1e69ddb64106566cbf05010503fc7f22070c61 100644 (file)
--- a/src/net/splice_test.go
+++ b/src/net/splice_test.go
@@ -10,6 +10,8 @@ import (
        "bytes"
        "fmt"
        "io"
+       "io/ioutil"
+       "sync"
        "testing"
 )
 
@@ -19,6 +21,7 @@ func TestSplice(t *testing.T) {
        t.Run("big", testSpliceBig)
        t.Run("honorsLimitedReader", testSpliceHonorsLimitedReader)
        t.Run("readerAtEOF", testSpliceReaderAtEOF)
+       t.Run("issue25985", testSpliceIssue25985)
 }
 
 func testSpliceSimple(t *testing.T) {
@@ -234,6 +237,66 @@ func testSpliceReaderAtEOF(t *testing.T) {
        }
 }
 
+func testSpliceIssue25985(t *testing.T) {
+       front, err := newLocalListener("tcp")
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer front.Close()
+       back, err := newLocalListener("tcp")
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer back.Close()
+
+       var wg sync.WaitGroup
+       wg.Add(2)
+
+       proxy := func() {
+               src, err := front.Accept()
+               if err != nil {
+                       return
+               }
+               dst, err := Dial("tcp", back.Addr().String())
+               if err != nil {
+                       return
+               }
+               defer dst.Close()
+               defer src.Close()
+               go func() {
+                       io.Copy(src, dst)
+                       wg.Done()
+               }()
+               go func() {
+                       io.Copy(dst, src)
+                       wg.Done()
+               }()
+       }
+
+       go proxy()
+
+       toFront, err := Dial("tcp", front.Addr().String())
+       if err != nil {
+               t.Fatal(err)
+       }
+
+       io.WriteString(toFront, "foo")
+       toFront.Close()
+
+       fromProxy, err := back.Accept()
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer fromProxy.Close()
+
+       _, err = ioutil.ReadAll(fromProxy)
+       if err != nil {
+               t.Fatal(err)
+       }
+
+       wg.Wait()
+}
+
 func BenchmarkTCPReadFrom(b *testing.B) {
        testHookUninstaller.Do(uninstallTestHooks)