caddyserver / caddy

Fast and extensible multi-platform HTTP/1-2-3 web server with automatic HTTPS
https://caddyserver.com
Apache License 2.0
55.45k stars 3.91k forks source link

Write the header if none had been written in WriteResponse #6380

Closed ankon closed 3 weeks ago

ankon commented 3 weeks ago

In one of our modules we had this code:

    buf := new(bytes.Buffer)
    shouldBuffer := func(status int, header http.Header) bool {
        // Returning false means headers/body will be streamed out directly to the underlying writer.
        return false
    }
    ww := caddyhttp.NewResponseRecorder(w, buf, shouldBuffer)

    // Do stuff with ww
    // ...

    // Write response and return error if needed
    return ww.WriteResponse()

This worked, but when reading through the sources of caddy and the documentation of NewResponseRecorder we realized that we could also initialize it without the buffer and function:

ww := caddyhttp.NewResponseRecorder(w, nil, nil)

This matches how caddy initializes it, and is explicitly allowed in the docs.

However, with that change we noticed panics happening:

runtime.gopanic (/usr/local/go/src/runtime/panic.go:770)
runtime.panicmem (/usr/local/go/src/runtime/panic.go:261)
runtime.sigpanic (/usr/local/go/src/runtime/signal_unix.go:881)
bytes.(*Buffer).WriteTo (/usr/local/go/src/bytes/buffer.go:259)
io.copyBuffer (/usr/local/go/src/io/io.go:411)
io.Copy (/usr/local/go/src/io/io.go:388)
caddyhttp.(*responseRecorder).WriteResponse (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/responsewriter.go:229)
caddy.(*Telemetry).captureResponse (/src/caddy/telemetry.go:312)
caddy.(*Telemetry).ServeHTTP (/src/caddy/telemetry.go:152)
caddyhttp.wrapMiddleware.func1.1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:331)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddy.(*NewRelic).ServeHTTP.func1 (/src/caddy/newrelic.go:69)
http.HandlerFunc.ServeHTTP (/usr/local/go/src/net/http/server.go:2166)
newrelic.WrapHandle.func1 (/src/vendor/github.com/newrelic/go-agent/v3/newrelic/instrumentation.go:81)
http.HandlerFunc.ServeHTTP (/usr/local/go/src/net/http/server.go:2166)
newrelic.WrapHandleFunc.func1 (/src/vendor/github.com/newrelic/go-agent/v3/newrelic/instrumentation.go:150)
http.HandlerFunc.ServeHTTP (/usr/local/go/src/net/http/server.go:2166)
caddy.(*NewRelic).ServeHTTP (/src/caddy/newrelic.go:72)
caddyhttp.wrapMiddleware.func1.1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:331)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddyhttp.RouteList.Compile.wrapRoute.func1.1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:300)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddyhttp.RouteList.Compile.wrapRoute.func1.1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:268)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddyhttp.RouteList.Compile.wrapRoute.func1.1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:268)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddyhttp.RouteList.Compile.wrapRoute.func1.1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:268)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddyhttp.(*Server).enforcementHandler (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/server.go:435)
caddyhttp.(*App).Provision.(*Server).wrapPrimaryRoute.func1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/server.go:411)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddyhttp.(*Server).ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/server.go:347)
http.serverHandler.ServeHTTP (/usr/local/go/src/net/http/server.go:3137)
http.initALPNRequest.ServeHTTP (/usr/local/go/src/net/http/server.go:3745)
http2.(*serverConn).runHandler (/src/vendor/golang.org/x/net/http2/server.go:2370)
runtime.goexit (/usr/local/go/src/runtime/asm_amd64.s:1695)

Checking the code shows that this seems to a confusion inside the response recorder: rr.stream is set by WriteHeader, so checking that and then realizing that the status code is unset (i.e. WriteHeader hadn't been called!) is the wrong way around.