From 478b73b6088f64a4e6d410d563fa85b0940bd4fd Mon Sep 17 00:00:00 2001 From: vatsalpatel Date: Sat, 15 Aug 2026 22:49:52 +0530 Subject: [PATCH 1/2] fix/webserver: recover gRPC handler panics, reject empty query nodes 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 #1139 Signed-off-by: vatsalpatel --- grpc/defaults/server.go | 39 ++++++++++ grpc/defaults/server_test.go | 140 +++++++++++++++++++++++++++++++++++ query/query_proto.go | 9 ++- query/query_proto_test.go | 44 +++++++++++ 4 files changed, 231 insertions(+), 1 deletion(-) create mode 100644 grpc/defaults/server_test.go diff --git a/grpc/defaults/server.go b/grpc/defaults/server.go index 714563d48..af149a644 100644 --- a/grpc/defaults/server.go +++ b/grpc/defaults/server.go @@ -1,14 +1,20 @@ package defaults import ( + "context" + "fmt" + "runtime" "sync" grpcprom "github.com/grpc-ecosystem/go-grpc-middleware/providers/prometheus" + "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/recovery" "github.com/prometheus/client_golang/prometheus" sglog "github.com/sourcegraph/log" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" "google.golang.org/grpc" + "google.golang.org/grpc/codes" "google.golang.org/grpc/reflection" + "google.golang.org/grpc/status" "github.com/sourcegraph/zoekt/grpc/internalerrs" "github.com/sourcegraph/zoekt/grpc/messagesize" @@ -19,6 +25,8 @@ import ( func NewServer(logger sglog.Logger, additionalOpts ...grpc.ServerOption) *grpc.Server { metrics := serverMetricsOnce() + recoveryOpt := recovery.WithRecoveryHandlerContext(panicRecoveryHandler(logger)) + opts := []grpc.ServerOption{ grpc.StatsHandler(otelgrpc.NewServerHandler()), grpc.ChainStreamInterceptor( @@ -27,6 +35,7 @@ func NewServer(logger sglog.Logger, additionalOpts ...grpc.ServerOption) *grpc.S metrics.StreamServerInterceptor(), messagesize.StreamServerInterceptor, internalerrs.LoggingStreamServerInterceptor(logger), + recovery.StreamServerInterceptor(recoveryOpt), ), grpc.ChainUnaryInterceptor( propagator.UnaryServerPropagator(tenant.Propagator{}), @@ -34,6 +43,7 @@ func NewServer(logger sglog.Logger, additionalOpts ...grpc.ServerOption) *grpc.S metrics.UnaryServerInterceptor(), messagesize.UnaryServerInterceptor, internalerrs.LoggingUnaryServerInterceptor(logger), + recovery.UnaryServerInterceptor(recoveryOpt), ), } @@ -51,6 +61,35 @@ func NewServer(logger sglog.Logger, additionalOpts ...grpc.ServerOption) *grpc.S return s } +// panicRecoveryHandler logs a recovered handler panic along with its stack and +// converts it into an Internal error. +// +// Nothing about the panic reaches the caller. Panic values here 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. The detail belongs in our logs. +func panicRecoveryHandler(logger sglog.Logger) recovery.RecoveryHandlerFuncContext { + return func(ctx context.Context, p any) error { + stack := make([]byte, 64<<10) + stack = stack[:runtime.Stack(stack, false)] + + // Without the method the log line is a stack blob with no indication of + // which call produced it. + method, ok := grpc.Method(ctx) + if !ok { + method = "unknown" + } + + logger.Error("recovered from panic in gRPC handler", + sglog.String("method", method), + sglog.String("panic", fmt.Sprint(p)), + sglog.String("stacktrace", string(stack)), + ) + + return status.Error(codes.Internal, "internal error") + } +} + // serviceMetricsOnce returns a singleton instance of the server metrics // that are shared across all gRPC servers that this process creates. // diff --git a/grpc/defaults/server_test.go b/grpc/defaults/server_test.go new file mode 100644 index 000000000..ac833ac56 --- /dev/null +++ b/grpc/defaults/server_test.go @@ -0,0 +1,140 @@ +package defaults + +import ( + "context" + "errors" + "io" + "net" + "strings" + "testing" + + "github.com/sourcegraph/log/logtest" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/status" + + webserverv1 "github.com/sourcegraph/zoekt/grpc/protos/zoekt/webserver/v1" +) + +// panicSecret is planted in the panic value so the test can assert that no part +// of it reaches the client. +const panicSecret = "shard /data/index/secret-repo_v16.00000.zoekt" + +type panickingServer struct { + webserverv1.UnimplementedWebserverServiceServer +} + +func (*panickingServer) Search(context.Context, *webserverv1.SearchRequest) (*webserverv1.SearchResponse, error) { + panic(panicSecret) +} + +func (*panickingServer) StreamSearch(*webserverv1.StreamSearchRequest, webserverv1.WebserverService_StreamSearchServer) error { + panic(panicSecret) +} + +// List does not panic, so the test can check the server is still serving +// afterwards. +func (*panickingServer) List(context.Context, *webserverv1.ListRequest) (*webserverv1.ListResponse, error) { + return &webserverv1.ListResponse{}, nil +} + +func newTestServer(t *testing.T) webserverv1.WebserverServiceClient { + t.Helper() + + s := NewServer(logtest.Scoped(t)) + webserverv1.RegisterWebserverServiceServer(s, &panickingServer{}) + + lis, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + + // Serve has to be joined before the test returns. Logging from a goroutine + // that outlives the test panics the whole package rather than failing this + // one test. + served := make(chan error, 1) + go func() { served <- s.Serve(lis) }() + t.Cleanup(func() { + s.Stop() + if err := <-served; err != nil { + t.Errorf("Serve returned: %v", err) + } + }) + + cc, err := grpc.NewClient(lis.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { cc.Close() }) + + return webserverv1.NewWebserverServiceClient(cc) +} + +// The interceptors are the whole point of this change, so exercise them through +// a real server rather than calling the recovery handler directly. This covers +// that recovery is registered at all, that both the unary and the stream +// interceptor are wired, and that a panic leaves the server able to serve. +func TestServerRecoversHandlerPanics(t *testing.T) { + client := newTestServer(t) + ctx := context.Background() + + assertRecovered := func(t *testing.T, err error) { + t.Helper() + + if got := status.Code(err); got != codes.Internal { + t.Fatalf("status code is %s (err=%v), want %s", got, err, codes.Internal) + } + // The panic value carries a shard path here, and callers may have no + // access to it. Neither it nor a stack trace should cross the wire. + if msg := status.Convert(err).Message(); strings.Contains(msg, panicSecret) || strings.Contains(msg, "goroutine ") { + t.Fatalf("panic detail reached the client: %q", msg) + } + } + + t.Run("unary", func(t *testing.T) { + _, err := client.Search(ctx, &webserverv1.SearchRequest{}) + assertRecovered(t, err) + }) + + t.Run("stream", func(t *testing.T) { + ss, err := client.StreamSearch(ctx, &webserverv1.StreamSearchRequest{}) + if err != nil { + t.Fatal(err) + } + for { + _, err = ss.Recv() + if err != nil { + break + } + } + if errors.Is(err, io.EOF) { + t.Fatal("stream ended cleanly, want the panic surfaced as an error") + } + assertRecovered(t, err) + }) + + t.Run("server still serving", func(t *testing.T) { + if _, err := client.List(ctx, &webserverv1.ListRequest{}); err != nil { + t.Fatalf("List after two panics: %v", err) + } + }) + +} + +func TestPanicRecoveryHandler(t *testing.T) { + handler := panicRecoveryHandler(logtest.Scoped(t)) + + err := handler(context.Background(), "boom") + if err == nil { + t.Fatal("handler returned nil, want an error") + } + + if got := status.Code(err); got != codes.Internal { + t.Errorf("status code is %s, want %s", got, codes.Internal) + } + + if strings.Contains(err.Error(), "goroutine ") { + t.Errorf("error sent to the client contains a stack trace: %s", err) + } +} diff --git a/query/query_proto.go b/query/query_proto.go index 621a12a7b..ea2f0dcd1 100644 --- a/query/query_proto.go +++ b/query/query_proto.go @@ -55,7 +55,14 @@ func QToProto(q Q) *webserverv1.Q { } } +// QFromProto converts a protobuf query node into a Q. The message is untrusted +// input, so a missing node or an unset oneof is reported as an error rather +// than panicking: these reach us straight from a gRPC request. func QFromProto(p *webserverv1.Q) (Q, error) { + if p == nil { + return nil, fmt.Errorf("query node is missing") + } + switch v := p.Query.(type) { case *webserverv1.Q_RawConfig: return RawConfigFromProto(v.RawConfig), nil @@ -96,7 +103,7 @@ func QFromProto(p *webserverv1.Q) (Q, error) { case *webserverv1.Q_Meta: return MetaFromProto(v.Meta) default: - panic(fmt.Sprintf("unknown query node %T", p.Query)) + return nil, fmt.Errorf("unknown query node %T", p.Query) } } diff --git a/query/query_proto_test.go b/query/query_proto_test.go index f966c0176..2dd1ef0b4 100644 --- a/query/query_proto_test.go +++ b/query/query_proto_test.go @@ -7,6 +7,8 @@ import ( "github.com/RoaringBitmap/roaring/v2" "github.com/google/go-cmp/cmp" "github.com/grafana/regexp" + + webserverv1 "github.com/sourcegraph/zoekt/grpc/protos/zoekt/webserver/v1" ) func TestQueryRoundtrip(t *testing.T) { @@ -112,6 +114,48 @@ func TestRegexpProtoUsesRegexpString(t *testing.T) { } } +// A query node arrives straight off the wire, so an absent node or an unset +// oneof has to come back as an error. Both used to panic, which took down the +// whole server since there is no recovery interceptor upstream of the handler. +func TestQFromProtoRejectsMissingNodes(t *testing.T) { + for _, tc := range []struct { + name string + q *webserverv1.Q + }{ + { + name: "nil node, as returned by GetQuery on a request with no query", + q: (&webserverv1.SearchRequest{}).GetQuery(), + }, + { + name: "node present but oneof unset", + q: &webserverv1.Q{}, + }, + { + name: "child node with oneof unset", + q: &webserverv1.Q{Query: &webserverv1.Q_And{And: &webserverv1.And{ + Children: []*webserverv1.Q{{}}, + }}}, + }, + { + name: "nil child node", + q: &webserverv1.Q{Query: &webserverv1.Q_Or{Or: &webserverv1.Or{ + Children: []*webserverv1.Q{nil}, + }}}, + }, + { + name: "wrapper with no child", + q: &webserverv1.Q{Query: &webserverv1.Q_Not{Not: &webserverv1.Not{}}}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + q, err := QFromProto(tc.q) + if err == nil { + t.Fatalf("QFromProto returned %v, want an error", q) + } + }) + } +} + func regexpMustParse(s string) *syntax.Regexp { re, err := syntax.Parse(s, syntax.Perl) if err != nil { From e98955f56138cba46f4afb32ab58519cc9b6a97c Mon Sep 17 00:00:00 2001 From: vatsalpatel Date: Mon, 17 Aug 2026 17:35:00 +0530 Subject: [PATCH 2/2] fix/webserver: Address requested changes --- grpc/defaults/server.go | 13 +--- grpc/defaults/server_test.go | 140 ----------------------------------- query/query_proto.go | 3 - query/query_proto_test.go | 44 ----------- 4 files changed, 4 insertions(+), 196 deletions(-) delete mode 100644 grpc/defaults/server_test.go diff --git a/grpc/defaults/server.go b/grpc/defaults/server.go index af149a644..281136218 100644 --- a/grpc/defaults/server.go +++ b/grpc/defaults/server.go @@ -61,20 +61,15 @@ func NewServer(logger sglog.Logger, additionalOpts ...grpc.ServerOption) *grpc.S return s } -// panicRecoveryHandler logs a recovered handler panic along with its stack and -// converts it into an Internal error. -// -// Nothing about the panic reaches the caller. Panic values here 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. The detail belongs in our logs. +// panicRecoveryHandler converts a recovered handler panic into an Internal +// error. Shard searches already recover their own panics in searchOneShard, so +// this only sees bugs in the layer between gRPC and the shard searchers. The +// panic value is logged rather than returned, since it is internal detail. func panicRecoveryHandler(logger sglog.Logger) recovery.RecoveryHandlerFuncContext { return func(ctx context.Context, p any) error { stack := make([]byte, 64<<10) stack = stack[:runtime.Stack(stack, false)] - // Without the method the log line is a stack blob with no indication of - // which call produced it. method, ok := grpc.Method(ctx) if !ok { method = "unknown" diff --git a/grpc/defaults/server_test.go b/grpc/defaults/server_test.go deleted file mode 100644 index ac833ac56..000000000 --- a/grpc/defaults/server_test.go +++ /dev/null @@ -1,140 +0,0 @@ -package defaults - -import ( - "context" - "errors" - "io" - "net" - "strings" - "testing" - - "github.com/sourcegraph/log/logtest" - "google.golang.org/grpc" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/credentials/insecure" - "google.golang.org/grpc/status" - - webserverv1 "github.com/sourcegraph/zoekt/grpc/protos/zoekt/webserver/v1" -) - -// panicSecret is planted in the panic value so the test can assert that no part -// of it reaches the client. -const panicSecret = "shard /data/index/secret-repo_v16.00000.zoekt" - -type panickingServer struct { - webserverv1.UnimplementedWebserverServiceServer -} - -func (*panickingServer) Search(context.Context, *webserverv1.SearchRequest) (*webserverv1.SearchResponse, error) { - panic(panicSecret) -} - -func (*panickingServer) StreamSearch(*webserverv1.StreamSearchRequest, webserverv1.WebserverService_StreamSearchServer) error { - panic(panicSecret) -} - -// List does not panic, so the test can check the server is still serving -// afterwards. -func (*panickingServer) List(context.Context, *webserverv1.ListRequest) (*webserverv1.ListResponse, error) { - return &webserverv1.ListResponse{}, nil -} - -func newTestServer(t *testing.T) webserverv1.WebserverServiceClient { - t.Helper() - - s := NewServer(logtest.Scoped(t)) - webserverv1.RegisterWebserverServiceServer(s, &panickingServer{}) - - lis, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatal(err) - } - - // Serve has to be joined before the test returns. Logging from a goroutine - // that outlives the test panics the whole package rather than failing this - // one test. - served := make(chan error, 1) - go func() { served <- s.Serve(lis) }() - t.Cleanup(func() { - s.Stop() - if err := <-served; err != nil { - t.Errorf("Serve returned: %v", err) - } - }) - - cc, err := grpc.NewClient(lis.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) - if err != nil { - t.Fatal(err) - } - t.Cleanup(func() { cc.Close() }) - - return webserverv1.NewWebserverServiceClient(cc) -} - -// The interceptors are the whole point of this change, so exercise them through -// a real server rather than calling the recovery handler directly. This covers -// that recovery is registered at all, that both the unary and the stream -// interceptor are wired, and that a panic leaves the server able to serve. -func TestServerRecoversHandlerPanics(t *testing.T) { - client := newTestServer(t) - ctx := context.Background() - - assertRecovered := func(t *testing.T, err error) { - t.Helper() - - if got := status.Code(err); got != codes.Internal { - t.Fatalf("status code is %s (err=%v), want %s", got, err, codes.Internal) - } - // The panic value carries a shard path here, and callers may have no - // access to it. Neither it nor a stack trace should cross the wire. - if msg := status.Convert(err).Message(); strings.Contains(msg, panicSecret) || strings.Contains(msg, "goroutine ") { - t.Fatalf("panic detail reached the client: %q", msg) - } - } - - t.Run("unary", func(t *testing.T) { - _, err := client.Search(ctx, &webserverv1.SearchRequest{}) - assertRecovered(t, err) - }) - - t.Run("stream", func(t *testing.T) { - ss, err := client.StreamSearch(ctx, &webserverv1.StreamSearchRequest{}) - if err != nil { - t.Fatal(err) - } - for { - _, err = ss.Recv() - if err != nil { - break - } - } - if errors.Is(err, io.EOF) { - t.Fatal("stream ended cleanly, want the panic surfaced as an error") - } - assertRecovered(t, err) - }) - - t.Run("server still serving", func(t *testing.T) { - if _, err := client.List(ctx, &webserverv1.ListRequest{}); err != nil { - t.Fatalf("List after two panics: %v", err) - } - }) - -} - -func TestPanicRecoveryHandler(t *testing.T) { - handler := panicRecoveryHandler(logtest.Scoped(t)) - - err := handler(context.Background(), "boom") - if err == nil { - t.Fatal("handler returned nil, want an error") - } - - if got := status.Code(err); got != codes.Internal { - t.Errorf("status code is %s, want %s", got, codes.Internal) - } - - if strings.Contains(err.Error(), "goroutine ") { - t.Errorf("error sent to the client contains a stack trace: %s", err) - } -} diff --git a/query/query_proto.go b/query/query_proto.go index ea2f0dcd1..38696a7c6 100644 --- a/query/query_proto.go +++ b/query/query_proto.go @@ -55,9 +55,6 @@ func QToProto(q Q) *webserverv1.Q { } } -// QFromProto converts a protobuf query node into a Q. The message is untrusted -// input, so a missing node or an unset oneof is reported as an error rather -// than panicking: these reach us straight from a gRPC request. func QFromProto(p *webserverv1.Q) (Q, error) { if p == nil { return nil, fmt.Errorf("query node is missing") diff --git a/query/query_proto_test.go b/query/query_proto_test.go index 2dd1ef0b4..f966c0176 100644 --- a/query/query_proto_test.go +++ b/query/query_proto_test.go @@ -7,8 +7,6 @@ import ( "github.com/RoaringBitmap/roaring/v2" "github.com/google/go-cmp/cmp" "github.com/grafana/regexp" - - webserverv1 "github.com/sourcegraph/zoekt/grpc/protos/zoekt/webserver/v1" ) func TestQueryRoundtrip(t *testing.T) { @@ -114,48 +112,6 @@ func TestRegexpProtoUsesRegexpString(t *testing.T) { } } -// A query node arrives straight off the wire, so an absent node or an unset -// oneof has to come back as an error. Both used to panic, which took down the -// whole server since there is no recovery interceptor upstream of the handler. -func TestQFromProtoRejectsMissingNodes(t *testing.T) { - for _, tc := range []struct { - name string - q *webserverv1.Q - }{ - { - name: "nil node, as returned by GetQuery on a request with no query", - q: (&webserverv1.SearchRequest{}).GetQuery(), - }, - { - name: "node present but oneof unset", - q: &webserverv1.Q{}, - }, - { - name: "child node with oneof unset", - q: &webserverv1.Q{Query: &webserverv1.Q_And{And: &webserverv1.And{ - Children: []*webserverv1.Q{{}}, - }}}, - }, - { - name: "nil child node", - q: &webserverv1.Q{Query: &webserverv1.Q_Or{Or: &webserverv1.Or{ - Children: []*webserverv1.Q{nil}, - }}}, - }, - { - name: "wrapper with no child", - q: &webserverv1.Q{Query: &webserverv1.Q_Not{Not: &webserverv1.Not{}}}, - }, - } { - t.Run(tc.name, func(t *testing.T) { - q, err := QFromProto(tc.q) - if err == nil { - t.Fatalf("QFromProto returned %v, want an error", q) - } - }) - } -} - func regexpMustParse(s string) *syntax.Regexp { re, err := syntax.Parse(s, syntax.Perl) if err != nil {