You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by asuhan <gi...@git.apache.org> on 2017/08/04 02:29:57 UTC
[GitHub] thrift pull request #1326: THRIFT-3821
GitHub user asuhan opened a pull request:
https://github.com/apache/thrift/pull/1326
THRIFT-3821
Going over 2 GB is quite possible when using Thrift for serialization. We can detect the condition and throw an exception.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/asuhan/thrift THRIFT-3821
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1326.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1326
----
commit 314a0b30c59b0e8b314dc6c82061eecabeb10e14
Author: Alex Şuhan <al...@gmail.com>
Date: 2017-08-03T18:45:31Z
THRIFT-3821 Test for overflow on buffer resize in TMemoryBuffer
commit 0fe1997d53020980abdb6f43843206a289a8e24d
Author: Alex Şuhan <al...@gmail.com>
Date: 2017-08-03T19:28:17Z
THRIFT-3821 Check for overflow on buffer resize in TMemoryBuffer
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1326: THRIFT-3821
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1326
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1326: THRIFT-3821
Posted by asuhan <gi...@git.apache.org>.
Github user asuhan commented on the issue:
https://github.com/apache/thrift/pull/1326
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").
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1326: THRIFT-3821
Posted by asuhan <gi...@git.apache.org>.
Github user asuhan commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1326#discussion_r132612556
--- Diff: lib/cpp/test/TMemoryBufferTest.cpp ---
@@ -117,4 +117,17 @@ BOOST_AUTO_TEST_CASE(test_exceptions) {
BOOST_CHECK_NO_THROW(buf2.write((const uint8_t*)"bar", 3));
}
+#ifndef _WIN32
+// We can't allocate 1 GB of memory in 32-bit environments.
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1326: THRIFT-3821
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1326
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1326: THRIFT-3821
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1326#discussion_r132611000
--- Diff: lib/cpp/src/thrift/transport/TBufferTransports.cpp ---
@@ -361,9 +361,13 @@ void TMemoryBuffer::ensureCanWrite(uint32_t len) {
}
// Grow the buffer as necessary.
- uint32_t new_size = bufferSize_;
+ 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()) {
--- End diff --
Shouldn't this be int32_t instead of uint32_t, if we want to test against 2GB? Sorry I didn't catch this before.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1326: THRIFT-3821
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1326#discussion_r132545680
--- Diff: lib/cpp/src/thrift/transport/TBufferTransports.cpp ---
@@ -361,9 +361,12 @@ void TMemoryBuffer::ensureCanWrite(uint32_t len) {
}
// Grow the buffer as necessary.
- uint32_t new_size = bufferSize_;
+ 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()) {
+ throw TTransportException("Internal buffer size exceeded 2GB");
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1326: THRIFT-3821
Posted by asuhan <gi...@git.apache.org>.
Github user asuhan commented on the issue:
https://github.com/apache/thrift/pull/1326
Are there any additional changes I should make to this pull request? As I've said in the inline comment, I believe comparing to `std::numeric_limits<uint32_t>::max()` is actually consistent with what the error message says.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1326: THRIFT-3821
Posted by asuhan <gi...@git.apache.org>.
Github user asuhan commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1326#discussion_r132612265
--- Diff: lib/cpp/src/thrift/transport/TBufferTransports.cpp ---
@@ -361,9 +361,13 @@ void TMemoryBuffer::ensureCanWrite(uint32_t len) {
}
// Grow the buffer as necessary.
- uint32_t new_size = bufferSize_;
+ 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()) {
--- End diff --
We check it post-resize and it fails if we go past 4 GB, 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1326: THRIFT-3821
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1326#discussion_r132611133
--- Diff: lib/cpp/test/TMemoryBufferTest.cpp ---
@@ -117,4 +117,17 @@ BOOST_AUTO_TEST_CASE(test_exceptions) {
BOOST_CHECK_NO_THROW(buf2.write((const uint8_t*)"bar", 3));
}
+#ifndef _WIN32
+// We can't allocate 1 GB of memory in 32-bit environments.
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1326: THRIFT-3821
Posted by asuhan <gi...@git.apache.org>.
Github user asuhan commented on the issue:
https://github.com/apache/thrift/pull/1326
I've disabled the test for Windows since allocating 1GB on 32-bit is likely to fail with `bad_alloc`.
Test failure looks unrelated, also got a clean run before: https://travis-ci.org/apache/thrift/jobs/263298365.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1326: THRIFT-3821
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1326
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1326: THRIFT-3821
Posted by asuhan <gi...@git.apache.org>.
Github user asuhan commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1326#discussion_r132613779
--- Diff: lib/cpp/src/thrift/transport/TBufferTransports.cpp ---
@@ -361,9 +361,13 @@ void TMemoryBuffer::ensureCanWrite(uint32_t len) {
}
// Grow the buffer as necessary.
- uint32_t new_size = bufferSize_;
+ 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()) {
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---