diff --git a/src/net/http/httputil/dump.go b/src/net/http/httputil/dump.go index 23918e2ad4bbd5..0ebc868212320d 100644 --- a/src/net/http/httputil/dump.go +++ b/src/net/http/httputil/dump.go @@ -14,11 +14,62 @@ import ( "net/http" "net/url" "strings" + "sync/atomic" "time" ) +// dumpBody is an io.ReadCloser that reports [http.ErrBodyReadAfterClose] +// once it has been closed, as a real request or response body does. +// +// Close may be called concurrently with Read, as [http.Request.Body] +// requires. The readers dumpBody wraps are all in memory, so Read never +// blocks and there is nothing for Close to interrupt. +type dumpBody struct { + r io.Reader + closed atomic.Bool +} + +func (b *dumpBody) Read(p []byte) (int, error) { + if b.closed.Load() { + return 0, http.ErrBodyReadAfterClose + } + return b.r.Read(p) +} + +func (b *dumpBody) Close() error { + b.closed.Store(true) + return nil +} + +// dumpBodyWriterTo is a dumpBody whose underlying reader implements +// [io.WriterTo]. It forwards WriteTo so that copying from the body avoids +// a buffer copy, just as [io.NopCloser] does for such readers. +type dumpBodyWriterTo struct{ *dumpBody } + +func (b dumpBodyWriterTo) WriteTo(w io.Writer) (int64, error) { + if b.closed.Load() { + return 0, http.ErrBodyReadAfterClose + } + return b.r.(io.WriterTo).WriteTo(w) +} + +// newDumpBody returns an [io.ReadCloser] that yields the bytes of r and +// reports [http.ErrBodyReadAfterClose] once it has been closed. +func newDumpBody(r io.Reader) io.ReadCloser { + b := &dumpBody{r: r} + if _, ok := r.(io.WriterTo); ok { + return dumpBodyWriterTo{b} + } + return b +} + // drainBody reads all of b to memory and then returns two equivalent -// ReadClosers yielding the same bytes. +// ReadClosers yielding the same bytes. Reading from r1 after it has been +// closed reports [http.ErrBodyReadAfterClose], as reading from a real +// request or response body does; r1 is the one the Dump functions hand +// back to their caller. r2 is only used to produce the dump, so it stays +// an [io.NopCloser], which lets net/http recognize it as an in-memory +// reader and copy from it without an intermediate buffer. // // It returns an error if the initial slurp of all bytes fails. It does not attempt // to make the returned ReadClosers have identical error-matching behavior. @@ -34,7 +85,7 @@ func drainBody(b io.ReadCloser) (r1, r2 io.ReadCloser, err error) { if err = b.Close(); err != nil { return nil, b, err } - return io.NopCloser(&buf), io.NopCloser(bytes.NewReader(buf.Bytes())), nil + return newDumpBody(&buf), io.NopCloser(bytes.NewReader(buf.Bytes())), nil } // dumpConn is a net.Conn which writes to Writer and reads from Reader diff --git a/src/net/http/httputil/dump_test.go b/src/net/http/httputil/dump_test.go index c20c054865fa32..f76f8d2446a468 100644 --- a/src/net/http/httputil/dump_test.go +++ b/src/net/http/httputil/dump_test.go @@ -16,6 +16,7 @@ import ( "runtime" "runtime/pprof" "strings" + "sync" "testing" "time" ) @@ -491,6 +492,136 @@ func TestDumpResponse(t *testing.T) { } } +// Issue 77463: the body the Dump functions leave behind should report +// http.ErrBodyReadAfterClose once closed, as a real body does. +func TestDumpBodyReadAfterClose(t *testing.T) { + const body = "hello body" + + newReq := func() *http.Request { + req, err := http.NewRequest("POST", "http://example.com/", strings.NewReader(body)) + if err != nil { + t.Fatal(err) + } + return req + } + + tests := []struct { + name string + dump func() (io.ReadCloser, error) + }{ + {"DumpRequest", func() (io.ReadCloser, error) { + req := newReq() + _, err := DumpRequest(req, true) + return req.Body, err + }}, + {"DumpRequestOut", func() (io.ReadCloser, error) { + req := newReq() + _, err := DumpRequestOut(req, true) + return req.Body, err + }}, + {"DumpResponse", func() (io.ReadCloser, error) { + res := &http.Response{ + StatusCode: 200, + ProtoMajor: 1, + ProtoMinor: 1, + ContentLength: int64(len(body)), + Body: io.NopCloser(strings.NewReader(body)), + } + _, err := DumpResponse(res, true) + return res.Body, err + }}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b, err := tt.dump() + if err != nil { + t.Fatalf("%s: %v", tt.name, err) + } + // The body is still readable until it is closed. + got, err := io.ReadAll(b) + if err != nil { + t.Fatalf("ReadAll before Close: %v", err) + } + if string(got) != body { + t.Errorf("body before Close = %q, want %q", got, body) + } + if err := b.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + n, err := b.Read(make([]byte, 1)) + if n != 0 || err != http.ErrBodyReadAfterClose { + t.Errorf("Read after Close = (%v, %v), want (0, %v)", n, err, http.ErrBodyReadAfterClose) + } + }) + } +} + +// http.Request.Body requires that Close may be called concurrently with +// Read, so the body left behind by the Dump functions must allow it too. +// Run under -race to be meaningful. +func TestDumpBodyConcurrentReadClose(t *testing.T) { + req, err := http.NewRequest("POST", "http://example.com/", strings.NewReader(strings.Repeat("x", 4096))) + if err != nil { + t.Fatal(err) + } + if _, err := DumpRequest(req, true); err != nil { + t.Fatal(err) + } + body := req.Body + + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + buf := make([]byte, 16) + for { + if _, err := body.Read(buf); err != nil { + return + } + } + }() + go func() { + defer wg.Done() + body.Close() + }() + wg.Wait() +} + +// The body left behind by the Dump functions should still implement +// io.WriterTo, as the io.NopCloser it replaced did, so that copying from +// it does not need an intermediate buffer. +func TestDumpBodyWriterTo(t *testing.T) { + const body = "hello body" + req, err := http.NewRequest("POST", "http://example.com/", strings.NewReader(body)) + if err != nil { + t.Fatal(err) + } + if _, err := DumpRequest(req, true); err != nil { + t.Fatal(err) + } + + wt, ok := req.Body.(io.WriterTo) + if !ok { + t.Fatalf("dumped body %T does not implement io.WriterTo", req.Body) + } + var buf bytes.Buffer + if _, err := wt.WriteTo(&buf); err != nil { + t.Fatalf("WriteTo: %v", err) + } + if buf.String() != body { + t.Errorf("WriteTo wrote %q, want %q", buf.String(), body) + } + + // Once closed, WriteTo reports the same error as Read. + if err := req.Body.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + if _, err := wt.WriteTo(io.Discard); err != http.ErrBodyReadAfterClose { + t.Errorf("WriteTo after Close = %v, want %v", err, http.ErrBodyReadAfterClose) + } +} + // Issue 38352: Check for deadlock on canceled requests. func TestDumpRequestOutIssue38352(t *testing.T) { if testing.Short() {