diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index ac1f84c1b..a74e30b11 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -53,4 +53,5 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi - [blockchain/v2] [\#4761](https://github.com/tendermint/tendermint/pull/4761) Fix excessive CPU usage caused by spinning on closed channels (@erikgrinaker) - [blockchain/v2] Respect `fast_sync` option (@erikgrinaker) - [light] [\#4741](https://github.com/tendermint/tendermint/pull/4741) Correctly return `ErrSignedHeaderNotFound` and `ErrValidatorSetNotFound` on corresponding RPC errors (@erikgrinaker) +- [rpc] \#4805 Attempt to handle panics during panic recovery (@erikgrinaker) - [types] [\#4764](https://github.com/tendermint/tendermint/pull/4764) Return an error if voting power overflows in `VerifyCommitTrusting` (@melekes) diff --git a/rpc/lib/server/http_server.go b/rpc/lib/server/http_server.go index 501396867..2f24b0545 100644 --- a/rpc/lib/server/http_server.go +++ b/rpc/lib/server/http_server.go @@ -7,6 +7,7 @@ import ( "fmt" "net" "net/http" + "os" "runtime/debug" "strings" "time" @@ -145,6 +146,20 @@ func RecoverAndLogHandler(handler http.Handler, logger log.Logger) http.Handler rww.Header().Set("X-Server-Time", fmt.Sprintf("%v", begin.Unix())) + defer func() { + // Handle any panics in the panic handler below. Does not use the logger, since we want + // to avoid any further panics. However, we try to return a 500, since it otherwise + // defaults to 200 and there is no other way to terminate the connection. If that + // should panic for whatever reason then the Go HTTP server will handle it and + // terminate the connection - panicing is the de-facto and only way to get the Go HTTP + // server to terminate the request and close the connection/stream: + // https://github.com/golang/go/issues/17790#issuecomment-258481416 + if e := recover(); e != nil { + fmt.Fprintf(os.Stderr, "Panic during RPC panic recovery: %v\n%v\n", e, string(debug.Stack())) + w.WriteHeader(500) + } + }() + defer func() { // Send a 500 error if a panic happens during a handler. // Without this, Chrome & Firefox were retrying aborted ajax requests, @@ -155,7 +170,18 @@ func RecoverAndLogHandler(handler http.Handler, logger log.Logger) http.Handler if res, ok := e.(types.RPCResponse); ok { WriteRPCResponseHTTP(rww, res) } else { - // For the rest, + // Panics can contain anything, attempt to normalize it as an error. + var err error + switch e := e.(type) { + case error: + err = e + case string: + err = errors.New(e) + case fmt.Stringer: + err = errors.New(e.String()) + default: + } + logger.Error( "Panic in RPC HTTP handler", "err", e, "stack", string(debug.Stack()), @@ -163,7 +189,7 @@ func RecoverAndLogHandler(handler http.Handler, logger log.Logger) http.Handler WriteRPCResponseHTTPError( rww, http.StatusInternalServerError, - types.RPCInternalError(types.JSONRPCIntID(-1), e.(error)), + types.RPCInternalError(types.JSONRPCIntID(-1), err), ) } }