QFromProto panics on a query message that a client can trivially send, and the gRPC server has no recovery interceptor, so the panic takes down the whole process rather than failing the one RPC.
Reproduced on main at b0de0bb.
The panic
QFromProto reads the oneof with a direct field access and panics in its default branch:
// query/query_proto.go:58
func QFromProto(p *webserverv1.Q) (Q, error) {
switch v := p.Query.(type) {
// ...
default:
panic(fmt.Sprintf("unknown query node %T", p.Query))
}
}Server.Search, StreamSearch and List all call it as query.QFromProto(req.GetQuery()), and the generated getter returns nil when the field is absent. Since p.Query is a field access rather than a method call, a nil p faults instead of reaching the default branch. That gives two ways in.
A request with no query field segfaults on line 59:
q, err := QFromProto((&webserverv1.SearchRequest{}).GetQuery())panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x28]
github.com/sourcegraph/zoekt/query.QFromProto query/query_proto.go:59
A request carrying a Q whose oneof is unset reaches the default branch and hits the explicit panic on line 99:
q, err := QFromProto(&webserverv1.Q{})panic: unknown query node <nil>
github.com/sourcegraph/zoekt/query.QFromProto query/query_proto.go:99
The second form applies to nested children too, so a well-formed outer query containing an empty child, such as And{children: [{}]}, does the same.
Why it kills the process
grpc/defaults/server.go builds the server option list and never installs a recovery interceptor:
opts := []grpc.ServerOption{
grpc.StatsHandler(otelgrpc.NewServerHandler()),
grpc.ChainStreamInterceptor(
propagator.StreamServerPropagator(tenant.Propagator{}),
tenant.StreamServerInterceptor,
metrics.StreamServerInterceptor(),
messagesize.StreamServerInterceptor,
internalerrs.LoggingStreamServerInterceptor(logger),
),
grpc.ChainUnaryInterceptor(
// ... same, no recovery
),
}grpc-go does not recover handler panics on its own, so any panic inside a handler goroutine terminates the process. Both zoekt-webserver and zoekt-sourcegraph-indexserver build their servers through this constructor.
There is a recover in the search path, but it only wraps the per-shard call in searchOneShard (search/shards.go:976), which is well below the handler. It does not help here, and it does not help for any other panic raised outside a single shard search.
reflection.Register(s) is called on the same server, so a caller needs no schema knowledge to construct either request.
Suggested fix
Two parts, and they are worth doing together.
Handle nil p and an unset oneof as errors in QFromProto. The three handlers already convert a returned error into codes.InvalidArgument alongside the other query parse failures, so no caller changes are needed.
Add a recovery interceptor in defaults.NewServer. github.com/grpc-ecosystem/go-grpc-middleware/v2 is already a direct dependency, so this needs no new module. Without it, the next panic from anywhere in a handler is still fatal.
Worth noting that the recovery interceptor alone is not enough here. It would stop the crash, but every malformed request would then log a full stack trace, so a client looping on SearchRequest{} turns a crash into a log volume problem. Fixing QFromProto makes those requests cost an ordinary InvalidArgument instead.
QFromProtopanics on a query message that a client can trivially send, and the gRPC server has no recovery interceptor, so the panic takes down the whole process rather than failing the one RPC.Reproduced on
mainat b0de0bb.The panic
QFromProtoreads the oneof with a direct field access and panics in its default branch:Server.Search,StreamSearchandListall call it asquery.QFromProto(req.GetQuery()), and the generated getter returns nil when the field is absent. Sincep.Queryis a field access rather than a method call, a nilpfaults instead of reaching the default branch. That gives two ways in.A request with no
queryfield segfaults on line 59:A request carrying a
Qwhose oneof is unset reaches the default branch and hits the explicit panic on line 99:The second form applies to nested children too, so a well-formed outer query containing an empty child, such as
And{children: [{}]}, does the same.Why it kills the process
grpc/defaults/server.gobuilds the server option list and never installs a recovery interceptor:grpc-go does not recover handler panics on its own, so any panic inside a handler goroutine terminates the process. Both
zoekt-webserverandzoekt-sourcegraph-indexserverbuild their servers through this constructor.There is a recover in the search path, but it only wraps the per-shard call in
searchOneShard(search/shards.go:976), which is well below the handler. It does not help here, and it does not help for any other panic raised outside a single shard search.reflection.Register(s)is called on the same server, so a caller needs no schema knowledge to construct either request.Suggested fix
Two parts, and they are worth doing together.
Handle nil
pand an unset oneof as errors inQFromProto. The three handlers already convert a returned error intocodes.InvalidArgumentalongside the other query parse failures, so no caller changes are needed.Add a recovery interceptor in
defaults.NewServer.github.com/grpc-ecosystem/go-grpc-middleware/v2is already a direct dependency, so this needs no new module. Without it, the next panic from anywhere in a handler is still fatal.Worth noting that the recovery interceptor alone is not enough here. It would stop the crash, but every malformed request would then log a full stack trace, so a client looping on
SearchRequest{}turns a crash into a log volume problem. FixingQFromProtomakes those requests cost an ordinaryInvalidArgumentinstead.