Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: Write() or Flush() on http.ResponseWriter after http.Handler has finished causes either panic or writing to response of ANOTHER request. #19959

Closed
matope opened this issue Apr 13, 2017 · 2 comments

Comments

Projects
None yet
4 participants
@matope
Copy link
Contributor

commented Apr 13, 2017

I've found 2 issues around Flush() ing on http.ResponseWriter after the http.Handler has finished. The first problem is it causes panic. The later one is that Write()ing on the http.ResponseWriter is redirected to the response of another request. I'd like to report them.

My version and env

go version go1.8 darwin/amd64

GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/8m/15jd53qj3sngxm2y8gsqm6252h4p5m/T/go-build435976512=/tmp/go-build -gno-record-gcc-switches -fno-common"

Issue 1(panic)

I can reproduce it on playground.

https://play.golang.org/p/63aDsPEvZ1

Flush() to the ResponseWriter after the http.ResponseWriter from separated goroutine causes a panic. I think it's because the http.response.w is reset with Reset(nil) to put it to the buffer pool(ref: go/server.go at master · golang/go ) in order to reuse it. And bufio.Writer.Flush() with nil underlying-buffer causes panic (ref: #9657 )).

&{Status:200 OK StatusCode:200 Proto:HTTP/1.1 ProtoMajor:1 ProtoMinor:1 Header:map[Content-Length:[1] Date:[Tue, 10 Nov 2009 23:00:00 GMT] Content-Type:[text/plain; charset=utf-8]] Body:0x10732780 ContentLength:1 TransferEncoding:[] Close:false Uncompressed:false Trailer:map[] Request:0x10760360 TLS:<nil>}
sleeping...
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0xffffffff addr=0x0 pc=0x131262]

goroutine 12 [running]:
bufio.(*Writer).Flush(0x0, 0x2dede0, 0x0, 0x3e0b00)
	/usr/local/go/src/bufio/bufio.go:560 +0x22
net/http.(*chunkWriter).flush(0x1076052c, 0x1073e700)
	/usr/local/go/src/net/http/server.go:374 +0x60
net/http.(*response).Flush(0x10760510, 0x3e2c20)
	/usr/local/go/src/net/http/server.go:1592 +0x60
main.main.func1.1(0x3e2c20, 0x10760510)
	/tmp/sandbox037200695/main.go:21 +0x140
created by main.main.func1
	/tmp/sandbox037200695/main.go:22 +0xc0

I think it's not very good manner writing to ResponseWriter after the handler has been finished. But we don't have ways to know if the handler has finished exactly (before the bufio.Writer.Reset(nil)).

Also you can use repeated Write() insted of Flush() because bufio.Writer.Write() potentially invoke its Flush() internally.

Issue 2(writing to the response of another request)

To make matters worse, because of the reuse of http.response.w, we can write to the response of another request unexpectedly (or expectedly).

https://play.golang.org/p/5z6VbtwqnR

body from /1:
body from /2:THIS IS DATA WRITTEN FOR /1

workaround to handle this issue

I think we can avoid this issue like below.

fixed version of first problem. https://play.golang.org/p/jfn1cG0006
fixed version of later problem. https://play.golang.org/p/7hmjZDuE7a

type safeResponseWriter struct {
	handlerDone int64
	http.ResponseWriter
}

func (w *safeResponseWriter) Write(p []byte) (int, error) {
	if atomic.LoadInt64(&w.handlerDone) == 0 {
		return w.ResponseWriter.Write(p)
	}
	return 0, errors.New("handler done")
}

func (w *safeResponseWriter) Flush() {
	if atomic.LoadInt64(&w.handlerDone) == 0 {
		w.ResponseWriter.(http.Flusher).Flush()
	}
}

func (w *safeResponseWriter) Done() {
	atomic.StoreInt64(&w.handlerDone, 1)
}

ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	ww := &safeResponseWriter{ResponseWriter: w}
	defer ww.Done()
	w = ww
	
	// ...
}

IMHO, though the first issue(panic) might not be a big problem, the later one should be fixed (in net/http package). I think it might be a kind of vulnerability because third party HTTP middleware could hijack any other HTTP handler running on the same HTTP.Server.

How do you think about this?

@matope matope changed the title net/http: Flush() (or Write()) on http.ResponseWriter after http.Handler has finished causes either panic or writing to response of ANOTHER request. net/http: Write() or Flush() on http.ResponseWriter after http.Handler has finished causes either panic or writing to response of ANOTHER request. Apr 13, 2017

@danp

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2017

https://golang.org/pkg/net/http/#ResponseWriter says:

A ResponseWriter may not be used after the Handler.ServeHTTP method has returned.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 13, 2017

Correct.

@bradfitz bradfitz closed this Apr 13, 2017

@golang golang locked and limited conversation to collaborators Apr 13, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.