Skip to content

Commit

Permalink
Fix various Read/WriteConsoleOutput bugs (#17567)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@lhecker
lhecker authored Jul 18, 2024
1 parent 1999366 commit 04e677d
Show file tree
Hide file tree
Showing 6 changed files with 89 additions and 177 deletions.
213 changes: 66 additions & 147 deletions src/host/directio.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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));

Expand All @@ -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())
{
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
Expand Down
12 changes: 11 additions & 1 deletion src/host/directio.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
23 changes: 9 additions & 14 deletions src/host/ft_host/API_OutputTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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()
Expand Down
7 changes: 1 addition & 6 deletions src/host/ut_host/ViewportTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
Expand Down
9 changes: 1 addition & 8 deletions src/types/viewport.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Loading

0 comments on commit 04e677d

Please sign in to comment.