[jira] [Updated] (THRIFT-5716) TMemoryBuffer resizing might shrink the buffer size due to uint32_t overflow

["Quanlong Huang (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/THRIFT-5716?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Quanlong Huang updated THRIFT-5716:
-----------------------------------
    Affects Version/s: 0.18.1
                       0.18.0
                       0.17.0
                       0.16.0
                       0.14.2
                       0.14.1
                       0.15.0

> TMemoryBuffer resizing might shrink the buffer size due to uint32_t overflow
> ----------------------------------------------------------------------------
>
>                 Key: THRIFT-5716
>                 URL: https://issues.apache.org/jira/browse/THRIFT-5716
>             Project: Thrift
>          Issue Type: Bug
>          Components: C++ - Library
>    Affects Versions: 0.14.0, 0.15.0, 0.14.1, 0.14.2, 0.16.0, 0.17.0, 0.18.0, 0.18.1
>            Reporter: Quanlong Huang
>            Assignee: Quanlong Huang
>            Priority: Critical
>             Fix For: 0.19.0
>
>          Time Spent: 1h
>  Remaining Estimate: 0h
>
> In TMemoryBuffer::ensureCanWrite(), the calculation of 'required_buffer_size' might overflow since it's the sum of two uint32_t values. The type of 'required_buffer_size' should be uint64_t.
> {code:cpp}
> void TMemoryBuffer::ensureCanWrite(uint32_t len) {
>   // Check available space
>   uint32_t avail = available_write();
>   if (len <= avail) {
>     return;
>   }
>   if (!owner_) {
>     throw TTransportException("Insufficient space in external MemoryBuffer");
>   }
>   // Grow the buffer as necessary.
>   const uint32_t current_used = bufferSize_ - avail;
>   const uint32_t required_buffer_size = len + current_used; // <-- This could overflow
>   if (required_buffer_size > maxBufferSize_) {
>     throw TTransportException(TTransportException::BAD_ARGS,
>                               "Internal buffer size overflow when requesting a buffer of size " + std::to_string(required_buffer_size));
>   }
>   // Always grow to the next bigger power of two:
>   const double suggested_buffer_size = std::exp2(std::ceil(std::log2(required_buffer_size)));
>   // Unless the power of two exceeds maxBufferSize_:
>   const uint64_t new_size = static_cast<uint64_t>((std::min)(suggested_buffer_size, static_cast<double>(maxBufferSize_)));
>   // Allocate into a new pointer so we don't bork ours if it fails.
>   auto* new_buffer = static_cast<uint8_t*>(std::realloc(buffer_, static_cast<std::size_t>(new_size)));
>   if (new_buffer == nullptr) {
>     throw std::bad_alloc();
>   }
>   rBase_ = new_buffer + (rBase_ - buffer_);
>   rBound_ = new_buffer + (rBound_ - buffer_);
>   wBase_ = new_buffer + (wBase_ - buffer_);
>   wBound_ = new_buffer + new_size;
>   // Note: with realloc() we do not need to free the previous buffer:
>   buffer_ = new_buffer;
>   bufferSize_ = static_cast<uint32_t>(new_size);
> } {code}
> Overflow example:
> {noformat}
> len=1066587646, current_used=4167372953, required_buffer_size=938993303{noformat}
> The buffer is not expanded if 'required_buffer_size' overflow. TMemoryBuffer::writeSlow() would finally writes to invalid address and crash the process:
> {code:cpp}
> void TMemoryBuffer::writeSlow(const uint8_t* buf, uint32_t len) {
>   ensureCanWrite(len);
>   // Copy into the buffer and increment wBase_.
>   memcpy(wBase_, buf, len);  // <-- This could crash
>   wBase_ += len;
> } {code}



--
This message was sent by Atlassian Jira
(v8.20.10#820010)