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

• Type: Bug
• Status: Closed
• Priority: Critical
• Resolution: Fixed
• Affects Version/s: 0.14.0, 0.15.0, 0.14.1, 0.14.2, 0.16.0, 0.17.0, 0.18.0, 0.18.1
• Fix Version/s: 0.19.0
• Component/s: C++ - Library
• Assignee: Quanlong Huang
• Reporter: Quanlong Huang
• Created: 16/Jun/23 12:50
• Updated: 02/Sep/23 11:53
• Resolved: 22/Jun/23 06:53

Description

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.

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);
} 

Overflow example:

len=1066587646, current_used=4167372953, required_buffer_size=938993303

The buffer is not expanded if 'required_buffer_size' overflow. TMemoryBuffer::writeSlow() would finally writes to invalid address and crash the process:

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;
} 

Issue Links

• causes: IMPALA-12223 Coordinator crash in serializing huge profile
• is caused by: THRIFT-5114 Simplify the computation of the size of TMemoryBuffer
• relates to: THRIFT-3821 TMemoryBuffer buffer may overflow when resizing
• links to: GitHub Pull Request #2818