THRIFT-3821 #1326
THRIFT-3821 #1326
Conversation
| while (len > avail) { | ||
| new_size = new_size > 0 ? new_size * 2 : 1; | ||
| if (new_size > std::numeric_limits<uint32_t>::max()) { | ||
| throw TTransportException("Internal buffer size exceeded 2GB"); |
There was a problem hiding this comment.
I would prefer to see a TTransportException with an error type BAD_ARGS here, however it looks like error types are not being used elsewhere in this module.
|
Please rebase (and squash to a single commit, if you can) and push here, so the build system can build the change and we can verify it before merging it. Thanks. |
|
Thanks for the review, I'll address both. It's my preference to squash the commits as well, sounds like I've misinterpreted https://thrift.apache.org/docs/HowToContribute (it says "When bugfixing: add test that will isolate bug before applying change that fixes it"). |
|
I've disabled the test for Windows since allocating 1GB on 32-bit is likely to fail with Test failure looks unrelated, also got a clean run before: https://travis-ci.org/apache/thrift/jobs/263298365. |
jeking3
left a comment
jeking3
left a comment
There was a problem hiding this comment.
Ignoring build job 19 failure, it's the ssl.d intermittent issue.
jeking3
left a comment
jeking3
left a comment
There was a problem hiding this comment.
Sorry I missed one thing in my previous review.
| uint64_t new_size = bufferSize_; | ||
| while (len > avail) { | ||
| new_size = new_size > 0 ? new_size * 2 : 1; | ||
| if (new_size > std::numeric_limits<uint32_t>::max()) { |
There was a problem hiding this comment.
Shouldn't this be int32_t instead of uint32_t, if we want to test against 2GB? Sorry I didn't catch this before.
There was a problem hiding this comment.
We check it post-resize and it fails if we go past 4GB, therefore pre-resize it must be at most 2GB. Checking against int32_t would be overly conservative; we mainly care about avoiding the arithmetic overflow. Maybe the error message could be improved? "Internal buffer size was already past 2GB when we attempted to resize" would be a more precise description, but I didn't want to make it overly verbose / obscure.
There was a problem hiding this comment.
Basically, if we only go through powers of 2 when resizing (which looks to be the case), the buffer can only grow to 2GB since the maximum value of uint32_t is one short of 4GB.
| } | ||
|
|
||
| #ifndef _WIN32 | ||
| // We can't allocate 1 GB of memory in 32-bit environments. |
There was a problem hiding this comment.
This makes me wonder if the TBufferTransport should have a size limit that is configurable, with a default of INT32_MAX, and then the test can make a smaller one like 4KB, and write 4KB and then one byte more, instead of using up 2GB of memory.
There was a problem hiding this comment.
We'd have to write in a loop to trigger buffer resizing if I understand correctly. It'd save the vector allocation in the test, but TMemoryBuffer would still grow. I agree a 50% reduction would be better than nothing, for sure.
|
Are there any additional changes I should make to this pull request? As I've said in the inline comment, I believe comparing to |
|
I'd prefer if we refactored it to make the limit configurable with a default matching what it is now; that way tests won't need multiple gigabytes of memory to function properly. I'll approve and merge this for now as it is an improvement. |
Client: C++ This closes apache#1326
2 participants
Going over 2 GB is quite possible when using Thrift for serialization. We can detect the condition and throw an exception.