http2_client: fix reader segfault on PROTOCOL_ERRORs #3926
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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).
This happens because http2.Framer#ErrorDetail() returned nil (it may return, as documented):
grpc-go/internal/transport/http2_client.go
Line 1309 in 4270c3c
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().