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

fix(web): fix empty trailer parsing causing infinite parser loop #1883

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

zakhenry
Copy link
Contributor

Motivation

Attempting to parse a grpc response from an envoy proxy that inserts trailers causes an infinite loop

Solution

Digging in to the code I found that there was an attempt to read more data after parsing trailers but the buffer was empty, ending up in the same place again. Guarding this fixes the issue and I can successfully fetch a message.

Note this fix needs to be combined with #1880 to get a valid grpc response back (otherwise it panics earlier).

@zakhenry
Copy link
Contributor Author

I want to add a test for this fix, however I got stuck trying to actually create trailers for a unary request with tonic? It seems to me that trailers are not actually supported by the server, and so therefore it becomes pretty difficult to test this functionality. I tried also with a streaming call to see if I could work around it that way, but no joy there either.

@@ -305,7 +305,11 @@ where
}
FindTrailers::IncompleteBuf => continue,
FindTrailers::Done(len) => {
Poll::Ready(Some(Ok(Frame::data(buf.split_to(len).freeze()))))
if len > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: please spell it like this:

Poll::Ready(match len {
    0 => None,
    _ => Some(Ok(Frame::data(buf.split_to(len).freeze()))),
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cargo fmt ended up inlining this expression with FindTrailers::Done(len) =>, which is neat enough I think

@zakhenry zakhenry force-pushed the fix/trailer-parsing-infinite-loop branch 2 times, most recently from 5aafae7 to 315c9c2 Compare August 22, 2024 16:20
@zakhenry zakhenry force-pushed the fix/trailer-parsing-infinite-loop branch from 315c9c2 to 26a939b Compare August 22, 2024 16:21
@zakhenry
Copy link
Contributor Author

Not sure what these CI failures are - the code does work as I'm using my fork as a git dep

@tottoto
Copy link
Collaborator

tottoto commented Aug 23, 2024

I rerun the CI and it seems successful.

@djc djc added this pull request to the merge queue Aug 23, 2024
Merged via the queue into hyperium:master with commit f321d6a Aug 23, 2024
16 checks passed
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

Successfully merging this pull request may close these issues.

4 participants