Fix various Read/WriteConsoleOutput bugs

Author: lhecker

src/host/directio.cpp

@@ -463,91 +463,53 @@ CATCH_RETURN();
     return result;
 }
 
-[[nodiscard]] static HRESULT _ReadConsoleOutputWImplHelper(const SCREEN_INFORMATION& context,
-                                                           std::span<CHAR_INFO> targetBuffer,
-                                                           const Microsoft::Console::Types::Viewport& requestRectangle,
-                                                           Microsoft::Console::Types::Viewport& readRectangle) noexcept
+[[nodiscard]] HRESULT ReadConsoleOutputWImplHelper(const SCREEN_INFORMATION& context,
+                                                   std::span<CHAR_INFO> targetBuffer,
+                                                   const Viewport& requestRectangle,
+                                                   Viewport& readRectangle) noexcept
 {
     try
     {
         const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-        const auto& storageBuffer = context.GetActiveBuffer().GetTextBuffer();
-        const auto storageSize = storageBuffer.GetSize().Dimensions();
-
-        const auto targetSize = requestRectangle.Dimensions();
+        auto& storageBuffer = context.GetActiveBuffer();
+        const auto storageRectangle = storageBuffer.GetBufferSize();
+        const auto clippedRectangle = storageRectangle.Clamp(requestRectangle);
 
-        // If either dimension of the request is too small, return an empty rectangle as read and exit early.
-        if (targetSize.width <= 0 || targetSize.height <= 0)
+        if (!clippedRectangle.IsValid())
         {
             readRectangle = Viewport::FromDimensions(requestRectangle.Origin(), { 0, 0 });
             return S_OK;
         }
 
-        // The buffer given should be big enough to hold the dimensions of the request.
-        const auto targetArea = targetSize.area<size_t>();
-        RETURN_HR_IF(E_INVALIDARG, targetArea < targetBuffer.size());
-
-        // Clip the request rectangle to the size of the storage buffer
-        auto clip = requestRectangle.ToExclusive();
-        clip.right = std::min(clip.right, storageSize.width);
-        clip.bottom = std::min(clip.bottom, storageSize.height);
-
-        // Find the target point (where to write the user's buffer)
-        // It will either be 0,0 or offset into the buffer by the inverse of the negative values.
-        til::point targetPoint;
-        targetPoint.x = clip.left < 0 ? -clip.left : 0;
-        targetPoint.y = clip.top < 0 ? -clip.top : 0;
-
-        // The clipped rect must be inside the buffer size, so it has a minimum value of 0. (max of itself and 0)
-        clip.left = std::max(clip.left, 0);
-        clip.top = std::max(clip.top, 0);
-
-        // The final "request rectangle" or the area inside the buffer we want to read, is the clipped dimensions.
-        const auto clippedRequestRectangle = Viewport::FromExclusive(clip);
-
-        // We will start reading the buffer at the point of the top left corner (origin) of the (potentially adjusted) request
-        const auto sourcePoint = clippedRequestRectangle.Origin();
-
-        // Get an iterator to the beginning of the return buffer
-        // We might have to seek this forward or skip around if we clipped the request.
-        auto targetIter = targetBuffer.begin();
-        til::point targetPos;
-        const auto targetLimit = Viewport::FromDimensions(targetPoint, clippedRequestRectangle.Dimensions());
-
-        // Get an iterator to the beginning of the request inside the screen buffer
-        // This should walk exactly along every cell of the clipped request.
-        auto sourceIter = storageBuffer.GetCellDataAt(sourcePoint, clippedRequestRectangle);
-
-        // Walk through every cell of the target, advancing the buffer.
-        // Validate that we always still have a valid iterator to the backing store,
-        // that we always are writing inside the user's buffer (before the end)
-        // and we're always targeting the user's buffer inside its original bounds.
-        while (sourceIter && targetIter < targetBuffer.end())
+        const auto bufferStride = gsl::narrow_cast<size_t>(std::max(0, requestRectangle.Width()));
+        const auto width = gsl::narrow_cast<size_t>(clippedRectangle.Width());
+        const auto offsetY = clippedRectangle.Top() - requestRectangle.Top();
+        const auto offsetX = clippedRectangle.Left() - requestRectangle.Left();
+        // We always write the intersection between the valid `storageRectangle` and the given `requestRectangle`.
+        // This means that if the `requestRectangle` is -3 rows above the top of the buffer, we'll start
+        // reading from `buffer` at row offset 3, because the first 3 are outside the valid range.
+        // clippedRectangle.Top/Left() cannot be negative due to the previous Clamp() call.
+        auto totalOffset = offsetY * bufferStride + offsetX;
+
+        if (bufferStride <= 0 || targetBuffer.size() < gsl::narrow_cast<size_t>(clippedRectangle.Height() * bufferStride))
         {
-            // If the point we're trying to write is inside the limited buffer write zone...
-            if (targetLimit.IsInBounds(targetPos))
-            {
-                // Copy the data into position...
-                *targetIter = gci.AsCharInfo(*sourceIter);
-                // ... and advance the read iterator.
-                ++sourceIter;
-            }
+            return E_INVALIDARG;
+        }
 
-            // Always advance the write iterator, we might have skipped it due to clipping.
-            ++targetIter;
+        for (til::CoordType y = clippedRectangle.Top(); y <= clippedRectangle.BottomInclusive(); y++)
+        {
+            auto it = storageBuffer.GetCellDataAt({ clippedRectangle.Left(), y });
 
-            // Increment the target
-            targetPos.x++;
-            if (targetPos.x >= targetSize.width)
+            for (size_t i = 0; i < width; i++)
             {
-                targetPos.x = 0;
-                targetPos.y++;
+                targetBuffer[totalOffset + i] = gci.AsCharInfo(*it);
+                ++it;
             }
-        }
 
-        // Reply with the region we read out of the backing buffer (potentially clipped)
-        readRectangle = clippedRequestRectangle;
+            totalOffset += bufferStride;
+        }
 
+        readRectangle = clippedRectangle;
         return S_OK;
     }
     CATCH_RETURN();
@@ -566,7 +528,7 @@ CATCH_RETURN();
         const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
         const auto codepage = gci.OutputCP;
 
-        RETURN_IF_FAILED(_ReadConsoleOutputWImplHelper(context, buffer, sourceRectangle, readRectangle));
+        RETURN_IF_FAILED(ReadConsoleOutputWImplHelper(context, buffer, sourceRectangle, readRectangle));
 
         LOG_IF_FAILED(_ConvertCellsToAInplace(codepage, buffer, readRectangle));
 
@@ -585,7 +547,7 @@ CATCH_RETURN();
 
     try
     {
-        RETURN_IF_FAILED(_ReadConsoleOutputWImplHelper(context, buffer, sourceRectangle, readRectangle));
+        RETURN_IF_FAILED(ReadConsoleOutputWImplHelper(context, buffer, sourceRectangle, readRectangle));
 
         if (!context.GetActiveBuffer().GetCurrentFont().IsTrueTypeFont())
         {
@@ -599,103 +561,59 @@ CATCH_RETURN();
     CATCH_RETURN();
 }
 
-[[nodiscard]] static HRESULT _WriteConsoleOutputWImplHelper(SCREEN_INFORMATION& context,
-                                                            std::span<CHAR_INFO> buffer,
-                                                            const Viewport& requestRectangle,
-                                                            Viewport& writtenRectangle) noexcept
+[[nodiscard]] HRESULT WriteConsoleOutputWImplHelper(SCREEN_INFORMATION& context,
+                                                    std::span<const CHAR_INFO> buffer,
+                                                    til::CoordType bufferStride,
+                                                    const Viewport& requestRectangle,
+                                                    Viewport& writtenRectangle) noexcept
 {
     try
     {
+        if (bufferStride <= 0)
+        {
+            return E_INVALIDARG;
+        }
+
         auto& storageBuffer = context.GetActiveBuffer();
         const auto storageRectangle = storageBuffer.GetBufferSize();
-        const auto storageSize = storageRectangle.Dimensions();
+        const auto clippedRectangle = storageRectangle.Clamp(requestRectangle);
 
-        const auto sourceSize = requestRectangle.Dimensions();
-
-        // If either dimension of the request is too small, return an empty rectangle as the read and exit early.
-        if (sourceSize.width <= 0 || sourceSize.height <= 0)
+        if (!clippedRectangle.IsValid())
         {
             writtenRectangle = Viewport::FromDimensions(requestRectangle.Origin(), { 0, 0 });
             return S_OK;
         }
 
-        // If the top and left of the destination we're trying to write it outside the buffer,
-        // give the original request rectangle back and exit early OK.
-        if (requestRectangle.Left() >= storageSize.width || requestRectangle.Top() >= storageSize.height)
-        {
-            writtenRectangle = requestRectangle;
-            return S_OK;
-        }
-
-        // Do clipping according to the legacy patterns.
-        auto writeRegion = requestRectangle.ToInclusive();
-        til::inclusive_rect sourceRect;
-        if (writeRegion.right > storageSize.width - 1)
-        {
-            writeRegion.right = storageSize.width - 1;
-        }
-        sourceRect.right = writeRegion.right - writeRegion.left;
-        if (writeRegion.bottom > storageSize.height - 1)
-        {
-            writeRegion.bottom = storageSize.height - 1;
-        }
-        sourceRect.bottom = writeRegion.bottom - writeRegion.top;
-
-        if (writeRegion.left < 0)
-        {
-            sourceRect.left = -writeRegion.left;
-            writeRegion.left = 0;
-        }
-        else
-        {
-            sourceRect.left = 0;
-        }
+        const auto width = clippedRectangle.Width();
+        // We always write the intersection between the valid `storageRectangle` and the given `requestRectangle`.
+        // This means that if the `requestRectangle` is -3 rows above the top of the buffer, we'll start
+        // reading from `buffer` at row offset 3, because the first 3 are outside the valid range.
+        // clippedRectangle.Top/Left() cannot be negative due to the previous Clamp() call.
+        const auto offsetY = clippedRectangle.Top() - requestRectangle.Top();
+        const auto offsetX = clippedRectangle.Left() - requestRectangle.Left();
+        auto totalOffset = offsetY * bufferStride + offsetX;
 
-        if (writeRegion.top < 0)
-        {
-            sourceRect.top = -writeRegion.top;
-            writeRegion.top = 0;
-        }
-        else
-        {
-            sourceRect.top = 0;
-        }
-
-        if (sourceRect.left > sourceRect.right || sourceRect.top > sourceRect.bottom)
+        if (bufferStride <= 0 || buffer.size() < gsl::narrow_cast<size_t>(clippedRectangle.Height() * bufferStride))
         {
             return E_INVALIDARG;
         }
 
-        const auto writeRectangle = Viewport::FromInclusive(writeRegion);
-
-        auto target = writeRectangle.Origin();
-
-        // For every row in the request, create a view into the clamped portion of just the one line to write.
-        // This allows us to restrict the width of the call without allocating/copying any memory by just making
-        // a smaller view over the existing big blob of data from the original call.
-        for (; target.y < writeRectangle.BottomExclusive(); target.y++)
+        for (til::CoordType y = clippedRectangle.Top(); y <= clippedRectangle.BottomInclusive(); y++)
         {
-            // We find the offset into the original buffer by the dimensions of the original request rectangle.
-            const auto rowOffset = (target.y - requestRectangle.Top()) * requestRectangle.Width();
-            const auto colOffset = target.x - requestRectangle.Left();
-            const auto totalOffset = rowOffset + colOffset;
-
-            // Now we make a subspan starting from that offset for as much of the original request as would fit
-            const auto subspan = buffer.subspan(totalOffset, writeRectangle.Width());
-
-            // Convert to a CHAR_INFO view to fit into the iterator
-            const auto charInfos = std::span<const CHAR_INFO>(subspan.data(), subspan.size());
+            const auto charInfos = buffer.subspan(totalOffset, width);
+            const til::point target{ clippedRectangle.Left(), y };
 
             // Make the iterator and write to the target position.
-            OutputCellIterator it(charInfos);
-            storageBuffer.Write(it, target);
+            storageBuffer.Write(OutputCellIterator(charInfos), target);
+
+            totalOffset += bufferStride;
         }
 
         // If we've overwritten image content, it needs to be erased.
-        ImageSlice::EraseBlock(storageBuffer.GetTextBuffer(), writeRectangle.ToExclusive());
+        ImageSlice::EraseBlock(storageBuffer.GetTextBuffer(), clippedRectangle.ToExclusive());
 
         // Since we've managed to write part of the request, return the clamped part that we actually used.
-        writtenRectangle = writeRectangle;
+        writtenRectangle = clippedRectangle;
 
         return S_OK;
     }
@@ -712,11 +630,12 @@ CATCH_RETURN();
 
     try
     {
-        const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+        auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+
         const auto codepage = gci.OutputCP;
         LOG_IF_FAILED(_ConvertCellsToWInplace(codepage, buffer, requestRectangle));
 
-        RETURN_IF_FAILED(_WriteConsoleOutputWImplHelper(context, buffer, requestRectangle, writtenRectangle));
+        RETURN_IF_FAILED(WriteConsoleOutputWImplHelper(context, buffer, requestRectangle.Width(), requestRectangle, writtenRectangle));
 
         return S_OK;
     }
@@ -738,11 +657,11 @@ CATCH_RETURN();
             // For compatibility reasons, we must maintain the behavior that munges the data if we are writing while a raster font is enabled.
             // This can be removed when raster font support is removed.
             auto translated = _ConvertCellsToMungedW(buffer, requestRectangle);
-            RETURN_IF_FAILED(_WriteConsoleOutputWImplHelper(context, translated, requestRectangle, writtenRectangle));
+            RETURN_IF_FAILED(WriteConsoleOutputWImplHelper(context, translated, requestRectangle.Width(), requestRectangle, writtenRectangle));
         }
         else
         {
-            RETURN_IF_FAILED(_WriteConsoleOutputWImplHelper(context, buffer, requestRectangle, writtenRectangle));
+            RETURN_IF_FAILED(WriteConsoleOutputWImplHelper(context, buffer, requestRectangle.Width(), requestRectangle, writtenRectangle));
         }
 
         return S_OK;

src/host/directio.h

@@ -17,10 +17,20 @@ Revision History:
 #pragma once
 
 #include "conapi.h"
-#include "inputBuffer.hpp"
 
 class SCREEN_INFORMATION;
 
+[[nodiscard]] HRESULT ReadConsoleOutputWImplHelper(const SCREEN_INFORMATION& context,
+                                                   std::span<CHAR_INFO> targetBuffer,
+                                                   const Microsoft::Console::Types::Viewport& requestRectangle,
+                                                   Microsoft::Console::Types::Viewport& readRectangle) noexcept;
+
+[[nodiscard]] HRESULT WriteConsoleOutputWImplHelper(SCREEN_INFORMATION& context,
+                                                    std::span<const CHAR_INFO> buffer,
+                                                    til::CoordType bufferStride,
+                                                    const Microsoft::Console::Types::Viewport& requestRectangle,
+                                                    Microsoft::Console::Types::Viewport& writtenRectangle) noexcept;
+
 [[nodiscard]] NTSTATUS ConsoleCreateScreenBuffer(std::unique_ptr<ConsoleHandleData>& handle,
                                                  _In_ PCONSOLE_API_MSG Message,
                                                  _In_ PCD_CREATE_OBJECT_INFORMATION Information,

src/host/ft_host/API_OutputTests.cpp

@@ -159,15 +159,16 @@ void OutputTests::WriteConsoleOutputWOutsideBuffer()
     // Call the API and confirm results.
 
     //  move outside in X and Y directions
-    auto shiftedRegion = region;
-    shiftedRegion.Left += bufferSize.X;
-    shiftedRegion.Right += bufferSize.X;
-    shiftedRegion.Top += bufferSize.Y;
-    shiftedRegion.Bottom += bufferSize.Y;
-
-    auto affected = shiftedRegion;
+    auto affected = region;
+    affected.Left += bufferSize.X;
+    affected.Right += bufferSize.X;
+    affected.Top += bufferSize.Y;
+    affected.Bottom += bufferSize.Y;
+    auto expected = affected;
+    expected.Right = expected.Left - 1;
+    expected.Bottom = expected.Top - 1;
     VERIFY_WIN32_BOOL_SUCCEEDED(WriteConsoleOutputW(consoleOutputHandle, buffer.data(), regionDimensions, regionOrigin, &affected));
-    VERIFY_ARE_EQUAL(shiftedRegion, affected);
+    VERIFY_ARE_EQUAL(expected, affected);
 
     // Read the entire buffer back and validate that we didn't write anything anywhere
     const auto readBack = std::make_unique<CHAR_INFO[]>(sbiex.dwSize.X * sbiex.dwSize.Y);
@@ -363,12 +364,6 @@ void OutputTests::WriteConsoleOutputWNegativePositions()
             VERIFY_ARE_EQUAL(expectedItem, readItem);
         }
     }
-
-    // Set the region so the left will end up past the right
-    adjustedRegion = region;
-    adjustedRegion.Left = -(adjustedRegion.Right + 1);
-    affected = adjustedRegion;
-    VERIFY_WIN32_BOOL_FAILED(WriteConsoleOutputW(consoleOutputHandle, buffer.data(), regionDimensions, regionOrigin, &affected));
 }
 
 void OutputTests::WriteConsoleOutputCharacterWRunoff()

src/types/viewport.cpp

@@ -198,14 +198,7 @@ void Viewport::Clamp(til::point& pos) const
 // - Clamped viewport
 Viewport Viewport::Clamp(const Viewport& other) const noexcept
 {
-    auto clampMe = other.ToInclusive();
-
-    clampMe.left = std::clamp(clampMe.left, Left(), RightInclusive());
-    clampMe.right = std::clamp(clampMe.right, Left(), RightInclusive());
-    clampMe.top = std::clamp(clampMe.top, Top(), BottomInclusive());
-    clampMe.bottom = std::clamp(clampMe.bottom, Top(), BottomInclusive());
-
-    return Viewport::FromInclusive(clampMe);
+    return Viewport::FromExclusive(ToExclusive() & other.ToExclusive());
 }
 
 // Method Description:

tools/OpenConsole.psm1

@@ -197,7 +197,7 @@ function Invoke-OpenConsoleTests()
     {
         $OpenConsolePlatform = 'Win32'
     }
-    $OpenConsolePath = "$env:OpenConsoleroot\bin\$OpenConsolePlatform\$Configuration\OpenConsole.exe"
+    $OpenConsolePath = "$root\bin\$OpenConsolePlatform\$Configuration\OpenConsole.exe"
     $TaefExePath = "$root\packages\Microsoft.Taef.10.60.210621002\build\Binaries\$Platform\te.exe"
     $BinDir = "$root\bin\$OpenConsolePlatform\$Configuration"