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.

Sign up for GitHub

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

Jump to bottom

rpc_util: Throws error if message is larger than MaxRecvMsgSize #6999

Closed
wants to merge 3 commits into from
Closed

rpc_util: Throws error if message is larger than MaxRecvMsgSize #6999

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e691e16
Increment: Throws error if message is larger than MaxRecvMsgSize
Aditya-Sood Feb 23, 2024
83a8c6a
Merge branch 'master' into 4552-empty-response
Aditya-Sood Feb 27, 2024
fbecbd2
Refactor: Adds function for message overflow check
Aditya-Sood Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 14 additions & 2 deletions rpc_util.go
View file
Open in desktop
Expand Up @@ -782,8 +782,20 @@
// size is used as an estimate to size the buffer, but we
// will read more data if available.
// +MinRead so ReadFrom will not reallocate if size is correct.
buf := bytes.NewBuffer(make([]byte, 0, size+bytes.MinRead))
bytesRead, err := buf.ReadFrom(io.LimitReader(dcReader, int64(maxReceiveMessageSize)+1))
bufferSize := uint64(size) + bytes.MinRead
if bufferSize > math.MaxInt {
bufferSize = math.MaxInt
}

Check warning on line 788 in rpc_util.go

View check run for this annotation

Codecov / codecov/patch

rpc_util.go#L787-L788 

Added lines #L787 - L788 were not covered by tests
buf := bytes.NewBuffer(make([]byte, 0, int(bufferSize)))

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

Check warning on line 795 in rpc_util.go

View check run for this annotation

Codecov / codecov/patch

rpc_util.go#L794-L795 

Added lines #L794 - L795 were not covered by tests
}
}

return buf.Bytes(), int(bytesRead), err
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Expand Down