[ https://issues.apache.org/jira/browse/THRIFT-5716?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Quanlong Huang resolved THRIFT-5716. ------------------------------------ Fix Version/s: 0.19.0 Resolution: Fixed > 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 > 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)