fishy commented on code in PR #3057:
URL: https://github.com/apache/thrift/pull/3057#discussion_r1819483243
##########
lib/go/thrift/binary_protocol.go:
##########
@@ -555,7 +555,28 @@ func safeReadBytes(size int32, trans io.Reader) ([]byte,
error) {
return nil, nil
}
- buf := new(bytes.Buffer)
- _, err := io.CopyN(buf, trans, int64(size))
- return buf.Bytes(), err
+ const readLimit = 10 * 1024 * 1024
Review Comment:
so there are 3 different cases:
1. ordinary case (reasonable size, not malformed)
2. legit very large payload (large size, not malformed)
3. malformed payload with large size
and the choice here is between 2 and 3 (we already agreed to optimize for 1).
between 2 and 3 I would prefer to optimize for 3, based on the consequences
of the opposite. if we optimize for 2 (your current approach), then whenever
the code get a malformed payload they would need to allocate 10MiB up front,
that can be a big risk for code running with tight resources so the consequence
can be more severe (e.g. they have more risk to crash due to insufficient
memory). with the `CopyN` approach the consequence is just more allocations for
legit cases, which is slightly slower and use more memory, but if the code is
intended to handle legit very large payloads then we can already assume that it
has more resource and those "wasted" memory will have a smaller consequences.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
