rpc_util: Throws error if message is larger than MaxRecvMsgSize

[Aditya-Sood]

Also prevents int overflow in bufferSize

Fixes #4552

RELEASE NOTES:

• grpc: Fix bug when using grpc.MaxRecvMsgSize(math.MaxInt)

[codecov]

Codecov Report

Merging #6999 (fbecbd2) into master (5ccf176) will decrease coverage by 0.08%.
Report is 65 commits behind head on master.
The diff coverage is 37.03%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6999      +/-   ##
==========================================
- Coverage   82.43%   82.36%   -0.08%     
==========================================
  Files         296      300       +4     
  Lines       31473    31381      -92     
==========================================
- Hits        25946    25847      -99     
+ Misses       4468     4467       -1     
- Partials     1059     1067       +8     

Files	Coverage Δ
rpc_util.go	75.78% <37.03%> (-2.00%)	⬇️

... and 57 files with indirect coverage changes

[arvindbr8]

@Aditya-Sood -- could you please resolve the merge conflicts to the file due to this change 5ccf176?

Also, seems like we are missing some coverage. Do you mind adding in some units to test the scenario?

[dfawley]

We will also need fixes below here in case DecompressedSize is not implemented by the compressor.

As @arvindbr8 mentioned, we'll also want tests for overflowing the max message size with both kinds of compressors, too (some may already exist), and also covering the case where the max size is set to MaxInt. I guess we can't test overflowing with MaxInt very easily, though. Maybe 2GB (on a 32-bit system) isn't asking too much(?), but even that could be problematic.

I think a unit test of decompress would be ideal here, with an internal way to override MaxInt to a smaller value.

[Aditya-Sood]

hi @dfawley,
regarding the fixes for compressor not implementing DecompressedSize, could you please elaborate on what the intended behaviour should be?
I had the following changes in mind:

if sizer, ok := compressor.(interface {
	DecompressedSize(compressedBytes []byte) int
}); ok {
	...
} else {
	log.Println("warn: compressor does not implement method DecompressedSize()")
}

d, err = io.ReadAll(io.LimitReader(dcReader, int64(maxReceiveMessageSize)))

if len(d) == maxReceiveMessageSize {
	b := make([]byte, 1)
	if n, err := dcReader.Read(b); n > 0 || err != io.EOF {
		return nil, len(d) + n, fmt.Errorf("overflow: message larger than max size receivable by client (%v bytes)", maxReceiveMessageSize)
	}
}
return d, len(d), err

[dfawley]

Sure, something like that LGTM. It would be great if you can refactor the code to remove the duplication, though, since both code paths are very similar.

[Aditya-Sood]

hi @arvindbr8, have resolved the merge conflict in the new commit
will work on adding the unit tests now

[github-actions]

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

[Aditya-Sood]

have implemented the change for #6999 (comment)
going through the test files to figure out where the unit tests are to be added

[github-actions]

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

[Aditya-Sood]

hi @arvindbr8, @dfawley
apologies for the delay - I was travelling earlier and then fell sick last week, so wasn't able to update before

I have been trying to trace the back the input tests which end up at rpc_util.go decompress(), but the paths diverge to both streams.go & server.go, with neither making it very apparent how the lead back to rpc_util_test.go eventually

do you have any advice on how I can figure this out?
(if I know one test that ends up at rpc_util.go decompress(), I can replicate it to create the test with payload of math.MaxInt size)

[dfawley]

Why not just write a new TestDecompress() that calls decompress with the inputs you're testing?

Do not create messages of size MaxInt, though. Instead define a var maxAllowedMessageSize = math.MaxInt and override it in the unit test.

[github-actions]

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

[Aditya-Sood]

got it @dfawley, I'll do it this weekend
sorry for the delay, I also started a new job this month so have been preoccupied coming up to speed there
thanks

[github-actions]

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.