Skip to content

fix/webserver: recover gRPC handler panics and reject empty query nodes - #1140

Open
vatsalpatel wants to merge 2 commits into
sourcegraph:mainfrom
vatsalpatel:fix/grpc-panic-recovery
Open

fix/webserver: recover gRPC handler panics and reject empty query nodes#1140
vatsalpatel wants to merge 2 commits into
sourcegraph:mainfrom
vatsalpatel:fix/grpc-panic-recovery

Conversation

@vatsalpatel

Copy link
Copy Markdown

A gRPC request with an empty query message crashes zoekt-webserver. QFromProto panics on it, and nothing recovers handler panics, so the process goes down instead of the request failing.

QFromProto

The oneof was read with a direct field access and the default branch panicked, so SearchRequest{} faulted on p.Query and a bare Q{} hit the explicit panic. Both now return an error. The three handlers in cmd/zoekt-webserver/grpc/server/server.go already turn a returned error into codes.InvalidArgument, so there are no caller changes. Nested children are covered too, so And{children: [{}]} is rejected rather than taking the server down.

Recovery interceptor

defaults.NewServer now installs recovery.UnaryServerInterceptor and recovery.StreamServerInterceptor from go-grpc-middleware, which is already a direct dependency, so go.mod and go.sum are unchanged.

Recovery sits last in both chains, closest to the handler, so the resulting codes.Internal still passes back out through the metrics interceptor and shows up in grpc_server_handled_total. Note that it does not show up in the internalerrs logging above it: that interceptor only logs errors that look like they came from go-grpc itself, matching on a "grpc: " prefix and two other specific shapes, and a flat "internal error" matches none of them. The logging is therefore done by the recovery handler, which the comment on the ordering now says explicitly.

The trade-off of putting recovery innermost is that a panic inside one of the interceptors above it is still fatal. That seemed like the right balance, since the handlers are the exposed surface and the interceptors are not, but I am happy to move it outermost if you would rather cover everything and give up the metrics accuracy.

The handler logs the panic, its stack and the gRPC method through the existing sglog.Logger, and returns a flat codes.Internal with no detail. Nothing about the panic reaches the caller on purpose: panic values on this path routinely carry shard paths, repository names and indexed file names, for example the corrupt shard reports in index/contentprovider.go, and the caller may have no access to any of it.

Tests

TestServerRecoversHandlerPanics runs a real server built by NewServer with a panicking service registered, over a real client on a loopback listener. It covers both the unary and the stream interceptor, asserts the client gets Internal with neither the panic value nor a stack in the message, and asserts a later List still succeeds. Removing the two interceptor lines kills the test binary, which is the point: the wiring is what this change is.

TestPanicRecoveryHandler covers the handler directly, for the status code and the absence of a stack in the client-visible message.

TestQFromProtoRejectsMissingNodes covers a nil node, an unset oneof, an unset oneof on a child, a nil child, and a wrapper with no child. Every one of those panicked before this change.

go test ./... passes, and go test ./grpc/defaults/ -race -count=30 passes.

Fixes #1139

A gRPC request with an empty query message crashes zoekt-webserver. QFromProto read the oneof with a direct field access and panicked in its default branch, so a request with no query field faulted on p.Query and a bare Q{} hit the explicit panic. Both now return an error, which the three handlers already map to codes.InvalidArgument.
grpc-go does not recover handler panics, and defaults.NewServer installed no recovery interceptor, so any panic below a handler took down the process rather than failing the one RPC. Add recovery.UnaryServerInterceptor and recovery.StreamServerInterceptor from go-grpc-middleware, which is already a direct dependency.
Recovery sits last in both chains, closest to the handler, so the resulting Internal error still reaches the metrics interceptor above it. The handler logs the method, panic value and stack, and returns a flat Internal with no detail: panic values here carry shard paths and repository names the caller may have no access to.

Fixes sourcegraph#1139

Signed-off-by: vatsalpatel <vatsalpatel.me@gmail.com>

@keegancsmith keegancsmith left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find on the empty Q, indeed that should of been validated before. Minor inline feedback mostly about cleaning up your agents over eagerness for writing lots of test code.

Comment thread grpc/defaults/server.go Outdated
Comment thread grpc/defaults/server_test.go Outdated
Comment thread query/query_proto.go Outdated
Comment thread query/query_proto_test.go Outdated
@vatsalpatel

Copy link
Copy Markdown
Author

@keegancsmith Thank you for the review
I have addressed all of the comments, please review again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A gRPC request with an empty query crashes zoekt-webserver

2 participants