microsoft / microsoft/terminal

ReadConsoleOutputW doesn't work with surrogate pairs (the buffer layers thread)

Open
#10,810 13 comments 0 reactions 1 assignee Claimed by @lhecker View on GitHub
Area-VT Help Wanted Issue-Feature Product-Conhost
Dominant language
C++
Stars
105k
Forks
9.6k
Avg merge
3d 17h
Merged PRs (30d)
29

Description

### Windows Terminal version (or Windows build number)

10.0.19043.1110

### Other Software

_No response_

### Steps to reproduce

Compile and run the following code:
```C++
#include

#include
#include

int main()
{
const auto Out = GetStdHandle(STD_OUTPUT_HANDLE);
const std::wstring Text = L"[👨👩👧👦]";

system("cls");

DWORD n;
WriteConsoleW(Out, Text.data(), Text.size(), &n, {});
CHAR_INFO Buffer[10];

const COORD BufferCoord{ 10, 1 };

SMALL_RECT ReadRegion{ 0, 0, 9, 0 };
ReadConsoleOutputW(Out, Buffer, BufferCoord, {}, &ReadRegion);

SMALL_RECT WriteRegion{ 0, 2, 9, 2 };

WriteConsoleOutputW(Out, Buffer, BufferCoord, {}, &WriteRegion);

int In;
std::cin >> In;
}
```

### Expected Behavior

![image](https://user-images.githubusercontent.com/11453922/127213042-caf3f95d-bd85-444b-9fc9-a3b8e445a092.png)

### Actual Behavior

![image](https://user-images.githubusercontent.com/11453922/127213051-15b1a1ac-6532-4260-8591-c949fa701d6c.png)

### Observations
The problem is here:
https://github.com/microsoft/terminal/blob/3f5f37d910a2c8fde3bb5123c9516f5f9e38af2b/src/host/directio.cpp#L848

AsCharInfo calls Utf16ToUcs2 for the same string over and over:
https://github.com/microsoft/terminal/blob/3f5f37d910a2c8fde3bb5123c9516f5f9e38af2b/src/host/consoleInformation.cpp#L408

which returns UNICODE_REPLACEMENT:
https://github.com/microsoft/terminal/blob/3f5f37d910a2c8fde3bb5123c9516f5f9e38af2b/src/types/convert.cpp#L172

Additionally, AsCharInfo incorrectly sets COMMON_LVB_LEADING_BYTE in this case:
https://github.com/microsoft/terminal/blob/3f5f37d910a2c8fde3bb5123c9516f5f9e38af2b/src/host/consoleInformation.cpp#L415

I assume that the host might use `Attribute::Leading` and `Attribute::Trailing` internally for its own purposes, but `COMMON_LVB_LEADING_BYTE` & `COMMON_LVB_TRAILING_BYTE` have a different purpose, not applicable in this case and shouldn't leak into the public interfaces for surrogates.

### A possible fix
```DIFF
diff --git a/src/host/directio.cpp b/src/host/directio.cpp
index 760a7ecf..4ed1ab0d 100644
--- a/src/host/directio.cpp
+++ b/src/host/directio.cpp
@@ -844,21 +844,34 @@ void EventsToUnicode(_Inout_ std::deque>& inEvents,
// If the point we're trying to write is inside the limited buffer write zone...
if (targetLimit.IsInBounds(targetPos))
{
+ auto charInfo = gci.AsCharInfo(*sourceIter);
+
+ if (sourceIter->Chars().size() > 1)
+ charInfo.Attributes &= ~COMMON_LVB_LEADING_BYTE;
+
// Copy the data into position...
- *targetIter = gci.AsCharInfo(*sourceIter);
- // ... and advance the read iterator.
- sourceIter++;
- }
+ for (const auto i: sourceIter->Chars())
+ {
+ targetIter->Attributes = charInfo.Attributes;
+ targetIter->Char.UnicodeChar = i;

- // Always advance the write iterator, we might have skipped it due to clipping.
- targetIter++;
+ // Always advance the write iterator, we might have skipped it due to clipping.
+ ++targetIter;

- // Increment the target
- targetPos.X++;
- if (targetPos.X >= targetSize.X)
- {
- targetPos.X = 0;
- targetPos.Y++;
+ // Increment the target
+ ++targetPos.X;
+ if (targetPos.X >= targetSize.X)
+ {
+ targetPos.X = 0;
+ ++targetPos.Y;
+ }
+
+ // ... and advance the read iterator.
+ sourceIter++;
+
+ if (!targetLimit.IsInBounds(targetPos))
+ break;
+ }
}
}
```

P.S. I remember that there's #8000 that should improve the situation with surrogates in general. This one is about the API issue: when a surrogate pair occupies two columns, there's really no reason to return two replacement characters if we could return the original pair. The situation with a pair that occupies one column is less obvious, but that's a different issue.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.