net/http: fix early side effects in the ResponseWriter's ReadFrom

The ResponseWriter's ReadFrom method was causing side effects on
the output before any data was read.

Now, bail out early and do a normal copy (which does a read
before writing) when our input and output are known to not to
be the pair of types we need for sendfile.

Fixes #5660

R=golang-dev, rsc, nightlyone
CC=golang-dev
https://golang.org/cl/12632043

diff --git a/src/pkg/net/http/serve_test.go b/src/pkg/net/http/serve_test.go
index 5b394660a3cb2276bdaa065c864cb6e81acff559..70b7e0f103e36d3d274e49190d4847ba68789169 100644 (file)
--- a/src/pkg/net/http/serve_test.go
+++ b/src/pkg/net/http/serve_test.go
@@ -1815,6 +1815,51 @@ func TestHTTP10ConnectionHeader(t *testing.T) {
        }
 }
 
+// See golang.org/issue/5660
+func TestServerReaderFromOrder(t *testing.T) {
+       defer afterTest(t)
+       pr, pw := io.Pipe()
+       const size = 3 << 20
+       ts := httptest.NewServer(HandlerFunc(func(rw ResponseWriter, req *Request) {
+               rw.Header().Set("Content-Type", "text/plain") // prevent sniffing path
+               done := make(chan bool)
+               go func() {
+                       io.Copy(rw, pr)
+                       close(done)
+               }()
+               time.Sleep(25 * time.Millisecond) // give Copy a chance to break things
+               n, err := io.Copy(ioutil.Discard, req.Body)
+               if err != nil {
+                       t.Errorf("handler Copy: %v", err)
+                       return
+               }
+               if n != size {
+                       t.Errorf("handler Copy = %d; want %d", n, size)
+               }
+               pw.Write([]byte("hi"))
+               pw.Close()
+               <-done
+       }))
+       defer ts.Close()
+
+       req, err := NewRequest("POST", ts.URL, io.LimitReader(neverEnding('a'), size))
+       if err != nil {
+               t.Fatal(err)
+       }
+       res, err := DefaultClient.Do(req)
+       if err != nil {
+               t.Fatal(err)
+       }
+       all, err := ioutil.ReadAll(res.Body)
+       if err != nil {
+               t.Fatal(err)
+       }
+       res.Body.Close()
+       if string(all) != "hi" {
+               t.Errorf("Body = %q; want hi", all)
+       }
+}
+
 func BenchmarkClientServer(b *testing.B) {
        b.ReportAllocs()
        b.StopTimer()

diff --git a/src/pkg/net/http/server.go b/src/pkg/net/http/server.go
index 92947052922ad00bdb6ba88ebae8029bcc50d5cf..56b8f4a58a8dabe0b80a9c0097ea26cbaa891a4b 100644 (file)
--- a/src/pkg/net/http/server.go
+++ b/src/pkg/net/http/server.go
@@ -16,6 +16,7 @@ import (
        "log"
        "net"
        "net/url"
+       "os"
        "path"
        "runtime"
        "strconv"
@@ -345,11 +346,44 @@ func (w *response) needsSniff() bool {
        return !w.cw.wroteHeader && w.handlerHeader.Get("Content-Type") == "" && w.written < sniffLen
 }
 
+// writerOnly hides an io.Writer value's optional ReadFrom method
+// from io.Copy.
 type writerOnly struct {
        io.Writer
 }
 
+func srcIsRegularFile(src io.Reader) (isRegular bool, err error) {
+       switch v := src.(type) {
+       case *os.File:
+               fi, err := v.Stat()
+               if err != nil {
+                       return false, err
+               }
+               return fi.Mode().IsRegular(), nil
+       case *io.LimitedReader:
+               return srcIsRegularFile(v.R)
+       default:
+               return
+       }
+}
+
+// ReadFrom is here to optimize copying from an *os.File regular file
+// to a *net.TCPConn with sendfile.
 func (w *response) ReadFrom(src io.Reader) (n int64, err error) {
+       // Our underlying w.conn.rwc is usually a *TCPConn (with its
+       // own ReadFrom method). If not, or if our src isn't a regular
+       // file, just fall back to the normal copy method.
+       rf, ok := w.conn.rwc.(io.ReaderFrom)
+       regFile, err := srcIsRegularFile(src)
+       if err != nil {
+               return 0, err
+       }
+       if !ok || !regFile {
+               return io.Copy(writerOnly{w}, src)
+       }
+
+       // sendfile path:
+
        if !w.wroteHeader {
                w.WriteHeader(StatusOK)
        }
@@ -367,16 +401,12 @@ func (w *response) ReadFrom(src io.Reader) (n int64, err error) {
 
        // Now that cw has been flushed, its chunking field is guaranteed initialized.
        if !w.cw.chunking && w.bodyAllowed() {
-               if rf, ok := w.conn.rwc.(io.ReaderFrom); ok {
-                       n0, err := rf.ReadFrom(src)
-                       n += n0
-                       w.written += n0
-                       return n, err
-               }
+               n0, err := rf.ReadFrom(src)
+               n += n0
+               w.written += n0
+               return n, err
        }
 
-       // Fall back to default io.Copy implementation.
-       // Use wrapper to hide w.ReadFrom from io.Copy.
        n0, err := io.Copy(writerOnly{w}, src)
        n += n0
        return n, err