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 <[email protected]>
Run-TryBot: Brad Fitzpatrick <[email protected]>

Author: jameshartig

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
 		}
 	}

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)
+}