connect-go icon indicating copy to clipboard operation
connect-go copied to clipboard
verified publisher icon connectrpc

Typed nil prevents correct nil checks in interceptors

Open replu opened this issue 10 months ago • 5 comments

Describe the bug connect-go internally uses typed nil, which causes issues when performing a nil check in an interceptor. Since the returned value is a typed nil, a standard if value == nil check does not work as expected, potentially leading to incorrect behavior in interceptors.

To Reproduce

example.proto

syntax = "proto3";

package example.v1;

option go_package = "example/gen/example/v1;examplev1";

message PingRequest {
  string message = 1;
}

message PingResponse {
  string message = 1;
}

service ExampleService {
  rpc Ping(PingRequest) returns (PingResponse) {}
}

example_test.go:

package bugreport

import (
	"context"
	"fmt"
	"net"
	"net/http"
	"testing"

	"connectrpc.com/connect"
	"golang.org/x/net/http2"
	"golang.org/x/net/http2/h2c"

	examplev1 "example/gen/example/v1"
	"example/gen/example/v1/examplev1connect"
)

type ExampleServer struct{}

func (s *ExampleServer) Ping(
	ctx context.Context,
	req *connect.Request[examplev1.PingRequest],
) (*connect.Response[examplev1.PingResponse], error) {
	return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("error"))
}

func exampleInterceptor() connect.UnaryInterceptorFunc {
	return func(next connect.UnaryFunc) connect.UnaryFunc {
		return func(ctx context.Context, req connect.AnyRequest) (connect.AnyResponse, error) {
			res, err := next(ctx, req)
			if res != nil {
				fmt.Println("res is not nil")
				res.Header().Add("example", "example") // <- this operation is panic (res is nil)
			}

			return res, err
		}
	}
}

type serverAndClient struct {
	server *http.Server
	client examplev1connect.ExampleServiceClient
}

func setupServerAndClient(
	t *testing.T,
) *serverAndClient {
	t.Helper()

	es := &ExampleServer{}
	mux := http.NewServeMux()
	mux.Handle(examplev1connect.NewExampleServiceHandler(es, connect.WithInterceptors(exampleInterceptor())))

	li, err := net.Listen("tcp", "127.0.0.1:0")
	if err != nil {
		t.Fatal(err)
	}
	_, port, err := net.SplitHostPort(li.Addr().String())
	if err != nil {
		t.Fatal(err)
	}
	srv := &http.Server{
		// Use h2c so we can serve HTTP/2 without TLS.
		Handler: h2c.NewHandler(mux, &http2.Server{}),
	}
	go srv.Serve(li)

	client := examplev1connect.NewExampleServiceClient(
		http.DefaultClient,
		fmt.Sprintf("http://localhost:%s", port),
	)

	return &serverAndClient{
		server: srv,
		client: client,
	}
}

func (r *serverAndClient) close(t *testing.T) {
	t.Helper()
	err := r.server.Close()
	if err != nil {
		t.Fatal(err)
	}
}

func TestThatReproducesBug(t *testing.T) {
	sc := setupServerAndClient(t)
	t.Cleanup(func() {
		sc.close(t)
	})

	c := sc.client

	ctx := t.Context()
	c.Ping(ctx, &connect.Request[examplev1.PingRequest]{Msg: &examplev1.PingRequest{}})
}

test result

res is not nil

2025/02/13 18:43:27 http: panic serving 127.0.0.1:62326: runtime error: invalid memory address or nil pointer dereference
goroutine 13 [running]:
net/http.(*conn).serve.func1()
        /Users/replu/sdk/go1.24.0/src/net/http/server.go:1947 +0xb0
panic({0x101058020?, 0x1013f5400?})
        /Users/replu/sdk/go1.24.0/src/runtime/panic.go:787 +0x124
connectrpc.com/connect.(*Response[...]).Header(...)
        /Users/replu/dev/pkg/mod/connectrpc.com/[email protected]/connect.go:265
example.setupServerAndClient.exampleInterceptor.func1.1({0x1010f5150?, 0x140001b6000?}, {0x1010f7230?, 0x140001ba080?})
        /Users/replu/dev/src/tmp/example_test.go:33 +0x80
connectrpc.com/connect.NewUnaryHandler[...].func2({0x1017a81e0, 0x1400018a080})
        /Users/replu/dev/pkg/mod/connectrpc.com/[email protected]/handler.go:69 +0x80
connectrpc.com/connect.(*Handler).ServeHTTP(0x14000160150, {0x1010f4a88, 0x140001b8000}, 0x14000162280)
        /Users/replu/dev/pkg/mod/connectrpc.com/[email protected]/handler.go:237 +0x7e8
example/gen/example/v1/examplev1connect.NewExampleServiceHandler.func1({0x1010f4a88, 0x140001b8000}, 0x14000162280)
        /Users/replu/dev/src/tmp/gen/example/v1/examplev1connect/example.connect.go:100 +0x90
net/http.HandlerFunc.ServeHTTP(0x1400013a0c0?, {0x1010f4a88?, 0x140001b8000?}, 0x0?)
        /Users/replu/sdk/go1.24.0/src/net/http/server.go:2294 +0x38
net/http.(*ServeMux).ServeHTTP(0x14000182150?, {0x1010f4a88, 0x140001b8000}, 0x14000162280)
        /Users/replu/sdk/go1.24.0/src/net/http/server.go:2822 +0x1b4
golang.org/x/net/http2/h2c.h2cHandler.ServeHTTP({{0x1010f0c40?, 0x1400013a0c0?}, 0x140001601c0?}, {0x1010f4a88, 0x140001b8000}, 0x14000162280)
        /Users/replu/dev/pkg/mod/golang.org/x/[email protected]/http2/h2c/h2c.go:125 +0x4a8
net/http.serverHandler.ServeHTTP({0x14000200870?}, {0x1010f4a88?, 0x140001b8000?}, 0x1?)
        /Users/replu/sdk/go1.24.0/src/net/http/server.go:3301 +0xbc
net/http.(*conn).serve(0x14000172870, {0x1010f5118, 0x14000182120})
        /Users/replu/sdk/go1.24.0/src/net/http/server.go:2102 +0x52c
created by net/http.(*Server).Serve in goroutine 5
        /Users/replu/sdk/go1.24.0/src/net/http/server.go:3454 +0x3d8
PASS
ok      example 0.541s

Environment (please complete the following information):

  • connect-go version or commit: v1.18.1
  • go version: go version go1.24.0 darwin/arm64
  • your complete go.mod:

go.mod:

module example

go 1.24.0

require (
	connectrpc.com/connect v1.18.1
	golang.org/x/net v0.34.0
)

require (
	buf.build/gen/go/bufbuild/bufplugin/protocolbuffers/go v1.36.3-20241031151143-70f632351282.1 // indirect
	buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.36.3-20241127180247-a33202765966.1 // indirect
	buf.build/gen/go/bufbuild/registry/connectrpc/go v1.18.1-20250106231242-56271afbd6ce.1 // indirect
	buf.build/gen/go/bufbuild/registry/protocolbuffers/go v1.36.3-20250106231242-56271afbd6ce.1 // indirect
	buf.build/gen/go/pluginrpc/pluginrpc/protocolbuffers/go v1.36.3-20241007202033-cf42259fcbfc.1 // indirect
	buf.build/go/bufplugin v0.6.0 // indirect
	buf.build/go/protoyaml v0.3.1 // indirect
	buf.build/go/spdx v0.2.0 // indirect
	cel.dev/expr v0.19.1 // indirect
	cloud.google.com/go/compute v1.23.3 // indirect
	cloud.google.com/go/compute/metadata v0.5.2 // indirect
	connectrpc.com/otelconnect v0.7.1 // indirect
	github.com/Azure/go-ansiterm v0.0.0-20250102033503-faa5f7b0171c // indirect
	github.com/Microsoft/go-winio v0.6.2 // indirect
	github.com/Microsoft/hcsshim v0.12.9 // indirect
	github.com/antlr4-go/antlr/v4 v4.13.1 // indirect
	github.com/bufbuild/buf v1.50.0 // indirect
	github.com/bufbuild/protocompile v0.14.1 // indirect
	github.com/bufbuild/protoplugin v0.0.0-20250106231243-3a819552c9d9 // indirect
	github.com/bufbuild/protovalidate-go v0.8.2 // indirect
	github.com/census-instrumentation/opencensus-proto v0.4.1 // indirect
	github.com/cespare/xxhash/v2 v2.3.0 // indirect
	github.com/cncf/udpa/go v0.0.0-20220112060539-c52dc94e7fbe // indirect
	github.com/cncf/xds/go v0.0.0-20240905190251-b4127c9b8d78 // indirect
	github.com/containerd/cgroups/v3 v3.0.5 // indirect
	github.com/containerd/containerd v1.7.25 // indirect
	github.com/containerd/continuity v0.4.5 // indirect
	github.com/containerd/errdefs v1.0.0 // indirect
	github.com/containerd/errdefs/pkg v0.3.0 // indirect
	github.com/containerd/log v0.1.0 // indirect
	github.com/containerd/platforms v0.2.1 // indirect
	github.com/containerd/stargz-snapshotter/estargz v0.16.3 // indirect
	github.com/containerd/ttrpc v1.2.7 // indirect
	github.com/containerd/typeurl/v2 v2.2.3 // indirect
	github.com/cpuguy83/go-md2man/v2 v2.0.6 // indirect
	github.com/distribution/reference v0.6.0 // indirect
	github.com/docker/cli v27.5.0+incompatible // indirect
	github.com/docker/distribution v2.8.3+incompatible // indirect
	github.com/docker/docker v27.5.0+incompatible // indirect
	github.com/docker/docker-credential-helpers v0.8.2 // indirect
	github.com/docker/go-connections v0.5.0 // indirect
	github.com/docker/go-units v0.5.0 // indirect
	github.com/envoyproxy/go-control-plane v0.13.1 // indirect
	github.com/envoyproxy/protoc-gen-validate v1.1.0 // indirect
	github.com/felixge/fgprof v0.9.5 // indirect
	github.com/felixge/httpsnoop v1.0.4 // indirect
	github.com/fullstorydev/grpcurl v1.9.2 // indirect
	github.com/go-chi/chi/v5 v5.2.0 // indirect
	github.com/go-logr/logr v1.4.2 // indirect
	github.com/go-logr/stdr v1.2.2 // indirect
	github.com/go-task/slim-sprig/v3 v3.0.0 // indirect
	github.com/gofrs/flock v0.12.1 // indirect
	github.com/gogo/protobuf v1.3.2 // indirect
	github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 // indirect
	github.com/golang/protobuf v1.5.4 // indirect
	github.com/google/cel-go v0.22.1 // indirect
	github.com/google/go-containerregistry v0.20.2 // indirect
	github.com/google/pprof v0.0.0-20241210010833-40e02aabc2ad // indirect
	github.com/google/uuid v1.6.0 // indirect
	github.com/inconshreveable/mousetrap v1.1.0 // indirect
	github.com/jdx/go-netrc v1.0.0 // indirect
	github.com/jhump/protoreflect v1.16.0 // indirect
	github.com/klauspost/compress v1.17.11 // indirect
	github.com/klauspost/pgzip v1.2.6 // indirect
	github.com/mattn/go-isatty v0.0.20 // indirect
	github.com/mitchellh/go-homedir v1.1.0 // indirect
	github.com/moby/docker-image-spec v1.3.1 // indirect
	github.com/moby/locker v1.0.1 // indirect
	github.com/moby/patternmatcher v0.6.0 // indirect
	github.com/moby/sys/mount v0.3.4 // indirect
	github.com/moby/sys/mountinfo v0.7.2 // indirect
	github.com/moby/sys/reexec v0.1.0 // indirect
	github.com/moby/sys/sequential v0.6.0 // indirect
	github.com/moby/sys/user v0.3.0 // indirect
	github.com/moby/sys/userns v0.1.0 // indirect
	github.com/moby/term v0.5.2 // indirect
	github.com/morikuni/aec v1.0.0 // indirect
	github.com/onsi/ginkgo/v2 v2.22.2 // indirect
	github.com/opencontainers/go-digest v1.0.0 // indirect
	github.com/opencontainers/image-spec v1.1.0 // indirect
	github.com/opencontainers/runtime-spec v1.2.0 // indirect
	github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c // indirect
	github.com/pkg/errors v0.9.1 // indirect
	github.com/pkg/profile v1.7.0 // indirect
	github.com/quic-go/qpack v0.5.1 // indirect
	github.com/quic-go/quic-go v0.48.2 // indirect
	github.com/rs/cors v1.11.1 // indirect
	github.com/russross/blackfriday/v2 v2.1.0 // indirect
	github.com/segmentio/asm v1.2.0 // indirect
	github.com/segmentio/encoding v0.4.1 // indirect
	github.com/sirupsen/logrus v1.9.3 // indirect
	github.com/spf13/cobra v1.8.1 // indirect
	github.com/spf13/pflag v1.0.5 // indirect
	github.com/stoewer/go-strcase v1.3.0 // indirect
	github.com/tetratelabs/wazero v1.8.2 // indirect
	github.com/vbatts/tar-split v0.11.6 // indirect
	go.lsp.dev/jsonrpc2 v0.10.0 // indirect
	go.lsp.dev/pkg v0.0.0-20210717090340-384b27a52fb2 // indirect
	go.lsp.dev/protocol v0.12.0 // indirect
	go.lsp.dev/uri v0.3.0 // indirect
	go.opencensus.io v0.24.0 // indirect
	go.opentelemetry.io/auto/sdk v1.1.0 // indirect
	go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.58.0 // indirect
	go.opentelemetry.io/otel v1.33.0 // indirect
	go.opentelemetry.io/otel/metric v1.33.0 // indirect
	go.opentelemetry.io/otel/trace v1.33.0 // indirect
	go.uber.org/mock v0.5.0 // indirect
	go.uber.org/multierr v1.11.0 // indirect
	go.uber.org/zap v1.27.0 // indirect
	go.uber.org/zap/exp v0.3.0 // indirect
	golang.org/x/crypto v0.32.0 // indirect
	golang.org/x/exp v0.0.0-20250106191152-7588d65b2ba8 // indirect
	golang.org/x/mod v0.22.0 // indirect
	golang.org/x/oauth2 v0.23.0 // indirect
	golang.org/x/sync v0.10.0 // indirect
	golang.org/x/sys v0.29.0 // indirect
	golang.org/x/term v0.28.0 // indirect
	golang.org/x/text v0.21.0 // indirect
	golang.org/x/tools v0.29.0 // indirect
	google.golang.org/appengine v1.6.8 // indirect
	google.golang.org/genproto v0.0.0-20240123012728-ef4313101c80 // indirect
	google.golang.org/genproto/googleapis/api v0.0.0-20250106144421-5f5ef82da422 // indirect
	google.golang.org/genproto/googleapis/rpc v0.0.0-20250106144421-5f5ef82da422 // indirect
	google.golang.org/grpc v1.69.4 // indirect
	google.golang.org/protobuf v1.36.5 // indirect
	gopkg.in/yaml.v3 v3.0.1 // indirect
	pluginrpc.com/pluginrpc v0.5.0 // indirect
)

tool (
	connectrpc.com/connect/cmd/protoc-gen-connect-go
	github.com/bufbuild/buf/cmd/buf
	github.com/fullstorydev/grpcurl/cmd/grpcurl
	google.golang.org/protobuf/cmd/protoc-gen-go
)

Additional context I have identified one occurrence of this problem in https://github.com/connectrpc/connect-go/blob/main/handler.go#L51-L57 If this were the only, I could send a PR. However, I have not investigated whether there are other occurrences in the codebase.

replu avatar Feb 13 '25 10:02 replu

@replu, that is not the proper idiom. Code should examine if the error is nil, and if it is not assume the other value is. So in that case, instead of checking res != nil and then doing something with it, you need the following:

 res, err := next(ctx, req)
-if res != nil {
+if err == nil { 
 	res.Header().Add("example", "example")
 }

This is fairly normal/idiomatic in Go APIs that return either a value or an error.

jhump avatar Feb 13 '25 13:02 jhump

@jhump

I’m already familiar with and agree that checking err == nil is the idiomatic way in Go. However, my concern is about interceptors that do not care about the error but only need to modify the response when it is not nil.

For example, if an interceptor wants to add a header (e.g., request_id) only when res is not nil, it becomes problematic because res != nil does not work as expected due to typed nil.

What do you think about this use case?

replu avatar Feb 13 '25 14:02 replu

If err is nil then res is guaranteed to be not-nil. Checking the nilness of err is the correct way to assert what you want, as @jhump suggested.

nicksnyder avatar Feb 13 '25 17:02 nicksnyder

Hi @nicksnyder

If err is nil then res is guaranteed to be not-nil.

This would certainly be true in this regard. However, this is only a matter of etiquette.

However, there does not appear to be any explicit written comments about it. https://github.com/connectrpc/connect-go/blob/v1.18.1/interceptor.go#L21-L27 https://github.com/connectrpc/connect-go/blob/v1.18.1/connect.go#L288-L304

We melted time to investigate this issue. A comment about the relationship between err and response would reduce the number of people struggling with similar problems.

For reference: io#CopyN

Thank you.

tomtwinkle avatar Feb 19 '25 01:02 tomtwinkle

Re-opening just to serve as a doc bug: we don't intend to change the current behavior, but we definitely want to be more clear in the docs and in Go doc comments about the correct usage pattern.

jhump avatar Feb 21 '25 16:02 jhump