grpc: fix message length checks when compression is enabled and maxReceiveMessageSize is MaxInt #7918
Conversation
arjan-bal
added
Area: RPC Features
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7918 +/- ##
==========================================
- Coverage 82.08% 82.03% -0.05%
==========================================
Files 379 384 +5
Lines 38261 38751 +490
==========================================
+ Hits 31406 31790 +384
- Misses 5551 5634 +83
- Partials 1304 1327 +23
|
rpc_util.go
Outdated
| if err != nil { | ||
| out.Free() | ||
| return nil, 0, err | ||
| } | ||
| if err = checkReceiveMessageOverflow(int64(out.Len()), int64(maxReceiveMessageSize), dcReader); err != nil { | ||
| return nil, out.Len() + 1, err |
There was a problem hiding this comment.
Adding 1 to out.Len() can result in an overflow on 32 bit systems. The following part of the function seems strange to me:
Optionally, if data will be over maxReceiveMessageSize, just return the size
I suggest making the following change to avoid this:
- Declare a global error for indicating that the max receive size is exceeded:
var errMaxMessageSizeExceeded = errors.New("max message size exceeded")- When the check here fails,
nil, 0, errMaxMessageSizeExceeded. Update the godoc to mention the same. - In the caller, i.e. recvAndDecompress, instead of checking
if size > maxReceiveMessageSize, checkif err == errMaxMessageSizeExceeded. Also update the error message to not mention the actual size, because we didn't read the entire message anyways:grpc: received message after decompression larger than max %d.
This ensures we're using the returned error to indicate the failure instead of relying on special values of the bytes read count.
There was a problem hiding this comment.
Please let the reviewer resolve comments.
There was a problem hiding this comment.
We should not attempt to add out.Len() + 1, instead return errMaxMessageSizeExceeded to signal the same.
There was a problem hiding this comment.
wait, in case of overflow i think we are still reading the partial data so we need to return the length upto which we have read?
There was a problem hiding this comment.
so, the logic should be read till maxMessageSize -1 first and then check if we still have bytes to read by reading one byte and if we overflow, throw the error along with lenght of how much we have read
arjan-bal
assigned vinothkumarr227 and arjan-bal and unassigned arjan-bal and vinothkumarr227
rpc_util.go
Outdated
| if err != nil { | ||
| out.Free() | ||
| return nil, 0, err | ||
| } | ||
| if err = checkReceiveMessageOverflow(int64(out.Len()), int64(maxReceiveMessageSize), dcReader); err != nil { | ||
| return nil, out.Len() + 1, err |
There was a problem hiding this comment.
Please let the reviewer resolve comments.
rpc_util.go
Outdated
| if err != nil { | ||
| out.Free() | ||
| return nil, 0, err | ||
| } | ||
| if err = checkReceiveMessageOverflow(int64(out.Len()), int64(maxReceiveMessageSize), dcReader); err != nil { | ||
| return nil, out.Len() + 1, err |
There was a problem hiding this comment.
We should not attempt to add out.Len() + 1, instead return errMaxMessageSizeExceeded to signal the same.
arjan-bal
assigned vinothkumarr227 and arjan-bal and unassigned arjan-bal and vinothkumarr227
vinothkumarr227
force-pushed
the
4552-max-receive-message-size-decompression
branch
from
9d80990 to
3cf9054
Compare
|
@purnesh42H I mistakenly clicked on 'Unassign me' from the issue. Can you please reassign the issue to me? |
Are you still working on resolving any remaining comments? If not, it should be assigned to Doug as 2nd reviewer to review. Regarding issue, you can comment here #4552 and we can assign to you. |
|
@purnesh42H I have completed addressing all the review comments. The changes are ready for review |
rpc_util.go
Outdated
| } | ||
| return out, nil | ||
| return d, nil |
There was a problem hiding this comment.
Isn't this an error scenario? If the payload is compressed (which is the only reason to call this function) and we don't have the decompressor, then we should error.
In reality, this can't happen because checkRecvPayload would prevent it, but this should still return an error, not a success.
rpc_util.go
Outdated
| } | ||
|
|
||
| // doesReceiveMessageOverflow checks if the number of bytes read from the stream | ||
| // exceed the maximum receive message size allowed by the receiver. If the `readBytes` |
rpc_util.go
Outdated
| return nil, status.Errorf(codes.Internal, "grpc: failed to read decompressed data: %v", err) | ||
| } | ||
|
|
||
| if doesReceiveMessageOverflow(out.Len(), maxReceiveMessageSize, dcReader) { |
There was a problem hiding this comment.
Nit:
How about something like this instead:
if out.Len() == maxReceiveMessageSize && !atEOF(dcReader) {// atEOF reads data from r and returns true iff zero bytes could be read and r.Read returns EOF.
func atEOF(r io.Reader) bool {
n, err := dcReader.Read(make([]byte, 1))
return n == 0 && err == io.EOF
}(Reasoning: "doesReceiveMessageOverflow" is very special-purpose, combines multiple operations into a single thing, and hides/splits relevant logic (reading & checking the length), whereas "atEOF" is potentially reusable, and does exactly one thing.)
|
@purnesh42H I have addressed all of Doug review comments. The changes are ready for a second review |
dfawley
changed the title
…ceiveMessageSize is MaxInt (grpc#7918)
…ceiveMessageSize is MaxInt (grpc#7918)
…ceiveMessageSize is MaxInt (grpc#7918)
…ceiveMessageSize is MaxInt (grpc#7918)
Fixes #4552
RELEASE NOTES: