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

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,13 +35,15 @@ 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{}),
 			tenant.UnaryServerInterceptor,
 			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.
 //

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)
+	}
+}

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)
 	}
 }
 

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 {