Repository: microsoft/terminal (language: cpp)

Issue:
TextBuffer::ResizeTraditional may leak stale pointers
The catch clause will allow `BufferAllocator` to be destroyed while the `ROW`s in `_storage` continue to reference its memory. The solution is to refactor `ROW::Resize` from being a mutating function over to allocating a new `_storage` vector and copying all rows over. That way the `TextBuffer` is only mutated once at the end, long after all throwing code has finished executing.


Discussion / hints:

Fix patch:
diff --git a/src/buffer/out/Row.cpp b/src/buffer/out/Row.cpp
index 38a82857cf2..9d883b09502 100644
--- a/src/buffer/out/Row.cpp
+++ b/src/buffer/out/Row.cpp
@@ -88,19 +88,6 @@ ROW::ROW(wchar_t* charsBuffer, uint16_t* charOffsetsBuffer, uint16_t rowWidth, c
     }
 }
 
-void swap(ROW& lhs, ROW& rhs) noexcept
-{
-    std::swap(lhs._charsBuffer, rhs._charsBuffer);
-    std::swap(lhs._charsHeap, rhs._charsHeap);
-    std::swap(lhs._chars, rhs._chars);
-    std::swap(lhs._charOffsets, rhs._charOffsets);
-    std::swap(lhs._attr, rhs._attr);
-    std::swap(lhs._columnCount, rhs._columnCount);
-    std::swap(lhs._lineRendition, rhs._lineRendition);
-    std::swap(lhs._wrapForced, rhs._wrapForced);
-    std::swap(lhs._doubleBytePadded, rhs._doubleBytePadded);
-}
-
 void ROW::SetWrapForced(const bool wrap) noexcept
 {
     _wrapForced = wrap;
@@ -154,78 +141,6 @@ void ROW::_init() noexcept
     std::iota(_charOffsets.begin(), _charOffsets.end(), uint16_t{ 0 });
 }
 
-// Routine Description:
-// - resizes ROW to new width
-// Arguments:
-// - charsBuffer - a new backing buffer to use for _charsBuffer
-// - charOffsetsBuffer - a new backing buffer to use for _charOffsets
-// - rowWidth - the new width, in cells
-// - fillAttribute - the attribute to use for any newly added, trailing cells
-void ROW::Resize(wchar_t* charsBuffer, uint16_t* charOffsetsBuffer, uint16_t rowWidth, const TextAttribute& fillAttribute)
-{
-    // A default-constructed ROW has no cols/chars to copy.
-    // It can be detected by the lack of a _charsBuffer (among others).
-    //
-    // Otherwise, this block figures out how much we can copy into the new `rowWidth`.
-    uint16_t colsToCopy = 0;
-    uint16_t charsToCopy = 0;
-    if (_charsBuffer)
-    {
-        colsToCopy = std::min(rowWidth, _columnCount);
-        // Safety: colsToCopy is [0, _columnCount].
-        charsToCopy = _uncheckedCharOffset(colsToCopy);
-        // Safety: colsToCopy is [0, _columnCount] due to colsToCopy != 0.
-        for (; colsToCopy != 0 && _uncheckedIsTrailer(colsToCopy); --colsToCopy)
-        {
-        }
-    }
-
-    // If we grow the row width, we have to append a bunch of whitespace.
-    // `trailingWhitespace` stores that amount.
-    // Safety: The preceding block left colsToCopy in the range [0, rowWidth].
-    const uint16_t trailingWhitespace = rowWidth - colsToCopy;
-
-    // Allocate memory for the new `_chars` array.
-    // Use the provided charsBuffer if possible, otherwise allocate a `_charsHeap`.
-    std::unique_ptr<wchar_t[]> charsHeap;
-    std::span chars{ charsBuffer, rowWidth };
-    const std::span charOffsets{ charOffsetsBuffer, ::base::strict_cast<size_t>(rowWidth) + 1u };
-    if (const uint16_t charsCapacity = charsToCopy + trailingWhitespace; charsCapacity > rowWidth)
-    {
-        charsHeap = std::make_unique_for_overwrite<wchar_t[]>(charsCapacity);
-        chars = { charsHeap.get(), charsCapacity };
-    }
-
-    // Copy chars and charOffsets over.
-    {
-        const auto it = std::copy_n(_chars.begin(), charsToCopy, chars.begin());
-        std::fill_n(it, trailingWhitespace, L' ');
-    }
-    {
-        const auto it = std::copy_n(_charOffsets.begin(), colsToCopy, charOffsets.begin());
-        // The _charOffsets array is 1 wider than newWidth indicates.
-        // This is because the extra column contains the past-the-end index into _chars.
-        iota_n(it, trailingWhitespace + 1u, charsToCopy);
-    }
-
-    _charsBuffer = charsBuffer;
-    _charsHeap = std::move(charsHeap);
-    _chars = chars;
-    _charOffsets = charOffsets;
-    _columnCount = rowWidth;
-
-    // .resize_trailing_extent() doesn't work if the vector is empty,
-    // since there's no trailing item that could be extended.
-    if (_attr.empty())
-    {
-        _attr = { rowWidth, fillAttribute };
-    }
-    else
-    {
-        _attr.resize_trailing_extent(rowWidth);
-    }
-}
-
 void ROW::TransferAttributes(const til::small_rle<TextAttribute, uint16_t, 1>& attr, til::CoordType newWidth)
 {
     _attr = attr;
diff --git a/src/buffer/out/Row.hpp b/src/buffer/out/Row.hpp
index 1e14c382fc5..c86312984e9 100644
--- a/src/buffer/out/Row.hpp
+++ b/src/buffer/out/Row.hpp
@@ -66,11 +66,9 @@ class ROW final
     ROW(const ROW& other) = delete;
     ROW& operator=(const ROW& other) = delete;
 
-    explicit ROW(ROW&& other) = default;
+    ROW(ROW&& other) = default;
     ROW& operator=(ROW&& other) = default;
 
-    friend void swap(ROW& lhs, ROW& rhs) noexcept;
-
     void SetWrapForced(const bool wrap) noexcept;
     bool WasWrapForced() const noexcept;
     void SetDoubleBytePadded(const bool doubleBytePadded) noexcept;
@@ -79,7 +77,6 @@ class ROW final
     LineRendition GetLineRendition() const noexcept;
 
     void Reset(const TextAttribute& attr);
-    void Resize(wchar_t* charsBuffer, uint16_t* charOffsetsBuffer, uint16_t rowWidth, const TextAttribute& fillAttribute);
     void TransferAttributes(const til::small_rle<TextAttribute, uint16_t, 1>& attr, til::CoordType newWidth);
 
     til::CoordType NavigateToPrevious(til::CoordType column) const noexcept;
diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp
index 8b8f9d1f79a..9e969c6ed24 100644
--- a/src/buffer/out/textBuffer.cpp
+++ b/src/buffer/out/textBuffer.cpp
@@ -13,75 +13,6 @@
 #include "../types/inc/convert.hpp"
 #include "../../types/inc/GlyphWidth.hpp"
 
-namespace
-{
-    struct BufferAllocator
-    {
-        BufferAllocator(til::size sz)
-        {
-            const auto w = gsl::narrow<uint16_t>(sz.width);
-            const auto h = gsl::narrow<uint16_t>(sz.height);
-
-            const auto charsBytes = w * sizeof(wchar_t);
-            // The ROW::_indices array stores 1 more item than the buffer is wide.
-            // That extra column stores the past-the-end _chars pointer.
-            const auto indicesBytes = w * sizeof(uint16_t) + sizeof(uint16_t);
-            const auto rowStride = charsBytes + indicesBytes;
-            // 65535*65535 cells would result in a charsAreaSize of 8GiB.
-            // --> Use uint64_t so that we can safely do our calculations even on x86.
-            const auto allocSize = gsl::narrow<size_t>(::base::strict_cast<uint64_t>(rowStride) * ::base::strict_cast<uint64_t>(h));
-
-            _buffer = wil::unique_virtualalloc_ptr<std::byte>{ static_cast<std::byte*>(VirtualAlloc(nullptr, allocSize, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE)) };
-            THROW_IF_NULL_ALLOC(_buffer);
-
-            _data = std::span{ _buffer.get(), allocSize }.begin();
-            _rowStride = rowStride;
-            _indicesOffset = charsBytes;
-            _width = w;
-            _height = h;
-        }
-
-        BufferAllocator& operator++() noexcept
-        {
-            _data += _rowStride;
-            return *this;
-        }
-
-        wchar_t* chars() const noexcept
-        {
-            return til::bit_cast<wchar_t*>(&*_data);
-        }
-
-        uint16_t* indices() const noexcept
-        {
-            return til::bit_cast<uint16_t*>(&*(_data + _indicesOffset));
-        }
-
-        uint16_t width() const noexcept
-        {
-            return _width;
-        }
-
-        uint16_t height() const noexcept
-        {
-            return _height;
-        }
-
-        wil::unique_virtualalloc_ptr<std::byte>&& take() noexcept
-        {
-            return std::move(_buffer);
-        }
-
-    private:
-        wil::unique_virtualalloc_ptr<std::byte> _buffer;
-        std::span<std::byte>::iterator _data;
-        size_t _rowStride;
-        size_t _indicesOffset;
-        uint16_t _width;
-        uint16_t _height;
-    };
-}
-
 using namespace Microsoft::Console;
 using namespace Microsoft::Console::Types;
 
@@ -111,16 +42,7 @@ TextBuffer::TextBuffer(til::size screenBufferSize,
     // Guard against resizing the text buffer to 0 columns/rows, which would break being able to insert text.
     screenBufferSize.width = std::max(screenBufferSize.width, 1);
     screenBufferSize.height = std::max(screenBufferSize.height, 1);
-
-    BufferAllocator allocator{ screenBufferSize };
-
-    _storage.reserve(allocator.height());
-    for (til::CoordType i = 0; i < screenBufferSize.height; ++i, ++allocator)
-    {
-        _storage.emplace_back(allocator.chars(), allocator.indices(), allocator.width(), _currentAttributes);
-    }
-
-    _charBuffer = allocator.take();
+    _charBuffer = _allocateBuffer(screenBufferSize, _currentAttributes, _storage);
     _UpdateSize();
 }
 
@@ -775,6 +697,37 @@ const Viewport TextBuffer::GetSize() const noexcept
     return _size;
 }
 
+wil::unique_virtualalloc_ptr<std::byte> TextBuffer::_allocateBuffer(til::size sz, const TextAttribute& attributes, std::vector<ROW>& rows)
+{
+    const auto w = gsl::narrow<uint16_t>(sz.width);
+    const auto h = gsl::narrow<uint16_t>(sz.height);
+
+    const auto charsBytes = w * sizeof(wchar_t);
+    // The ROW::_indices array stores 1 more item than the buffer is wide.
+    // That extra column stores the past-the-end _chars pointer.
+    const auto indicesBytes = w * sizeof(uint16_t) + sizeof(uint16_t);
+    const auto rowStride = charsBytes + indicesBytes;
+    // 65535*65535 cells would result in a charsAreaSize of 8GiB.
+    // --> Use uint64_t so that we can safely do our calculations even on x86.
+    const auto allocSize = gsl::narrow<size_t>(::base::strict_cast<uint64_t>(rowStride) * ::base::strict_cast<uint64_t>(h));
+
+    auto buffer = wil::unique_virtualalloc_ptr<std::byte>{ static_cast<std::byte*>(VirtualAlloc(nullptr, allocSize, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE)) };
+    THROW_IF_NULL_ALLOC(buffer);
+
+    auto data = std::span{ buffer.get(), allocSize }.begin();
+
+    rows.resize(h);
+    for (auto& row : rows)
+    {
+        const auto chars = til::bit_cast<wchar_t*>(&*data);
+        const auto indices = til::bit_cast<uint16_t*>(&*(data + charsBytes));
+        row = { chars, indices, w, attributes };
+        data += rowStride;
+    }
+
+    return buffer;
+}
+
 void TextBuffer::_UpdateSize()
 {
     _size = Viewport::FromDimensions({ _storage.at(0).size(), gsl::narrow<til::CoordType>(_storage.size()) });
@@ -1001,37 +954,55 @@ void TextBuffer::Reset()
 
     try
     {
-        BufferAllocator allocator{ newSize };
-
-        const auto currentSize = GetSize().Dimensions();
-        const auto attributes = GetCurrentAttributes();
-
         til::CoordType TopRow = 0; // new top row of the screen buffer
         if (newSize.height <= GetCursor().GetPosition().y)
         {
             TopRow = GetCursor().GetPosition().y - newSize.height + 1;
         }
-        const auto TopRowIndex = (GetFirstRowIndex() + TopRow) % currentSize.height;
+        const auto TopRowIndex = gsl::narrow_cast<size_t>(_firstRow + TopRow) % _storage.size();
 
-        // rotate rows until the top row is at index 0
-        std::rotate(_storage.begin(), _storage.begin() + TopRowIndex, _storage.end());
-        _SetFirstRowIndex(0);
-
-        // realloc in the Y direction
-        // remove rows if we're shrinking
-        _storage.resize(allocator.height());
+        std::vector<ROW> newStorage;
+        auto newBuffer = _allocateBuffer(newSize, _currentAttributes, newStorage);
 
-        // realloc in the X direction
-        for (auto& it : _storage)
+        // This basically imitates a std::rotate_copy(first, mid, last), but uses ROW::CopyRangeFrom() to do the copying.
         {
-            it.Resize(allocator.chars(), allocator.indices(), allocator.width(), attributes);
-            ++allocator;
+            const auto first = _storage.begin();
+            const auto last = _storage.end();
+            const auto mid = first + TopRowIndex;
+            auto dest = newStorage.begin();
+
+            std::span<ROW> sourceRanges[]{
+                { mid, last },
+                { first, mid },
+            };
+
+            // Ensure we don't copy more from `_storage` than fit into `newStorage`.
+            if (sourceRanges[0].size() > newStorage.size())
+            {
+                sourceRanges[0] = sourceRanges[0].subspan(0, newStorage.size());
+            }
+            if (const auto remaining = newStorage.size() - sourceRanges[0].size(); sourceRanges[1].size() > remaining)
+            {
+                sourceRanges[1] = sourceRanges[1].subspan(0, remaining);
+            }
+
+            for (const auto& sourceRange : sourceRanges)
+            {
+                for (const auto& oldRow : sourceRange)
+                {
+                    til::CoordType begin = 0;
+                    dest->CopyRangeFrom(0, til::CoordTypeMax, oldRow, begin, til::CoordTypeMax);
+                    dest->TransferAttributes(oldRow.Attributes(), newSize.width);
+                    ++dest;
+                }
+            }
         }
 
-        // Update the cached size value
-        _UpdateSize();
+        _charBuffer = std::move(newBuffer);
+        _storage = std::move(newStorage);
 
-        _charBuffer = allocator.take();
+        _SetFirstRowIndex(0);
+        _UpdateSize();
     }
     CATCH_RETURN();
 
diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp
index fe27d4ef007..555531a25aa 100644
--- a/src/buffer/out/textBuffer.hpp
+++ b/src/buffer/out/textBuffer.hpp
@@ -219,6 +219,8 @@ class TextBuffer final
     interval_tree::IntervalTree<til::point, size_t> GetPatterns(const til::CoordType firstRow, const til::CoordType lastRow) const;
 
 private:
+    static wil::unique_virtualalloc_ptr<std::byte> _allocateBuffer(til::size sz, const TextAttribute& attributes, std::vector<ROW>& rows);
+
     void _UpdateSize();
     void _SetFirstRowIndex(const til::CoordType FirstRowIndex) noexcept;
     til::point _GetPreviousFromCursor() const noexcept;


Test patch: