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(codec): Properly decode partial DATA frames #83

Merged
merged 28 commits into from
Oct 22, 2019
Merged

fix(codec): Properly decode partial DATA frames #83

merged 28 commits into from
Oct 22, 2019

Conversation

cpcloud
Copy link
Contributor

@cpcloud cpcloud commented Oct 21, 2019

Closes #76

TODO:

  • Add mock to reproduce in a unit test

Motivation

Reproduced from #76:

When receiving data by polling the body, it's possible to end up in a
situation where the polling results in a buffer of size N where N >= length, but also that N < 5 + length.

Roughly the flow is like this:

  1. Tonic polls for data, gets N bytes, where length <= N < 5 + length
  2. Tonic sees that N bytes have been read, and if N >= length, _advances the buffer by 5 bytes
  3. Advancing the buffer reduces the buffer length by 5, now we have that length - 5 <= N < length
  4. Since we need length bytes for the proto, prost fails with a buffer underflow

Solution

The solution that I've implemented here is to check that self.buf--the buffer
that holds the bytes for decoding--has enough data to decode both the tonic
header and the message. If the check returns false, wait for more data,
otherwise decode the bytes

tonic-examples/Cargo.toml Outdated Show resolved Hide resolved
@@ -0,0 +1,29 @@
// Copyright 2015 gRPC authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");

Choose a reason for hiding this comment

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

This is also not accurate, I think. It's Apache OR MIT dual-licensed.

Choose a reason for hiding this comment

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

As an addendum, the right thing to do if license headers are desired is to use SPDX format ones, otherwise the LICENSE in the repo works just fine.

tonic/src/codec/mod.rs Outdated Show resolved Hide resolved
@LucioFranco
Copy link
Member

@cpcloud so if I understand correct the issue is that we may not have the initial 5 bytes from the buffer handed to us from hyper? Thus the buf.advance(5) fails?

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 21, 2019

@LucioFranco No, the issue is that we'll receive the header + a partial body. For example:

> read 10005 bytes from the body (tonic header + 3 bytes from protos + 9997 bytes of the byte array)
  > advance 5
  > 10000 left over, but the message length is actually 10003
  > BEFORE: try to decode
  > AFTER: check that we've read enough to contain the entire message and if not, poll the body again

We need to ensure that self.buf has enough of the body to decode the full message including the header.

@LucioFranco
Copy link
Member

Ok so after our discussion on discord I am happy with this solution. I hate to ask this but would it be possible to try and make this change with the smallest footprint possible which I think is just the second if statement in readbody. And instead of introducing a new example service can we add a test, there should already be tests for the codec. In the mock body simulate that the frames are split.

@LucioFranco
Copy link
Member

and thank you so much for figuring this out! It is super helpful, I'd like to get this change in before the next release this week. Let me know if you need anything.

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 21, 2019

@LucioFranco Looks like the decode test never actually reads anything! 😂 Each poll_data() call is checking whether 0 > 100.

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 21, 2019

I'll add a fix in this PR.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you so much for digging into this!

@LucioFranco LucioFranco changed the title bug(decoding): Fix buffer size for partial message reads fix(codec): Properly decode partial DATA frames Oct 21, 2019
@LucioFranco LucioFranco merged commit 9079e0f into hyperium:master Oct 22, 2019
@cpcloud cpcloud deleted the fix-partial-message-buf-size branch October 22, 2019 00:06
@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 22, 2019

Yeah! Thanks for reviewing + merging!

blittable pushed a commit to blittable/tonic that referenced this pull request Oct 22, 2019
rabbitinspace pushed a commit to satelit-project/tonic that referenced this pull request Jan 1, 2020
brentalanmiller pushed a commit to brentalanmiller/tonic that referenced this pull request Oct 6, 2023
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.

Decoding occasionally fails when streaming
3 participants