panic: runtime error: invalid memory address or nil pointer dereference at [email protected]/stream.go:380

[ukai]

NOTE: if you are reporting is a potential security vulnerability or a crash,
please follow our CVE process at
https://github.com/grpc/proposal/blob/master/P4-grpc-cve-process.md instead of
filing an issue here.

Please see the FAQ in our main README.md, then answer the questions below
before submitting your issue.

What version of gRPC are you using?

1.48.0

What version of Go are you using (go version)?

1.18.4

What operating system (Linux, Windows, …) and version?

linux (ubuntu 18.04)

What did you do?

If possible, provide a recipe for reproducing the error.

https://chromium.googlesource.com/infra/goma/server/+/refs/heads/main/profiler/profiler.go#34

               err = profiler.Start(profiler.Config{Service: target},
                        // Disallow grpc in google-api-go-client to send stats/trace of profiler grpc's api call.
                        option.WithGRPCDialOption(grpc.WithStatsHandler(nil)))

What did you expect to see?

no runtime panic (as it was in 1.47.0 or prior version)

What did you see instead?

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x8e9e84]

goroutine 40 [running]:
google.golang.org/grpc.(*clientStream).newAttemptLocked(0xc0000ca5a0, 0x0)
	/workspace/pkg/mod/google.golang.org/[email protected]/stream.go:380 +0x744
google.golang.org/grpc.newClientStreamWithParams({0x11c51d0, 0xc00062f650}, 0x1833f40, 0xc000359c00, {0xf2fe1a, 0x3f}, {0x0, 0x0, 0x0, 0x0, ...}, ...)
	/workspace/pkg/mod/google.golang.org/[email protected]/stream.go:306 +0x90c
google.golang.org/grpc.newClientStream.func2({0x11c51d0?, 0xc00062f650?}, 0xc00062f650?)
	/workspace/pkg/mod/google.golang.org/[email protected]/stream.go:192 +0x9f
google.golang.org/grpc.newClientStream({0x11c51d0, 0xc00062f650}, 0x1833f40, 0xc000359c00, {0xf2fe1a, 0x3f}, {0xc000046870, 0x1, 0x1})
	/workspace/pkg/mod/google.golang.org/[email protected]/stream.go:220 +0x4c2
google.golang.org/grpc.invoke({0x11c51d0?, 0xc00062f650?}, {0xf2fe1a?, 0x70?}, {0xe668e0, 0xc00044e300}, {0xe99000, 0xc0006312d0}, 0x0?, {0xc000046870, ...})
	/workspace/pkg/mod/google.golang.org/[email protected]/call.go:66 +0x7d
google.golang.org/grpc.(*ClientConn).Invoke(0x7f5fb9688108?, {0x11c51d0?, 0xc00062f650?}, {0xf2fe1a?, 0x0?}, {0xe668e0?, 0xc00044e300?}, {0xe99000?, 0xc0006312d0?}, {0xc000046870, ...})
	/workspace/pkg/mod/google.golang.org/[email protected]/call.go:37 +0x265
google.golang.org/genproto/googleapis/devtools/cloudprofiler/v2.(*profilerServiceClient).CreateProfile(0xc000110c30, {0x11c51d0, 0xc00062f650}, 0x7f5fb9688108?, {0xc000046870, 0x1, 0x1})
	/workspace/pkg/mod/google.golang.org/[email protected]/googleapis/devtools/cloudprofiler/v2/profiler.pb.go:847 +0xce
cloud.google.com/go/profiler.(*agent).createProfile.func1({0x11c51d0, 0xc00062f650}, {0xc000046860, {0x0, 0x0, 0x0}, {0x0, 0x0}})
	/workspace/pkg/mod/cloud.google.com/go/[email protected]/profiler.go:337 +0x114
github.com/googleapis/gax-go/v2.invoke({0x11c51d0, 0xc00062f650}, 0xc00006fec8, {0xc000046860, {0x0, 0x0, 0x0}, {0x0, 0x0}}, 0xf55210)
	/workspace/pkg/mod/github.com/googleapis/gax-go/[email protected]/invoke.go:72 +0xce
github.com/googleapis/gax-go/v2.Invoke({0x11c51d0, 0xc00062f650}, 0x0?, {0xc000059eb8, 0x1, 0x0?})
	/workspace/pkg/mod/github.com/googleapis/gax-go/[email protected]/invoke.go:50 +0xa5
cloud.google.com/go/profiler.(*agent).createProfile(0xc000652400, {0x11c51d0, 0xc00062f650})
	/workspace/pkg/mod/cloud.google.com/go/[email protected]/profiler.go:334 +0x23c
cloud.google.com/go/profiler.pollProfilerService({0x11c51d0, 0xc00062f650}, 0x1c?)
	/workspace/pkg/mod/cloud.google.com/go/[email protected]/profiler.go:617 +0xd2
created by cloud.google.com/go/profiler.start
	/workspace/pkg/mod/cloud.google.com/go/[email protected]/profiler.go:262 +0x6b3

[dfawley]

+@codyoss -- tl;dr: a client library is installing a gRPC stats handler and the user wishes to disable it, and is doing so by passing their own gRPC dial option that sets a nil stats handler. We changed this API slightly in the last release to allow multiple stats handlers to be enabled at the same time: the DialOption for stats handlers now accumulates StatsHandlers instead of overwriting them.

This was an unexpected use case. The expectation was that stats handlers were only installed once, and never cleared or overwritten using something like this.

Some options:

1. Roll these changes back and make new stats handlers overwrite the previous stats handler, as before.

   • Safest; needs a new design to enable multiple stats handlers.

2. Accept nil as a special case to reset the list.

   • Handles this case fine; still breaks any users attempting to overwrite a stats handler with another one. An untyped nil is never a valid StatsHandler so this special case behavior is quite reasonable.

3. Do nothing in gRPC and make this profiler expose an option to avoid registering the stats handler.

   • This is potentially a better design for the library than requiring the user to set a gRPC option which the library can't detect

Also note that stats handlers are and have always been explicitly marked as "experimental", which means client libraries should not be using them except in experimental packages. If we delete them now, we will break all your users.

[codyoss]

Do nothing in gRPC and make this profiler expose an option to avoid registering the stats handler.

This already exists today: option.WithTelemetryDisabled

Our telemetry story needs to be cleaned up in client libraries a bit. We are transitively pulling in the use of the experimental API I guess by using go.opencensus.io/plugin/ocgrpc. This is before my time on the team so I don't have all of the context here, but the user should have a way forward for now at least.

[dfawley]

This already exists today: option.WithTelemetryDisabled

Excellent, thank you @codyoss. Can you give this a try, please, @ukai?

Argh, good point; we should theoretically need to consider all of OpenCensus as experimental. Realistically our hands are completely tied here and we just need to mark the stats package as stable, even though there are some things we'd like to change about it. #4690

[ukai]

thanks!
seems ok with option.WithTelemetryDisabled