defer Close your gzip handler will prevent panics from writing http headers

[azr]

Hello there !

https://github.com/golang/go/blob/d0e8d3a7ae2194b1753bc4e419d5f00aa2d5cb86/src/compress/gzip/gzip.go#L235

+

https://github.com/NYTimes/gziphandler/blob/master/gzip.go#L115

This causes StatusCode to be 200 on panics.

Defer will close the gzip handler before the http handler to catch the panic

We had the same issue here : https://github.com/BakedSoftware/go-parameters/pull/6/files

[azr]

Sorry I was totally wrong for you :)

[azr]

Whoops I actually found a way to reproduce: https://play.golang.org/p/zDqhbl4Nnn

log outputs : 2016/08/22 17:24:16 http: multiple response.WriteHeader calls

$ curl -H "Accept-Encoding: gzip" -i localhost:8000
HTTP/1.1 200 OK
Content-Encoding: gzip
Vary: Accept-Encoding
Date: Mon, 22 Aug 2016 15:24:18 GMT
Content-Length: 23
Content-Type: application/x-gzip

�       n����%

[adammck]

Ouch! That's a nasty bug. Thank you very much for reporting it, and especially for including a POC. The solution isn't immediately obvious to me, but so long as we call Reset before attempting to re-use the gzip writers (which we do), I see no harm in simply not calling Close when a panic occurs. Perhaps we should (partially) revert #8?

[elithrar]

You could also just call Reset explicitly instead of Close (as a precaution).

On Mon, Aug 22, 2016 at 8:46 AM Adam Mckaig notifications@github.com wrote:

Ouch! That's a nasty bug. Thank you very much for reporting it, and especially for including a POC. The solution isn't immediately obvious to me, but so long as we call Reset before attempting to re-use the gzip writers (which we do), I see no harm in simply not calling Close when a panic occurs. Perhaps we should (partially) revert #8 https://github.com/NYTimes/gziphandler/pull/8?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/NYTimes/gziphandler/issues/15#issuecomment-241456330, or mute the thread https://github.com/notifications/unsubscribe-auth/AABIcLfhMi0aryIkpJ0M0fZPHXhWvisGks5qicRQgaJpZM4Jp5xq .

[adammck]

We already call Reset immediately after grabbing a gzw from the pool, so I think omitting the Close should be okay.

[azr]

Hey, you're welcome :)

[jameshartig]

@azr Looks like https://github.com/NYTimes/gziphandler/commit/d3d7d6775a254572f12ff981eaea827229eb2925 fixed your POC. Now when Close() is called, gw == nil which causes it to immediately return.

If you write anything inside of CatchPanic it won't be gzip'd but that's expected since its wrapping withGz. So this can be closed.