Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-5716

TMemoryBuffer resizing might shrink the buffer size due to uint32_t overflow

    XMLWordPrintableJSON

Details

    • Bug
    • Status: Closed
    • Critical
    • Resolution: Fixed
    • 0.14.0, 0.15.0, 0.14.1, 0.14.2, 0.16.0, 0.17.0, 0.18.0, 0.18.1
    • 0.19.0
    • C++ - Library
    • None

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

      Attachments

        Issue Links

          Activity

            People

              stigahuang Quanlong Huang
              stigahuang Quanlong Huang
              Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Time Tracking

                  Estimated:
                  Original Estimate - Not Specified
                  Not Specified
                  Remaining:
                  Remaining Estimate - 0h
                  0h
                  Logged:
                  Time Spent - 1h
                  1h