http2_client: fix reader segfault on PROTOCOL_ERRORs

[sorah]

http2.Framer ErrorDetail() might return nil even in case of http2.StreamError, as documented.

---

(Note: This problem was initially reported to grpc-security@ just in case, but they assessed this segfault isn't a security issue.)

What did you do?

Wrote a gRPC server may send an invalid HEADERS frame to a gRPC client process written with grpc-go. The frame is set length to 0. https://img.sorah.jp/x/20200917_185642_kdwgHWb8KG.png

(a server is written using our original protocol implementation, which is I'm maintaining and sending an invalid HEADERS frame is a different bug. Found during investigation of server process crash)

What did you expect to see?

HTTP/2 session would properly shut down with PROTOCOL_ERROR.

What did you see instead?

a client process would crash with SEGV (segmentation fault).

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x144f08f]
goroutine 772 [running]:
google.golang.org/grpc/internal/transport.(*http2Client).reader(0xc0012f4380)
/.../pkg/mod/google.golang.org/gr...@v1.31.0/internal/transport/http2_client.go:1309 +0x2af
created by google.golang.org/grpc/internal/transport.newHTTP2Client
/.../pkg/mod/google.golang.org/gr...@v1.31.0/internal/transport/http2_client.go:310 +0x1071

This happens because http2.Framer#ErrorDetail() returned nil (it may return, as documented):

grpc-go/internal/transport/http2_client.go

Line 1309 in 4270c3c

msg := t.framer.fr.ErrorDetail().Error()

It might return nil even in case of StreamError.

For instance, when a server sent an invalid HEADERS frame where a length is zero, http2.Framer#ReadFrame returns an StreamError
without setting http2.Framer#lastError (which is a struct field back of ErrorDetail).
https://github.com/golang/net/blob/62affa334b73ec65ed44a326519ac12c421905e3/http2/frame.go#L1022

Due to this crash existing in http2Client Reader goroutine, this is unavoidable and a process would result in a segmentation fault.

This bug seems to be introduced at commit 40a879c which changed to call Error() of a return value of ErrorDetail().

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Sorah Fukumori (275719c)

[dfawley]

Thank you for the patch! One minor comment.

[dfawley]

Please add some text to msg when errorDetail is nil.

	} else {
		msg = "unknown framer error"  // ??
	}

[sorah]

I was unsure what comment is appropriate when errorDetail is unavailable...

[sorah]

c8eec21

[dfawley]

That sounds fine to me.

[dfawley]

That sounds fine to me.

[dfawley]

Thanks again!