net/http/httputil: make ReverseProxy panic on error while copying body

Fixes #23643.

Change-Id: I4f8195a36be817d79b9e7c61a5301f153b681493
Reviewed-on: https://go-review.googlesource.com/91675
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index 8704ab7a909492aa23cddfb433f5f81af0d31e46..80ee22895af6ac93b52dccff65ba5c981476b2f0 100644 (file)
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -237,7 +237,14 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
                        fl.Flush()
                }
        }
-       p.copyResponse(rw, res.Body)
+       err = p.copyResponse(rw, res.Body)
+       if err != nil {
+               defer res.Body.Close()
+               // Since we're streaming the response, if we run into an error all we can do
+               // is abort the request. Issue 23643: ReverseProxy should use ErrAbortHandler
+               // on read error while copying body
+               panic(http.ErrAbortHandler)
+       }
        res.Body.Close() // close now, instead of defer, to populate res.Trailer
 
        if len(res.Trailer) == announcedTrailers {
@@ -265,7 +272,7 @@ func removeConnectionHeaders(h http.Header) {
        }
 }
 
-func (p *ReverseProxy) copyResponse(dst io.Writer, src io.Reader) {
+func (p *ReverseProxy) copyResponse(dst io.Writer, src io.Reader) error {
        if p.FlushInterval != 0 {
                if wf, ok := dst.(writeFlusher); ok {
                        mlw := &maxLatencyWriter{
@@ -282,13 +289,14 @@ func (p *ReverseProxy) copyResponse(dst io.Writer, src io.Reader) {
        var buf []byte
        if p.BufferPool != nil {
                buf = p.BufferPool.Get()
+               defer p.BufferPool.Put(buf)
        }
-       p.copyBuffer(dst, src, buf)
-       if p.BufferPool != nil {
-               p.BufferPool.Put(buf)
-       }
+       _, err := p.copyBuffer(dst, src, buf)
+       return err
 }
 
+// copyBuffer returns any write errors or non-EOF read errors, and the amount
+// of bytes written.
 func (p *ReverseProxy) copyBuffer(dst io.Writer, src io.Reader, buf []byte) (int64, error) {
        if len(buf) == 0 {
                buf = make([]byte, 32*1024)
@@ -312,6 +320,9 @@ func (p *ReverseProxy) copyBuffer(dst io.Writer, src io.Reader, buf []byte) (int
                        }
                }
                if rerr != nil {
+                       if rerr == io.EOF {
+                               rerr = nil
+                       }
                        return written, rerr
                }
        }

diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go
index 2232042d3ed14eec76116bc0b778e5b14e3fdb74..3dcc5c72874facc6d0ba96da34c24ed989ddfe9f 100644 (file)
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -649,18 +649,22 @@ func TestReverseProxy_CopyBuffer(t *testing.T) {
        var proxyLog bytes.Buffer
        rproxy := NewSingleHostReverseProxy(rpURL)
        rproxy.ErrorLog = log.New(&proxyLog, "", log.Lshortfile)
-       frontendProxy := httptest.NewServer(rproxy)
+       donec := make(chan bool, 1)
+       frontendProxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+               defer func() { donec <- true }()
+               rproxy.ServeHTTP(w, r)
+       }))
        defer frontendProxy.Close()
 
-       resp, err := http.Get(frontendProxy.URL)
-       if err != nil {
-               t.Fatalf("failed to reach proxy: %v", err)
-       }
-       defer resp.Body.Close()
-
-       if _, err := ioutil.ReadAll(resp.Body); err == nil {
+       if _, err = frontendProxy.Client().Get(frontendProxy.URL); err == nil {
                t.Fatalf("want non-nil error")
        }
+       // The race detector complains about the proxyLog usage in logf in copyBuffer
+       // and our usage below with proxyLog.Bytes() so we're explicitly using a
+       // channel to ensure that the ReverseProxy's ServeHTTP is done before we
+       // continue after Get.
+       <-donec
+
        expected := []string{
                "EOF",
                "read",
@@ -813,3 +817,35 @@ func (cc *checkCloser) Close() error {
 func (cc *checkCloser) Read(b []byte) (int, error) {
        return len(b), nil
 }
+
+// Issue 23643: panic on body copy error
+func TestReverseProxy_PanicBodyError(t *testing.T) {
+       backendServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+               out := "this call was relayed by the reverse proxy"
+               // Coerce a wrong content length to induce io.ErrUnexpectedEOF
+               w.Header().Set("Content-Length", fmt.Sprintf("%d", len(out)*2))
+               fmt.Fprintln(w, out)
+       }))
+       defer backendServer.Close()
+
+       rpURL, err := url.Parse(backendServer.URL)
+       if err != nil {
+               t.Fatal(err)
+       }
+
+       rproxy := NewSingleHostReverseProxy(rpURL)
+
+       // Ensure that the handler panics when the body read encounters an
+       // io.ErrUnexpectedEOF
+       defer func() {
+               err := recover()
+               if err == nil {
+                       t.Fatal("handler should have panicked")
+               }
+               if err != http.ErrAbortHandler {
+                       t.Fatal("expected ErrAbortHandler, got", err)
+               }
+       }()
+       req, _ := http.NewRequest("GET", "http://foo.tld/", nil)
+       rproxy.ServeHTTP(httptest.NewRecorder(), req)
+}