[GitHub] thrift pull request #1326: THRIFT-3821
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1326: THRIFT-3821
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::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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1326: THRIFT-3821
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1326: THRIFT-3821
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::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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1326: THRIFT-3821
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::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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1326: THRIFT-3821
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1326: THRIFT-3821
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::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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[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 Date: 2017-08-03T18:45:31Z THRIFT-3821 Test for overflow on buffer resize in TMemoryBuffer commit 0fe1997d53020980abdb6f43843206a289a8e24d Author: Alex Åuhan 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
