Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stream creation concurrency bug #136

Open
davisp opened this issue Sep 11, 2019 · 2 comments
Open

Stream creation concurrency bug #136

davisp opened this issue Sep 11, 2019 · 2 comments

Comments

@davisp
Copy link

davisp commented Sep 11, 2019

For starters, I'm not 100% certain on where to file this bug at. I've found a bug via grpcbox's use of streams, however, I don't see a way that it can be fixed externally from the h2_connection pid.

The code in grpcbox:

https://github.com/tsloughter/grpcbox/blob/545ccdfe3f54469d53f373384e81f00b0b51807c/src/grpcbox_client_stream.erl#L75-L86

The issue here is that we're allocating and returning a stream id from inside the h2_connection but we don't send headers from inside the h2_connection pid. The relevant section of RFC7541 is here:

https://httpwg.org/specs/rfc7540.html#rfc.section.5.1.1

The identifier of a newly established stream MUST be numerically greater than all streams that the initiating endpoint has opened or reserved. This governs streams that are opened using a HEADERS frame and streams that are reserved using PUSH_PROMISE. An endpoint that receives an unexpected stream identifier MUST respond with a connection error (Section 5.4.1) of type PROTOCOL_ERROR.

Given that we allocate the ids before headers are sent means that concurrent use of the h2_connection can easily crash the entire connection if the headers are sent in a different order than the stream ids are allocated which is fairly easy to induce by something like such:

[spawn(fun() -> use_grpcbox_client() end || N <- lists:seq(1, 1000)]

Theoretically grpcbox could do some sort of insistence on serializing stream creation but it really seems like this should be enforced inside h2_connection. The most obvious thing I can see would be to change h2_connection:new_stream to take the headers as an argument so that they're sent as ids are allocated.

@tsloughter
Copy link
Collaborator

Finally working on this and have a test in grpcbox that reproduces it.

@tsloughter
Copy link
Collaborator

I think your idea of passing the headers to new_stream is probably the simplest and most correct to ensure the headers are sent for an allocated stream before another stream is allocated that could be sending its headers as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants