Fix various Read/WriteConsoleOutput bugs

[lhecker]

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.

[carlos-zamora]

Looks great! Nice work!

[DHowett]

we don't want the API to fail in this case?

[lhecker]

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 STATUS_SUCCESS for targets that are too far to the right/below, MSDN also clearly says:

If the rectangle specified by lpWriteRegion lies completely outside the boundaries of the console screen buffer, [...] no data is written. In this case, the function returns [...] lpWriteRegion parameter set such that [it's empty]

...implying that it should also return STATUS_SUCCESS for targets that are too far to the left or top.

[DHowett]

the actual expected values have changed here. does this have an impact on API consumers?

[lhecker]

To quote the above MSDN link again:

If the rectangle specified by lpWriteRegion lies completely outside the boundaries of the console screen buffer, [...] no data is written. In this case, the function returns [...] such that the Right member is less than the Left, or the Bottom member is less than the Top.

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

	// 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;
	}

[DHowett]

i love that we had Clamp and just... didn't use it.