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

[Quanlong Huang (Jira)]

Quanlong Huang created THRIFT-5716:
--------------------------------------

             Summary: 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
            Reporter: Quanlong Huang
            Assignee: Quanlong Huang


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)