[jira] [Commented] (THRIFT-3821) TMemoryBuffer buffer may overflow when resizing
[ https://issues.apache.org/jira/browse/THRIFT-3821?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122799#comment-16122799 ] ASF GitHub Bot commented on 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. > TMemoryBuffer buffer may overflow when resizing > --- > > Key: THRIFT-3821 > URL: https://issues.apache.org/jira/browse/THRIFT-3821 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.9.3 >Reporter: Huaisi Xu > > In ensurecanwrite(): > {code} > uint32_t new_size = bufferSize_; > while (len > avail) { > new_size = new_size > 0 ? new_size * 2 : 1; > avail = available_write() + (new_size - bufferSize_); > } > // Allocate into a new pointer so we don't bork ours if it fails. > uint8_t* new_buffer = static_cast(std::realloc(buffer_, > new_size)); > if (new_buffer == NULL) { > throw std::bad_alloc(); > } > rBase_ = new_buffer + (rBase_ - buffer_); > rBound_ = new_buffer + (rBound_ - buffer_); > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > buffer_ = new_buffer; > bufferSize_ = new_size; > {code} > If old bufferSize_ is lager than 2gb, then calculating new size will overflow. > i.e. if bufferSize_ = 3355443200, then new buffer size will be 2415919104, > which is less than old size. > However, > {code} > avail = available_write() + (new_size - bufferSize_) > {code} > overflows again, so we will end up with an shrinked buffer. > What is worse is that after > {code} > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > {code} > wBase_ stays the same, but wBound_ becomes lower than it. What happens next > is that uint32_t avail = available_write() may overflow every time > subsequently. and thrift writes to unknown memory. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[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. ---
[jira] [Commented] (THRIFT-3821) TMemoryBuffer buffer may overflow when resizing
[ https://issues.apache.org/jira/browse/THRIFT-3821?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122776#comment-16122776 ] ASF GitHub Bot commented on 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. > TMemoryBuffer buffer may overflow when resizing > --- > > Key: THRIFT-3821 > URL: https://issues.apache.org/jira/browse/THRIFT-3821 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.9.3 >Reporter: Huaisi Xu > > In ensurecanwrite(): > {code} > uint32_t new_size = bufferSize_; > while (len > avail) { > new_size = new_size > 0 ? new_size * 2 : 1; > avail = available_write() + (new_size - bufferSize_); > } > // Allocate into a new pointer so we don't bork ours if it fails. > uint8_t* new_buffer = static_cast(std::realloc(buffer_, > new_size)); > if (new_buffer == NULL) { > throw std::bad_alloc(); > } > rBase_ = new_buffer + (rBase_ - buffer_); > rBound_ = new_buffer + (rBound_ - buffer_); > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > buffer_ = new_buffer; > bufferSize_ = new_size; > {code} > If old bufferSize_ is lager than 2gb, then calculating new size will overflow. > i.e. if bufferSize_ = 3355443200, then new buffer size will be 2415919104, > which is less than old size. > However, > {code} > avail = available_write() + (new_size - bufferSize_) > {code} > overflows again, so we will end up with an shrinked buffer. > What is worse is that after > {code} > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > {code} > wBase_ stays the same, but wBound_ becomes lower than it. What happens next > is that uint32_t avail = available_write() may overflow every time > subsequently. and thrift writes to unknown memory. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[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 issue #1279: THRIFT-4212: Fix flush on ssl socket thrift
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1279 When THRIFT-4211 is clean in CI and merged, you will need to rebase this since it includes THRIFT-4211, and then we can get it into CI for a build then merge. 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (THRIFT-4212) c_glib flush tries to close SSL even if socket is invalid
[ https://issues.apache.org/jira/browse/THRIFT-4212?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122775#comment-16122775 ] ASF GitHub Bot commented on THRIFT-4212: Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1279 When THRIFT-4211 is clean in CI and merged, you will need to rebase this since it includes THRIFT-4211, and then we can get it into CI for a build then merge. Thanks. > c_glib flush tries to close SSL even if socket is invalid > - > > Key: THRIFT-4212 > URL: https://issues.apache.org/jira/browse/THRIFT-4212 > Project: Thrift > Issue Type: Bug > Components: C glib - Library >Affects Versions: 1.0 >Reporter: Gonzalo Aguilar >Assignee: Gonzalo Aguilar > Fix For: 1.0 > > > It seems that SSL is trying to get info from socket even when socket is > already invalid, making the app to crash. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-4211) Fix GError glib management under Thrift
[ https://issues.apache.org/jira/browse/THRIFT-4211?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122773#comment-16122773 ] ASF GitHub Bot commented on THRIFT-4211: Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1278 Would you be able to rebase against master and push so there is one commit (just like there is now), but more importantly it will kick off a new CI build for us. Thanks. > Fix GError glib management under Thrift > --- > > Key: THRIFT-4211 > URL: https://issues.apache.org/jira/browse/THRIFT-4211 > Project: Thrift > Issue Type: Bug > Components: C glib - Library >Affects Versions: 1.0 >Reporter: Gonzalo Aguilar >Assignee: Gonzalo Aguilar > Fix For: 1.0 > > > It seems that current Gerror management done in thrift is not quite ok and > causes the library to fail. > This issue tracks and fixes all problems found during testing. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1278: THRIFT-4211: Fix logging in thrift library
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1278 Would you be able to rebase against master and push so there is one commit (just like there is now), but more importantly it will kick off a new CI build for us. 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1265: THRIFT-3963 Thrift.cabal filename does not match ...
Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1265 --- 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. ---
[jira] [Commented] (THRIFT-3963) Thrift.cabal filename does not match module name
[ https://issues.apache.org/jira/browse/THRIFT-3963?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122771#comment-16122771 ] ASF GitHub Bot commented on THRIFT-3963: Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1265 > Thrift.cabal filename does not match module name > > > Key: THRIFT-3963 > URL: https://issues.apache.org/jira/browse/THRIFT-3963 > Project: Thrift > Issue Type: Bug > Components: Haskell - Library >Affects Versions: 0.10.0 > Environment: Haskell Stack with LTS 7.7, on Ubuntu >Reporter: Isaac Sheff >Assignee: James E. King, III > Labels: github-import > Fix For: 0.11.0 > > Original Estimate: 20m > Remaining Estimate: 20m > > Some haskell environments, including stack (http://haskellstack.org/) require > that the cabal file name match the module name (case sensitive). This means > that it is difficult to import the (git source repository) thrift library > into stack projects (https://github.com/commercialhaskell/stack/issues/317). > The fix is easy: change the name "Thrift.cabal" to "thrift.cabal." -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Resolved] (THRIFT-3963) Thrift.cabal filename does not match module name
[ https://issues.apache.org/jira/browse/THRIFT-3963?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] James E. King, III resolved THRIFT-3963. Resolution: Fixed Fix Version/s: 0.11.0 > Thrift.cabal filename does not match module name > > > Key: THRIFT-3963 > URL: https://issues.apache.org/jira/browse/THRIFT-3963 > Project: Thrift > Issue Type: Bug > Components: Haskell - Library >Affects Versions: 0.10.0 > Environment: Haskell Stack with LTS 7.7, on Ubuntu >Reporter: Isaac Sheff >Assignee: James E. King, III > Labels: github-import > Fix For: 0.11.0 > > Original Estimate: 20m > Remaining Estimate: 20m > > Some haskell environments, including stack (http://haskellstack.org/) require > that the cabal file name match the module name (case sensitive). This means > that it is difficult to import the (git source repository) thrift library > into stack projects (https://github.com/commercialhaskell/stack/issues/317). > The fix is easy: change the name "Thrift.cabal" to "thrift.cabal." -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Updated] (THRIFT-3963) Thrift.cabal filename does not match module name
[ https://issues.apache.org/jira/browse/THRIFT-3963?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] James E. King, III updated THRIFT-3963: --- Affects Version/s: (was: 1.0) 0.10.0 > Thrift.cabal filename does not match module name > > > Key: THRIFT-3963 > URL: https://issues.apache.org/jira/browse/THRIFT-3963 > Project: Thrift > Issue Type: Bug > Components: Haskell - Library >Affects Versions: 0.10.0 > Environment: Haskell Stack with LTS 7.7, on Ubuntu >Reporter: Isaac Sheff > Labels: github-import > Original Estimate: 20m > Remaining Estimate: 20m > > Some haskell environments, including stack (http://haskellstack.org/) require > that the cabal file name match the module name (case sensitive). This means > that it is difficult to import the (git source repository) thrift library > into stack projects (https://github.com/commercialhaskell/stack/issues/317). > The fix is easy: change the name "Thrift.cabal" to "thrift.cabal." -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Assigned] (THRIFT-3963) Thrift.cabal filename does not match module name
[ https://issues.apache.org/jira/browse/THRIFT-3963?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] James E. King, III reassigned THRIFT-3963: -- Assignee: James E. King, III > Thrift.cabal filename does not match module name > > > Key: THRIFT-3963 > URL: https://issues.apache.org/jira/browse/THRIFT-3963 > Project: Thrift > Issue Type: Bug > Components: Haskell - Library >Affects Versions: 0.10.0 > Environment: Haskell Stack with LTS 7.7, on Ubuntu >Reporter: Isaac Sheff >Assignee: James E. King, III > Labels: github-import > Original Estimate: 20m > Remaining Estimate: 20m > > Some haskell environments, including stack (http://haskellstack.org/) require > that the cabal file name match the module name (case sensitive). This means > that it is difficult to import the (git source repository) thrift library > into stack projects (https://github.com/commercialhaskell/stack/issues/317). > The fix is easy: change the name "Thrift.cabal" to "thrift.cabal." -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-4248) Compile fails - strncpy, memcmp, memset not declared in src/thrift/transport/TSSLSocket.cpp
[ https://issues.apache.org/jira/browse/THRIFT-4248?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122765#comment-16122765 ] ASF GitHub Bot commented on THRIFT-4248: Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1307 > Compile fails - strncpy, memcmp, memset not declared in > src/thrift/transport/TSSLSocket.cpp > --- > > Key: THRIFT-4248 > URL: https://issues.apache.org/jira/browse/THRIFT-4248 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.10.0 >Reporter: Josip Sokcevic >Assignee: James E. King, III > Fix For: 0.11.0 > > Attachments: 0001-THRIFT-4248-Import-cstring-in-TSSLSocket.patch > > -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift pull request #1307: THRIFT-4248: Import cstring in TSSLSocket
Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1307 --- 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. ---
[jira] [Assigned] (THRIFT-4248) Compile fails - strncpy, memcmp, memset not declared in src/thrift/transport/TSSLSocket.cpp
[ https://issues.apache.org/jira/browse/THRIFT-4248?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] James E. King, III reassigned THRIFT-4248: -- Assignee: James E. King, III > Compile fails - strncpy, memcmp, memset not declared in > src/thrift/transport/TSSLSocket.cpp > --- > > Key: THRIFT-4248 > URL: https://issues.apache.org/jira/browse/THRIFT-4248 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.10.0 >Reporter: Josip Sokcevic >Assignee: James E. King, III > Fix For: 0.11.0 > > Attachments: 0001-THRIFT-4248-Import-cstring-in-TSSLSocket.patch > > -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-4248) Compile fails - strncpy, memcmp, memset not declared in src/thrift/transport/TSSLSocket.cpp
[ https://issues.apache.org/jira/browse/THRIFT-4248?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122758#comment-16122758 ] James E. King, III commented on THRIFT-4248: Compile fails - on what platform / environment? Not in CI and not on any of my machines? > Compile fails - strncpy, memcmp, memset not declared in > src/thrift/transport/TSSLSocket.cpp > --- > > Key: THRIFT-4248 > URL: https://issues.apache.org/jira/browse/THRIFT-4248 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.10.0 >Reporter: Josip Sokcevic > Attachments: 0001-THRIFT-4248-Import-cstring-in-TSSLSocket.patch > > -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-3821) TMemoryBuffer buffer may overflow when resizing
[ https://issues.apache.org/jira/browse/THRIFT-3821?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122757#comment-16122757 ] ASF GitHub Bot commented on 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. > TMemoryBuffer buffer may overflow when resizing > --- > > Key: THRIFT-3821 > URL: https://issues.apache.org/jira/browse/THRIFT-3821 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.9.3 >Reporter: Huaisi Xu > > In ensurecanwrite(): > {code} > uint32_t new_size = bufferSize_; > while (len > avail) { > new_size = new_size > 0 ? new_size * 2 : 1; > avail = available_write() + (new_size - bufferSize_); > } > // Allocate into a new pointer so we don't bork ours if it fails. > uint8_t* new_buffer = static_cast(std::realloc(buffer_, > new_size)); > if (new_buffer == NULL) { > throw std::bad_alloc(); > } > rBase_ = new_buffer + (rBase_ - buffer_); > rBound_ = new_buffer + (rBound_ - buffer_); > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > buffer_ = new_buffer; > bufferSize_ = new_size; > {code} > If old bufferSize_ is lager than 2gb, then calculating new size will overflow. > i.e. if bufferSize_ = 3355443200, then new buffer size will be 2415919104, > which is less than old size. > However, > {code} > avail = available_write() + (new_size - bufferSize_) > {code} > overflows again, so we will end up with an shrinked buffer. > What is worse is that after > {code} > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > {code} > wBase_ stays the same, but wBound_ becomes lower than it. What happens next > is that uint32_t avail = available_write() may overflow every time > subsequently. and thrift writes to unknown memory. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-3821) TMemoryBuffer buffer may overflow when resizing
[ https://issues.apache.org/jira/browse/THRIFT-3821?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122756#comment-16122756 ] ASF GitHub Bot commented on 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. > TMemoryBuffer buffer may overflow when resizing > --- > > Key: THRIFT-3821 > URL: https://issues.apache.org/jira/browse/THRIFT-3821 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.9.3 >Reporter: Huaisi Xu > > In ensurecanwrite(): > {code} > uint32_t new_size = bufferSize_; > while (len > avail) { > new_size = new_size > 0 ? new_size * 2 : 1; > avail = available_write() + (new_size - bufferSize_); > } > // Allocate into a new pointer so we don't bork ours if it fails. > uint8_t* new_buffer = static_cast(std::realloc(buffer_, > new_size)); > if (new_buffer == NULL) { > throw std::bad_alloc(); > } > rBase_ = new_buffer + (rBase_ - buffer_); > rBound_ = new_buffer + (rBound_ - buffer_); > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > buffer_ = new_buffer; > bufferSize_ = new_size; > {code} > If old bufferSize_ is lager than 2gb, then calculating new size will overflow. > i.e. if bufferSize_ = 3355443200, then new buffer size will be 2415919104, > which is less than old size. > However, > {code} > avail = available_write() + (new_size - bufferSize_) > {code} > overflows again, so we will end up with an shrinked buffer. > What is worse is that after > {code} > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > {code} > wBase_ stays the same, but wBound_ becomes lower than it. What happens next > is that uint32_t avail = available_write() may overflow every time > subsequently. and thrift writes to unknown memory. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[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. ---
[jira] [Commented] (THRIFT-3821) TMemoryBuffer buffer may overflow when resizing
[ https://issues.apache.org/jira/browse/THRIFT-3821?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122696#comment-16122696 ] ASF GitHub Bot commented on THRIFT-3821: 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. > TMemoryBuffer buffer may overflow when resizing > --- > > Key: THRIFT-3821 > URL: https://issues.apache.org/jira/browse/THRIFT-3821 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.9.3 >Reporter: Huaisi Xu > > In ensurecanwrite(): > {code} > uint32_t new_size = bufferSize_; > while (len > avail) { > new_size = new_size > 0 ? new_size * 2 : 1; > avail = available_write() + (new_size - bufferSize_); > } > // Allocate into a new pointer so we don't bork ours if it fails. > uint8_t* new_buffer = static_cast(std::realloc(buffer_, > new_size)); > if (new_buffer == NULL) { > throw std::bad_alloc(); > } > rBase_ = new_buffer + (rBase_ - buffer_); > rBound_ = new_buffer + (rBound_ - buffer_); > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > buffer_ = new_buffer; > bufferSize_ = new_size; > {code} > If old bufferSize_ is lager than 2gb, then calculating new size will overflow. > i.e. if bufferSize_ = 3355443200, then new buffer size will be 2415919104, > which is less than old size. > However, > {code} > avail = available_write() + (new_size - bufferSize_) > {code} > overflows again, so we will end up with an shrinked buffer. > What is worse is that after > {code} > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > {code} > wBase_ stays the same, but wBound_ becomes lower than it. What happens next > is that uint32_t avail = available_write() may overflow every time > subsequently. and thrift writes to unknown memory. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-3821) TMemoryBuffer buffer may overflow when resizing
[ https://issues.apache.org/jira/browse/THRIFT-3821?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122386#comment-16122386 ] ASF GitHub Bot commented on THRIFT-3821: 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"). > TMemoryBuffer buffer may overflow when resizing > --- > > Key: THRIFT-3821 > URL: https://issues.apache.org/jira/browse/THRIFT-3821 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.9.3 >Reporter: Huaisi Xu > > In ensurecanwrite(): > {code} > uint32_t new_size = bufferSize_; > while (len > avail) { > new_size = new_size > 0 ? new_size * 2 : 1; > avail = available_write() + (new_size - bufferSize_); > } > // Allocate into a new pointer so we don't bork ours if it fails. > uint8_t* new_buffer = static_cast(std::realloc(buffer_, > new_size)); > if (new_buffer == NULL) { > throw std::bad_alloc(); > } > rBase_ = new_buffer + (rBase_ - buffer_); > rBound_ = new_buffer + (rBound_ - buffer_); > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > buffer_ = new_buffer; > bufferSize_ = new_size; > {code} > If old bufferSize_ is lager than 2gb, then calculating new size will overflow. > i.e. if bufferSize_ = 3355443200, then new buffer size will be 2415919104, > which is less than old size. > However, > {code} > avail = available_write() + (new_size - bufferSize_) > {code} > overflows again, so we will end up with an shrinked buffer. > What is worse is that after > {code} > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > {code} > wBase_ stays the same, but wBound_ becomes lower than it. What happens next > is that uint32_t avail = available_write() may overflow every time > subsequently. and thrift writes to unknown memory. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-4246) Sequence number mismatch on multiplexed clients
[ https://issues.apache.org/jira/browse/THRIFT-4246?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122274#comment-16122274 ] ASF GitHub Bot commented on THRIFT-4246: Github user boivie commented on the issue: https://github.com/apache/thrift/pull/1322 Thanks James. I just rebased it, but travis and AppVeyor fails from unrelated reasons. Can they be retriggered? > Sequence number mismatch on multiplexed clients > --- > > Key: THRIFT-4246 > URL: https://issues.apache.org/jira/browse/THRIFT-4246 > Project: Thrift > Issue Type: Bug > Components: Node.js - Library >Affects Versions: 0.10.0 >Reporter: Victor Boivie >Priority: Critical > Attachments: > 0001-THRIFT-4246-Multiplexed-clients-sequence-id-fix.patch > > > When performing calls using a multiplexed client and when having multiple > connections and clients, the wrong sequence numbers are used which will often > result in responses not being able to be delivered to the client. This is > because every connection will make the client class (not instance) use the > latest created multiplexer class to generate sequence numbers. The following > example shows the problem: > {code:javascript} > const thrift = require('thrift'); > const AlphaService = require('./gen-nodejs/AlphaService'); > const BetaService = require('./gen-nodejs/BetaService'); > let connection1 = thrift.createConnection('localhost', 9091, { > transport: thrift.TFrameTransport, > protocol: thrift.TCompactProtocol, > }); > let multiplexer1 = new thrift.Multiplexer(); > let alpha1 = multiplexer1.createClient('alpha', AlphaService, connection1); > let beta1 = multiplexer1.createClient('beta', BetaService, connection1); > alpha1.echoAlpha("hello") > let connection2 = thrift.createConnection('localhost', 9091, { > transport: thrift.TFrameTransport, > protocol: thrift.TCompactProtocol, > }); > let multiplexer2 = new thrift.Multiplexer(); > let alpha2 = multiplexer2.createClient('alpha', AlphaService, connection2); > let beta2 = multiplexer2.createClient('beta', BetaService, connection2); > beta1.echoBeta("Hi") > beta2.echoBeta("hello") > console.log("alpha1 seqId", alpha1._seqid) > console.log("beta1 seqId", beta1._seqid) > console.log("beta2 seqId", beta2._seqid) > console.log("multiplexer1 latest", multiplexer1.seqid) > console.log("multiplexer2 latest", multiplexer2.seqid) > console.log("connection1 mapping", connection1.seqId2Service) > console.log("connection2 mapping", connection2.seqId2Service) > {code} > Result: > {noformat} > alpha1 seqId 1 > beta1 seqId 1 > beta2 seqId 2 > multiplexer1 latest 1 > multiplexer2 latest 2 > connection1 mapping { '1': 'beta' } > connection2 mapping { '2': 'beta' } > {noformat} > Connection1 should have mapping 1 -> alpha, 2-> beta -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1322: THRIFT-4246 Multiplexed clients sequence id fix
Github user boivie commented on the issue: https://github.com/apache/thrift/pull/1322 Thanks James. I just rebased it, but travis and AppVeyor fails from unrelated reasons. Can they be retriggered? --- 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. ---
[jira] [Commented] (THRIFT-4274) Python feature tests for SSL/TLS failing
[ https://issues.apache.org/jira/browse/THRIFT-4274?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122187#comment-16122187 ] ASF GitHub Bot commented on THRIFT-4274: Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1321 > Python feature tests for SSL/TLS failing > > > Key: THRIFT-4274 > URL: https://issues.apache.org/jira/browse/THRIFT-4274 > Project: Thrift > Issue Type: Bug > Components: Python - Library > Environment: Arch Linux, Python 3.6.2, OpenSSL 1.1.0.f >Reporter: Håkon Hitland >Assignee: James E. King, III >Priority: Minor > Fix For: 0.11.0 > > > {{make cross}} fails on the Python feature tests {{py-nosslv3}} and > {{py-tls}}, as the test server exits with a stack trace like > {noformat} > Traceback (most recent call last): > File "/thrift/test/py/TestServer.py", line 315, in > sys.exit(main(options)) > File "/thrift/test/py/TestServer.py", line 268, in main > server.serve() > File "/thrift/lib/py/build/lib.linux-x86_64-3.6/thrift/server/TServer.py", > line 76, in serve > client = self.serverTransport.accept() > File > "/thrift/lib/py/build/lib.linux-x86_64-3.6/thrift/transport/TSSLSocket.py", > line 370, in accept > client = self._wrap_socket(plain_client) > File > "/thrift/lib/py/build/lib.linux-x86_64-3.6/thrift/transport/TSSLSocket.py", > line 184, in _wrap_socket > server_hostname=self._server_hostname) > File "/usr/lib/python3.6/ssl.py", line 401, in wrap_socket > _context=self, _session=session) > File "/usr/lib/python3.6/ssl.py", line 808, in __init__ > self.do_handshake() > File "/usr/lib/python3.6/ssl.py", line 1061, in do_handshake > self._sslobj.do_handshake() > File "/usr/lib/python3.6/ssl.py", line 683, in do_handshake > self._sslobj.do_handshake() > OSError: [Errno 0] Error > {noformat} > This seems to be caused by {{ensure_socket_open}} in > {{test/crossrunner/run.py}}, which will make a connection and immediately > close the socket without completing the handshake. > On the server side this causes an {{OSError}} to be thrown from > {{SSLContext.wrap_socket()}}, which is not caught in > {{TSSLServerSocket.accept()}}. > This works on older installations, so presumably either Python or OpenSSL has > changed to cause an {{OSError}} instead of a {{SSLError}}. > Catching {{OSError}} in {{accept()}} and handling it the same as {{SSLError}} > looks like a reasonable fix. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift pull request #1321: THRIFT-4274: Catch OSError in TSSLServerSocket.ac...
Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1321 --- 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. ---
[jira] [Commented] (THRIFT-3821) TMemoryBuffer buffer may overflow when resizing
[ https://issues.apache.org/jira/browse/THRIFT-3821?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122170#comment-16122170 ] ASF GitHub Bot commented on THRIFT-3821: 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. > TMemoryBuffer buffer may overflow when resizing > --- > > Key: THRIFT-3821 > URL: https://issues.apache.org/jira/browse/THRIFT-3821 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.9.3 >Reporter: Huaisi Xu > > In ensurecanwrite(): > {code} > uint32_t new_size = bufferSize_; > while (len > avail) { > new_size = new_size > 0 ? new_size * 2 : 1; > avail = available_write() + (new_size - bufferSize_); > } > // Allocate into a new pointer so we don't bork ours if it fails. > uint8_t* new_buffer = static_cast(std::realloc(buffer_, > new_size)); > if (new_buffer == NULL) { > throw std::bad_alloc(); > } > rBase_ = new_buffer + (rBase_ - buffer_); > rBound_ = new_buffer + (rBound_ - buffer_); > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > buffer_ = new_buffer; > bufferSize_ = new_size; > {code} > If old bufferSize_ is lager than 2gb, then calculating new size will overflow. > i.e. if bufferSize_ = 3355443200, then new buffer size will be 2415919104, > which is less than old size. > However, > {code} > avail = available_write() + (new_size - bufferSize_) > {code} > overflows again, so we will end up with an shrinked buffer. > What is worse is that after > {code} > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > {code} > wBase_ stays the same, but wBound_ becomes lower than it. What happens next > is that uint32_t avail = available_write() may overflow every time > subsequently. and thrift writes to unknown memory. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-3775) 0.10.0 release candidate
[ https://issues.apache.org/jira/browse/THRIFT-3775?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122171#comment-16122171 ] ASF GitHub Bot commented on THRIFT-3775: Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1323 @jfarrell this needs to be closed. It is invalid. > 0.10.0 release candidate > > > Key: THRIFT-3775 > URL: https://issues.apache.org/jira/browse/THRIFT-3775 > Project: Thrift > Issue Type: Bug > Components: Build Process >Reporter: Jake Farrell >Assignee: Jake Farrell > Fix For: 0.10.0 > > -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1323: THRIFT-3775: 0.10.0 release
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1323 @jfarrell this needs to be closed. It is invalid. --- 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. ---
[jira] [Commented] (THRIFT-3821) TMemoryBuffer buffer may overflow when resizing
[ https://issues.apache.org/jira/browse/THRIFT-3821?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122168#comment-16122168 ] ASF GitHub Bot commented on 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. > TMemoryBuffer buffer may overflow when resizing > --- > > Key: THRIFT-3821 > URL: https://issues.apache.org/jira/browse/THRIFT-3821 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.9.3 >Reporter: Huaisi Xu > > In ensurecanwrite(): > {code} > uint32_t new_size = bufferSize_; > while (len > avail) { > new_size = new_size > 0 ? new_size * 2 : 1; > avail = available_write() + (new_size - bufferSize_); > } > // Allocate into a new pointer so we don't bork ours if it fails. > uint8_t* new_buffer = static_cast(std::realloc(buffer_, > new_size)); > if (new_buffer == NULL) { > throw std::bad_alloc(); > } > rBase_ = new_buffer + (rBase_ - buffer_); > rBound_ = new_buffer + (rBound_ - buffer_); > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > buffer_ = new_buffer; > bufferSize_ = new_size; > {code} > If old bufferSize_ is lager than 2gb, then calculating new size will overflow. > i.e. if bufferSize_ = 3355443200, then new buffer size will be 2415919104, > which is less than old size. > However, > {code} > avail = available_write() + (new_size - bufferSize_) > {code} > overflows again, so we will end up with an shrinked buffer. > What is worse is that after > {code} > wBase_ = new_buffer + (wBase_ - buffer_); > wBound_ = new_buffer + new_size; > {code} > wBase_ stays the same, but wBound_ becomes lower than it. What happens next > is that uint32_t avail = available_write() may overflow every time > subsequently. and thrift writes to unknown memory. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[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 issue #1327: make thrift error:class ‘apache::thrift::transport::TH...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1327 @jfarrell this pull request is bogus and this topic should be closed. I reproduced your issue with THeaderTransport under CentOS 6.9 and the same installation instructions. The construction arguments need to pass the complete template in this case: : TVirtualTransport(inTransport) instead of : TVirtualTransport(inTransport) That should get you past the issue. It appears none of the more recent compilers require this to function properly, so we're probably not going to patch for 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (THRIFT-2221) Generate c++ code with std::shared_ptr instead of boost::shared_ptr.
[ https://issues.apache.org/jira/browse/THRIFT-2221?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16122089#comment-16122089 ] Mario Emmenlauer commented on THRIFT-2221: -- Thanks again [~jking] , super great work and its highly appreciated! I'm very much looking forward to this release! > Generate c++ code with std::shared_ptr instead of boost::shared_ptr. > > > Key: THRIFT-2221 > URL: https://issues.apache.org/jira/browse/THRIFT-2221 > Project: Thrift > Issue Type: Improvement > Components: C++ - Compiler, C++ - Library >Affects Versions: 0.9.1 > Environment: C++11 compilers with std::shared_ptr support >Reporter: Chris Stylianou >Assignee: James E. King, III > Labels: c++11, compiler, thrift > Fix For: 0.11.0 > > > Most modern compilers now have full support for std::shared_ptr when enable > with c++11 flags. It would be nice to have the option to generate code that > uses this instead of boost::shared_ptr. This would enable us to remove > another boost dependency, on the road to a dependency-free thrift library :) > Solution summary (so you don't need to wade through the comments): > A new header, , creates the namespace apache::thrift::stdcxx > and aliases the proper std:: or boost:: constructs depending on the > capabilities of the environment and whether any build flags are used to force > an override of this behavior. All code throughout the project now uses > stdcxx:: for all smart_ptr and functional (bind, function, placeholders, _1, > _2, ...) constructs. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-3978) Thrift C++ runtime uses assert to prevent overflows, checks sanity only in debug builds
[ https://issues.apache.org/jira/browse/THRIFT-3978?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16121982#comment-16121982 ] James E. King, III commented on THRIFT-3978: The following still need to be cleaned up before we can consider this story complete: {noformat} jking@dvm:~/thrift/github/thrift$ find lib/cpp/src -type f -name '*.cpp' -exec grep -l 'assert' {} \; lib/cpp/src/thrift/VirtualProfiling.cpp lib/cpp/src/thrift/windows/OverlappedSubmissionThread.cpp lib/cpp/src/thrift/windows/TWinsockSingleton.cpp lib/cpp/src/thrift/server/TNonblockingServer.cpp lib/cpp/src/thrift/protocol/TBase64Utils.cpp lib/cpp/src/thrift/protocol/TDebugProtocol.cpp lib/cpp/src/thrift/protocol/THeaderProtocol.cpp lib/cpp/src/thrift/protocol/TJSONProtocol.cpp lib/cpp/src/thrift/concurrency/Monitor.cpp lib/cpp/src/thrift/concurrency/BoostMonitor.cpp lib/cpp/src/thrift/concurrency/TimerManager.cpp lib/cpp/src/thrift/concurrency/StdThreadFactory.cpp lib/cpp/src/thrift/concurrency/BoostThreadFactory.cpp lib/cpp/src/thrift/concurrency/Util.cpp lib/cpp/src/thrift/concurrency/StdMutex.cpp lib/cpp/src/thrift/concurrency/Mutex.cpp lib/cpp/src/thrift/concurrency/StdMonitor.cpp lib/cpp/src/thrift/concurrency/BoostMutex.cpp lib/cpp/src/thrift/concurrency/PosixThreadFactory.cpp lib/cpp/src/thrift/transport/TFileTransport.cpp lib/cpp/src/thrift/transport/TBufferTransports.cpp lib/cpp/src/thrift/transport/TSSLSocket.cpp lib/cpp/src/thrift/transport/TZlibTransport.cpp lib/cpp/src/thrift/transport/TPipe.cpp lib/cpp/src/thrift/async/TEvhttpClientChannel.cpp {noformat} > Thrift C++ runtime uses assert to prevent overflows, checks sanity only in > debug builds > --- > > Key: THRIFT-3978 > URL: https://issues.apache.org/jira/browse/THRIFT-3978 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.10.0 > Environment: All >Reporter: James E. King, III >Assignee: James E. King, III > Labels: security > > Currently there is widespread use of assert in the thrift C++ runtime > library. Some of the more disturbing cases are security related, for example > checking header sizes. I recommend we eliminate assertions that are only > checked in debug mode, and instead throw the appropriate exception, usually a > TTransportException with CORRUPTED_DATA as the reason. If we're going to > check for an overflow or a buffer overrun, we should do so in debug and > release modes. Further, assertions are not easily tested whereas exceptions > are. > In THRIFT-3873 apache::thrift::transport::safe_numeric_cast was added, so I > also suggest changing static_cast to safe_numeric_cast where appropriate > throughout the transport code to catch any overflow errors. > Another location where assert is used liberally is inside the posix Mutex > implementation. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Updated] (THRIFT-2221) Generate c++ code with std::shared_ptr instead of boost::shared_ptr.
[ https://issues.apache.org/jira/browse/THRIFT-2221?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] James E. King, III updated THRIFT-2221: --- Description: Most modern compilers now have full support for std::shared_ptr when enable with c++11 flags. It would be nice to have the option to generate code that uses this instead of boost::shared_ptr. This would enable us to remove another boost dependency, on the road to a dependency-free thrift library :) Solution summary (so you don't need to wade through the comments): A new header, , creates the namespace apache::thrift::stdcxx and aliases the proper std:: or boost:: constructs depending on the capabilities of the environment and whether any build flags are used to force an override of this behavior. All code throughout the project now uses stdcxx:: for all smart_ptr and functional (bind, function, placeholders, _1, _2, ...) constructs. was:Most modern compilers now have full support for std::shared_ptr when enable with c++11 flags. It would be nice to have the option to generate code that uses this instead of boost::shared_ptr. This would enable us to remove another boost dependency, on the road to a dependency-free thrift library :) > Generate c++ code with std::shared_ptr instead of boost::shared_ptr. > > > Key: THRIFT-2221 > URL: https://issues.apache.org/jira/browse/THRIFT-2221 > Project: Thrift > Issue Type: Improvement > Components: C++ - Compiler, C++ - Library >Affects Versions: 0.9.1 > Environment: C++11 compilers with std::shared_ptr support >Reporter: Chris Stylianou >Assignee: James E. King, III > Labels: c++11, compiler, thrift > Fix For: 0.11.0 > > > Most modern compilers now have full support for std::shared_ptr when enable > with c++11 flags. It would be nice to have the option to generate code that > uses this instead of boost::shared_ptr. This would enable us to remove > another boost dependency, on the road to a dependency-free thrift library :) > Solution summary (so you don't need to wade through the comments): > A new header, , creates the namespace apache::thrift::stdcxx > and aliases the proper std:: or boost:: constructs depending on the > capabilities of the environment and whether any build flags are used to force > an override of this behavior. All code throughout the project now uses > stdcxx:: for all smart_ptr and functional (bind, function, placeholders, _1, > _2, ...) constructs. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Updated] (THRIFT-2221) Generate c++ code with std::shared_ptr instead of boost::shared_ptr.
[ https://issues.apache.org/jira/browse/THRIFT-2221?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] James E. King, III updated THRIFT-2221: --- Fix Version/s: 0.11.0 Component/s: C++ - Library > Generate c++ code with std::shared_ptr instead of boost::shared_ptr. > > > Key: THRIFT-2221 > URL: https://issues.apache.org/jira/browse/THRIFT-2221 > Project: Thrift > Issue Type: Improvement > Components: C++ - Compiler, C++ - Library >Affects Versions: 0.9.1 > Environment: C++11 compilers with std::shared_ptr support >Reporter: Chris Stylianou >Assignee: James E. King, III > Labels: c++11, compiler, thrift > Fix For: 0.11.0 > > > Most modern compilers now have full support for std::shared_ptr when enable > with c++11 flags. It would be nice to have the option to generate code that > uses this instead of boost::shared_ptr. This would enable us to remove > another boost dependency, on the road to a dependency-free thrift library :) -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Resolved] (THRIFT-2221) Generate c++ code with std::shared_ptr instead of boost::shared_ptr.
[ https://issues.apache.org/jira/browse/THRIFT-2221?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] James E. King, III resolved THRIFT-2221. Resolution: Fixed > Generate c++ code with std::shared_ptr instead of boost::shared_ptr. > > > Key: THRIFT-2221 > URL: https://issues.apache.org/jira/browse/THRIFT-2221 > Project: Thrift > Issue Type: Improvement > Components: C++ - Compiler, C++ - Library >Affects Versions: 0.9.1 > Environment: C++11 compilers with std::shared_ptr support >Reporter: Chris Stylianou >Assignee: James E. King, III > Labels: c++11, compiler, thrift > Fix For: 0.11.0 > > > Most modern compilers now have full support for std::shared_ptr when enable > with c++11 flags. It would be nice to have the option to generate code that > uses this instead of boost::shared_ptr. This would enable us to remove > another boost dependency, on the road to a dependency-free thrift library :) -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-2221) Generate c++ code with std::shared_ptr instead of boost::shared_ptr.
[ https://issues.apache.org/jira/browse/THRIFT-2221?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16121953#comment-16121953 ] James E. King, III commented on THRIFT-2221: Okay that one was fun... :) > Generate c++ code with std::shared_ptr instead of boost::shared_ptr. > > > Key: THRIFT-2221 > URL: https://issues.apache.org/jira/browse/THRIFT-2221 > Project: Thrift > Issue Type: Improvement > Components: C++ - Compiler >Affects Versions: 0.9.1 > Environment: C++11 compilers with std::shared_ptr support >Reporter: Chris Stylianou >Assignee: James E. King, III > Labels: c++11, compiler, thrift > > Most modern compilers now have full support for std::shared_ptr when enable > with c++11 flags. It would be nice to have the option to generate code that > uses this instead of boost::shared_ptr. This would enable us to remove > another boost dependency, on the road to a dependency-free thrift library :) -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-2221) Generate c++ code with std::shared_ptr instead of boost::shared_ptr.
[ https://issues.apache.org/jira/browse/THRIFT-2221?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16121952#comment-16121952 ] ASF GitHub Bot commented on THRIFT-2221: Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1328 > Generate c++ code with std::shared_ptr instead of boost::shared_ptr. > > > Key: THRIFT-2221 > URL: https://issues.apache.org/jira/browse/THRIFT-2221 > Project: Thrift > Issue Type: Improvement > Components: C++ - Compiler >Affects Versions: 0.9.1 > Environment: C++11 compilers with std::shared_ptr support >Reporter: Chris Stylianou >Assignee: James E. King, III > Labels: c++11, compiler, thrift > > Most modern compilers now have full support for std::shared_ptr when enable > with c++11 flags. It would be nice to have the option to generate code that > uses this instead of boost::shared_ptr. This would enable us to remove > another boost dependency, on the road to a dependency-free thrift library :) -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift pull request #1328: THRIFT-2221: detect C++11 and use std namespace f...
Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1328 --- 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. ---
[jira] [Commented] (THRIFT-2221) Generate c++ code with std::shared_ptr instead of boost::shared_ptr.
[ https://issues.apache.org/jira/browse/THRIFT-2221?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16121937#comment-16121937 ] ASF GitHub Bot commented on THRIFT-2221: Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1328 The lone build failure is a "D" language/test issue that appears sporadically. All other builds passed. As such, I am going to merge this in given it was already reviewed, and the follow-on changes were fairly trivial. > Generate c++ code with std::shared_ptr instead of boost::shared_ptr. > > > Key: THRIFT-2221 > URL: https://issues.apache.org/jira/browse/THRIFT-2221 > Project: Thrift > Issue Type: Improvement > Components: C++ - Compiler >Affects Versions: 0.9.1 > Environment: C++11 compilers with std::shared_ptr support >Reporter: Chris Stylianou >Assignee: James E. King, III > Labels: c++11, compiler, thrift > > Most modern compilers now have full support for std::shared_ptr when enable > with c++11 flags. It would be nice to have the option to generate code that > uses this instead of boost::shared_ptr. This would enable us to remove > another boost dependency, on the road to a dependency-free thrift library :) -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1328: THRIFT-2221: detect C++11 and use std namespace for memo...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1328 The lone build failure is a "D" language/test issue that appears sporadically. All other builds passed. As such, I am going to merge this in given it was already reviewed, and the follow-on changes were fairly trivial. --- 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. ---
[jira] [Commented] (THRIFT-2221) Generate c++ code with std::shared_ptr instead of boost::shared_ptr.
[ https://issues.apache.org/jira/browse/THRIFT-2221?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16121665#comment-16121665 ] ASF GitHub Bot commented on THRIFT-2221: Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1328 I added a TPipe test because I changed one line of code in TPipe to work around a build issue, and I didn't see a TTransportTest test for TPipe. > Generate c++ code with std::shared_ptr instead of boost::shared_ptr. > > > Key: THRIFT-2221 > URL: https://issues.apache.org/jira/browse/THRIFT-2221 > Project: Thrift > Issue Type: Improvement > Components: C++ - Compiler >Affects Versions: 0.9.1 > Environment: C++11 compilers with std::shared_ptr support >Reporter: Chris Stylianou >Assignee: James E. King, III > Labels: c++11, compiler, thrift > > Most modern compilers now have full support for std::shared_ptr when enable > with c++11 flags. It would be nice to have the option to generate code that > uses this instead of boost::shared_ptr. This would enable us to remove > another boost dependency, on the road to a dependency-free thrift library :) -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift issue #1328: THRIFT-2221: detect C++11 and use std namespace for memo...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1328 I added a TPipe test because I changed one line of code in TPipe to work around a build issue, and I didn't see a TTransportTest test for TPipe. --- 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. ---
[jira] [Updated] (THRIFT-4283) TNamedPipeServer race condition in interrupt
[ https://issues.apache.org/jira/browse/THRIFT-4283?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Jean-Noël Goor updated THRIFT-4283: --- Labels: c++ (was: ) > TNamedPipeServer race condition in interrupt > > > Key: THRIFT-4283 > URL: https://issues.apache.org/jira/browse/THRIFT-4283 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.10.0 > Environment: Windows >Reporter: Jean-Noël Goor > Labels: c++ > Attachments: thrift-4283-TNamedPipeServer-race-condition.patch > > > In TNamedPipeServer, there exists a race condition between the 'serve' thread > and another thread that would attempt to interrupt it. > While the 'serve' thread is waiting for clients to connect, it is blocked on > a GetOverlappedResult call. > To stop the server, another thread calls the interrupt function that submits > a CANCELIO operation to the TOverlappedSubmissionThread. > The ::CancelIo function is called, freeing the 'serve' thread with an > unsuccessful OvelappedResult which then throws and terminate the server by > deleting the serverTransport_. > All this can happen while the call to interrupt has not yet returned, and > even the event signaling the execution of the CANCELIO request might not have > been caught. > In our code, this result in unpredictable behavior (mostly detected as > stalled). > It is very hard to reproduce in test environment as it consists in a running > threads race condition. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Updated] (THRIFT-4283) TNamedPipeServer race condition in interrupt
[ https://issues.apache.org/jira/browse/THRIFT-4283?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Jean-Noël Goor updated THRIFT-4283: --- Attachment: thrift-4283-TNamedPipeServer-race-condition.patch > TNamedPipeServer race condition in interrupt > > > Key: THRIFT-4283 > URL: https://issues.apache.org/jira/browse/THRIFT-4283 > Project: Thrift > Issue Type: Bug > Components: C++ - Library >Affects Versions: 0.10.0 > Environment: Windows >Reporter: Jean-Noël Goor > Attachments: thrift-4283-TNamedPipeServer-race-condition.patch > > > In TNamedPipeServer, there exists a race condition between the 'serve' thread > and another thread that would attempt to interrupt it. > While the 'serve' thread is waiting for clients to connect, it is blocked on > a GetOverlappedResult call. > To stop the server, another thread calls the interrupt function that submits > a CANCELIO operation to the TOverlappedSubmissionThread. > The ::CancelIo function is called, freeing the 'serve' thread with an > unsuccessful OvelappedResult which then throws and terminate the server by > deleting the serverTransport_. > All this can happen while the call to interrupt has not yet returned, and > even the event signaling the execution of the CANCELIO request might not have > been caught. > In our code, this result in unpredictable behavior (mostly detected as > stalled). > It is very hard to reproduce in test environment as it consists in a running > threads race condition. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Created] (THRIFT-4283) TNamedPipeServer race condition in interrupt
Jean-Noël Goor created THRIFT-4283: -- Summary: TNamedPipeServer race condition in interrupt Key: THRIFT-4283 URL: https://issues.apache.org/jira/browse/THRIFT-4283 Project: Thrift Issue Type: Bug Components: C++ - Library Affects Versions: 0.10.0 Environment: Windows Reporter: Jean-Noël Goor In TNamedPipeServer, there exists a race condition between the 'serve' thread and another thread that would attempt to interrupt it. While the 'serve' thread is waiting for clients to connect, it is blocked on a GetOverlappedResult call. To stop the server, another thread calls the interrupt function that submits a CANCELIO operation to the TOverlappedSubmissionThread. The ::CancelIo function is called, freeing the 'serve' thread with an unsuccessful OvelappedResult which then throws and terminate the server by deleting the serverTransport_. All this can happen while the call to interrupt has not yet returned, and even the event signaling the execution of the CANCELIO request might not have been caught. In our code, this result in unpredictable behavior (mostly detected as stalled). It is very hard to reproduce in test environment as it consists in a running threads race condition. -- This message was sent by Atlassian JIRA (v6.4.14#64029)