Fix various Read/WriteConsoleOutput bugs #17567
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -159,15 +159,16 @@ void OutputTests::WriteConsoleOutputWOutsideBuffer() | |
| // Call the API and confirm results. | ||
|
|
||
| // move outside in X and Y directions | ||
| 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(expected, affected); | ||
|
Member
There was a problem hiding this comment. the actual expected values have changed here. does this have an impact on API consumers?
Member
Author
There was a problem hiding this comment. To quote the above MSDN link again:
Meanwhile this test asserted that we return the rectangle completely unmodified. It's caused by this branch: terminal/src/host/directio.cpp Lines 622 to 628 in ec92892 |
||
|
|
||
| // 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); | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. we don't want the API to fail in this case?
Member
Author
There was a problem hiding this comment. Yes, it should succeed, it makes absolutely no sense otherwise. From what I can tell by reading just the source code, this was a regression introduced in Windows Vista, most likely as a crash fix for conhost. However, the person who wrote the crash fix didn't seem to understand how the code worked. In Windows 2000, where this code was first added, we had something like this (pseudo-code, comments are mine): NTSTATUS WriteScreenBuffer(...) {
// Source buffer empty?
if (SourceSize.X <= 0 || SourceSize.Y <= 0) {
return STATUS_SUCCESS;
}
// Target area is outside the buffer...
// ...but it only checks whether it's too far right / below??
if (psrWriteRegion->Left >= BufferSize.X || psrWriteRegion->Top >= BufferSize.Y) {
return STATUS_SUCCESS;
}
// ... clamping ...
return WriteRectToScreenBuffer(...);
}Then in Windows Vista it looks like this: NTSTATUS WriteScreenBuffer(...) {
// Source buffer empty?
if (SourceSize.X <= 0 || SourceSize.Y <= 0) {
return STATUS_SUCCESS;
}
// Target area is outside the buffer...
// ...but it only checks whether it's too far right / below??
if (psrWriteRegion->Left >= BufferSize.X || psrWriteRegion->Top >= BufferSize.Y) {
return STATUS_SUCCESS;
}
// ... clamping ...
// Clamped rect is empty...
// ...but the check is completely wrong??
// The clamping logic assumes that the target rect is in bounds!
if (clamped rect is empty) {
return STATUS_INVALID_PARAMETER;
}
return WriteRectToScreenBuffer(...);
}What the person who changed this code failed to understand is that the issue isn't a missing clamp-rect check, the issue is that the 2nd if condition is just plain wrong. The clamping logic then relies on the incorrect assumption that the if condition is correct which results in a completely incorrect clamped region (the clamped region can be outside the buffer to the left or above with negative coordinates among others). In addition to the 2nd if condition returning
...implying that it should also return |
||
| } | ||
|
|
||
| void OutputTests::WriteConsoleOutputCharacterWRunoff() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i love that we had Clamp and just... didn't use it.