From 04e677d7c8173ec0f3b1cc04210ea01d30ec3f40 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 18 Jul 2024 02:26:16 +0200 Subject: [PATCH] Fix various Read/WriteConsoleOutput bugs (#17567) Split off from #17510: * `Viewport::Clamp` used `std::clamp` to calculate the intersection between two rectangles. That works for exclusive rectangles, because `.left == .right` indicates an empty rectangle. But `Viewport` is an inclusive one, and so `.left == .right` is non-empty. For instance, if the to-be-clamped rect is fully outside the bounding rect, the result is a 1x1 viewport. In effect this meant that `Viewport::Clamp` never clamped so far. * The `targetArea < targetBuffer.size()` check is the wrong way around. It should be `targetArea > targetBuffer.size()`. * The `sourceSize` and `targetSize` checks are incorrect, because the rectangles may be non-empty but outside the valid bounding rect. * If these sizes were empty, we'd return the requested rectangle which is a regression since conhost v1 and violates the API contract. * The `sourceRect` emptiness check is incorrect, because the clamping logic before it doesn't actually clamp to the bounding rect. * The entire clamping and iteration logic is just overall too complex. --- src/host/directio.cpp | 217 +++++++++------------------ src/host/directio.h | 12 +- src/host/ft_host/API_OutputTests.cpp | 23 ++- src/host/ut_host/ViewportTests.cpp | 7 +- src/types/viewport.cpp | 9 +- tools/OpenConsole.psm1 | 2 +- 6 files changed, 91 insertions(+), 179 deletions(-) diff --git a/src/host/directio.cpp b/src/host/directio.cpp index 8bc0bf36fd..e9338efd09 100644 --- a/src/host/directio.cpp +++ b/src/host/directio.cpp @@ -463,91 +463,53 @@ CATCH_RETURN(); return result; } -[[nodiscard]] static HRESULT _ReadConsoleOutputWImplHelper(const SCREEN_INFORMATION& context, - std::span targetBuffer, - const Microsoft::Console::Types::Viewport& requestRectangle, - Microsoft::Console::Types::Viewport& readRectangle) noexcept +[[nodiscard]] HRESULT ReadConsoleOutputWImplHelper(const SCREEN_INFORMATION& context, + std::span 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(); + auto& storageBuffer = context.GetActiveBuffer(); + const auto storageRectangle = storageBuffer.GetBufferSize(); + const auto clippedRectangle = storageRectangle.Clamp(requestRectangle); - const auto targetSize = requestRectangle.Dimensions(); - - // 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(); - RETURN_HR_IF(E_INVALIDARG, targetArea < targetBuffer.size()); + const auto bufferStride = gsl::narrow_cast(std::max(0, requestRectangle.Width())); + const auto width = gsl::narrow_cast(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; - // 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()) + if (bufferStride <= 0 || targetBuffer.size() < gsl::narrow_cast(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; - } - - // Always advance the write iterator, we might have skipped it due to clipping. - ++targetIter; - - // Increment the target - targetPos.x++; - if (targetPos.x >= targetSize.width) - { - targetPos.x = 0; - targetPos.y++; - } + return E_INVALIDARG; } - // Reply with the region we read out of the backing buffer (potentially clipped) - readRectangle = clippedRequestRectangle; + for (til::CoordType y = clippedRectangle.Top(); y <= clippedRectangle.BottomInclusive(); y++) + { + auto it = storageBuffer.GetCellDataAt({ clippedRectangle.Left(), y }); + for (size_t i = 0; i < width; i++) + { + targetBuffer[totalOffset + i] = gci.AsCharInfo(*it); + ++it; + } + + 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 buffer, - const Viewport& requestRectangle, - Viewport& writtenRectangle) noexcept +[[nodiscard]] HRESULT WriteConsoleOutputWImplHelper(SCREEN_INFORMATION& context, + std::span 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; - } + 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; - // 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; - } - - 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(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(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; diff --git a/src/host/directio.h b/src/host/directio.h index e8cd977d51..d4864f501f 100644 --- a/src/host/directio.h +++ b/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 targetBuffer, + const Microsoft::Console::Types::Viewport& requestRectangle, + Microsoft::Console::Types::Viewport& readRectangle) noexcept; + +[[nodiscard]] HRESULT WriteConsoleOutputWImplHelper(SCREEN_INFORMATION& context, + std::span buffer, + til::CoordType bufferStride, + const Microsoft::Console::Types::Viewport& requestRectangle, + Microsoft::Console::Types::Viewport& writtenRectangle) noexcept; + [[nodiscard]] NTSTATUS ConsoleCreateScreenBuffer(std::unique_ptr& handle, _In_ PCONSOLE_API_MSG Message, _In_ PCD_CREATE_OBJECT_INFORMATION Information, diff --git a/src/host/ft_host/API_OutputTests.cpp b/src/host/ft_host/API_OutputTests.cpp index baa539c22e..5b8d4cd571 100644 --- a/src/host/ft_host/API_OutputTests.cpp +++ b/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(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() diff --git a/src/host/ut_host/ViewportTests.cpp b/src/host/ut_host/ViewportTests.cpp index 04947a8deb..606279be44 100644 --- a/src/host/ut_host/ViewportTests.cpp +++ b/src/host/ut_host/ViewportTests.cpp @@ -425,12 +425,7 @@ class ViewportTests testView = Viewport::FromInclusive(testRect); Log::Comment(L"We expect it to be pulled back so each coordinate is in bounds, but the rectangle is still invalid (since left will be > right)."); - til::inclusive_rect expected; - expected.top = rect.bottom; - expected.bottom = rect.top; - expected.left = rect.right; - expected.right = rect.left; - const auto expectedView = Viewport::FromInclusive(expected); + const auto expectedView = Viewport::Empty(); actual = view.Clamp(testView); VERIFY_ARE_EQUAL(expectedView, actual, L"Every dimension should be pulled just inside the clamping rectangle."); diff --git a/src/types/viewport.cpp b/src/types/viewport.cpp index 6a64ecf7f6..ad276f8ecb 100644 --- a/src/types/viewport.cpp +++ b/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: diff --git a/tools/OpenConsole.psm1 b/tools/OpenConsole.psm1 index 0f25a8e93a..c3c3e6dbcc 100644 --- a/tools/OpenConsole.psm1 +++ b/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"