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.

Sign up for GitHub

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

Jump to bottom

net/http: Program crash when handling panic with wrapped http.ErrAbortHandler #62510

Open
ewohltman opened this issue Sep 7, 2023 · 4 comments
Open

net/http: Program crash when handling panic with wrapped http.ErrAbortHandler #62510

ewohltman opened this issue Sep 7, 2023 · 4 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@ewohltman
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.21.0 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN='/home/eric/work/bin'
GOCACHE='/home/eric/.cache/go-build'
GOENV='/home/eric/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/eric/work/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/eric/work'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/eric/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/eric/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build978392017=/tmp/go-build -gno-record-gcc-switches'

What did you do?

The net/http.conn.serve method is equipped to handle a special case for when panic is called with http.ErrAbortHandler by downstream http.Handler implementations where it silently recovers and prevents the program from crashing.

When chaining http.Handler implementations, if an http.Handler uses golang.org/x/sync/singleflight.Group, and an http.Handler downstream of that panics with http.ErrAbortHandler, the singleflight package recovers from the panic first, wraps the panic value in its own private error type, and re-panics with its private type. When the new panic gets to net/http.conn.serve, a direct comparison is made to see if err != ErrAbortHandler. Since technically err is not ErrAbortHandler, just another error type that wraps ErrAbortHandler, net/http.conn.serve does not handle the special case and allows the program to crash.

Example where net/http.conn.serve handles the panic and the program does not crash:

Example using golang.org/x/sync/singleflight.Group where net/http.conn.serve does not handle the panic and the program crashes:

What did you expect to see?

When using golang.org/x/sync/singleflight.Group in a chain of http.Handler implementations where a panic(http.ErrAbortHandler is called downstream, net/http.conn.serve is able to unwrap errors to determine if the error is actually http.ErrAbortHandler and handle the panic without allowing the program to crash.

What did you see instead?

The program crashes with the message panic: net/http: abort Handler.
@ewohltman ewohltman mentioned this issue Sep 7, 2023
Closed
@ewohltman ewohltman changed the title affected/package: net/http affected/package: net/http: Program crash when handling panic with wrapped http.ErrAbortHandler Sep 7, 2023
@gopherbot
Copy link

Change https://go.dev/cl/526418 mentions this issue: net/http: unwrap errors when checking for ErrAbortHandler

@cherrymui cherrymui added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 7, 2023
@cherrymui cherrymui changed the title affected/package: net/http: Program crash when handling panic with wrapped http.ErrAbortHandler net/http: Program crash when handling panic with wrapped http.ErrAbortHandler Sep 7, 2023
@cherrymui cherrymui added this to the Backlog milestone Sep 7, 2023
@cherrymui
Copy link
Member

cc @neild

@gopherbot
Copy link

Change https://go.dev/cl/526171 mentions this issue: singleflight: add panicError.Unwrap method

@neild
Copy link
Contributor

neild commented Sep 8, 2023

Since technically err is not ErrAbortHandler, just another error type that wraps ErrAbortHandler, net/http.conn.serve does not handle the special case and allows the program to crash.

net/http swallows all panics from handlers. ErrAbortHandler just controls whether it logs an error or not.If the program is crashing, something else is going on.

The net/http change in https://go.dev/cl/526418 seems reasonable to me in isolation--we're checking for ErrAbortHandler, I don't see a good reason not to check for errors which wrap ErrAbortHandler.

singleflight's abstraction is leaky for panics: If the function passed to singleflight.Group.Do panics, then Do propagates the panic, but it hides the original panic value in a type which adds a stack trace. There is no way for the caller to recover the original panic.

singleflight does not document how it handles panics.

https://go.dev/cl/526171 proposes changing singleflight to wrap the original panic value, when that value is an error. This will allow recovering the original panic value, but only when that value is an error.

I'm uncertain about the special handling of error values; what if the panic value isn't an error, but the caller wants it anyway? On the other hand, it seems useful for singleflight to add stack information for the source of the original panic, and when the original value is an error using error wrapping to augment that error is fairly sensible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@neild @ewohltman @gopherbot @cherrymui