panic: csAttempt.s nil pointer in CloseSend

[ashinrot]

What version of gRPC are you using?

1.42.0

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

1.15

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

Linux

What did you do?

Call bi-directional stream client's recv and send in different goroutine, and panic during CloseSend() in send goroutine.

The panic stack is

panic(0x135ed00, 0x20eb530)
        /usr/local/go/src/runtime/panic.go:969 +0x1b9
google.golang.org/grpc/internal/transport.(*Stream).compareAndSwapState(...)
        /google.golang.org/grpc@v1.42.0/internal/transport/transport.go:310
google.golang.org/grpc/internal/transport.(*http2Client).Write(0xc341ace1e0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc79d5b9fbc, 0x2, ...)
        /google.golang.org/grpc@v1.42.0/internal/transport/http2_client.go:954 +0x3e
google.golang.org/grpc.(*clientStream).CloseSend.func1(0xc32e4e4b00, 0x0, 0xc2c33e4780)
        /google.golang.org/grpc@v1.42.0/stream.go:864 +0x79

I think I see the problem is in withRetry, definition is

func (cs *clientStream) withRetry(op func(a *csAttempt) error, onSuccess func()) error {
	cs.mu.Lock()
	for {
		if cs.committed {
			cs.mu.Unlock()
			// toRPCErr is used in case the error from the attempt comes from
			// NewClientStream, which intentionally doesn't return a status
			// error to allow for further inspection; all other errors should
			// already be status errors.
			return toRPCErr(op(cs.attempt))
		}
		a := cs.attempt
...

Bug is in return toRPCErr(op(cs.attempt)) which is not locked, and other recv can modify cs.attempt.

[dfawley]

I don't think your assessment of the bug is correct.

cs.attempt is only assigned from newAttemptLocked, which is only called either at the very start of the RPC (while the stream is being created) or during retryLocked which is not called if cs.committed is true. So if withRetry sees cs.committed == true, then it can safely read cs.attempt outside of the lock.

Do you have a reproducible test case for your situation? I'm sure there is something wrong here, but I'm having a hard time designing a test that expose it. Thanks.

[github-actions]

This issue is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.