Repository: microsoft/terminal (language: cpp)

Issue:
Regression vs legacy conhost: different behavior from ReadConsoleOutputCharacterW when surrogate pair(s) are present.
### Windows Terminal version

1.19.10573.0

### Windows build number

10.0.19045.4046

### Other Software

This is an API issue, and affects any software that calls `ReadConsoleOutputCharacterW`.
The API issue only repros with Windows Terminal, not with legacy conhost (nor with ConEmu, ConsoleZ, etc).

For example [clink](https://github.com/chrisant996/clink) was affected by this when also using [eza](https://github.com/eza-community/eza) or [dirx](https://github.com/chrisant996/dirx) (see [here](https://github.com/chrisant996/clink/issues/574#issuecomment-1996293488) for details of how this was encountered during real world usage).

### Steps to reproduce

When a line of text in the console screen buffer contains one or more surrogate pairs, then the behavior `ReadConsoleOutputCharacterW` API does not match the documented contract, and is different from the behavior with legacy conhost.

The attached repro program demonstrates the behavior:
- It works as expected in legacy conhost (and ConEmu, ConsoleZ, etc).
- It malfunctions in Windows Terminal.

[repro_surrogate_pairs_issue.zip](https://github.com/microsoft/terminal/files/14640769/repro_surrogate_pairs_issue.zip)

Repro:
- Use `WriteConsoleW` to print a line of text that includes one or more surrogate pairs.
- Use `ReadConsoleOutputCharacterW` to read back the same line.

### Easy demonstration program:
1. Create a folder and extract the files from the .zip file linked above.
2. Optional: run the signed Repro.exe file.
3. Or, you can build the source files and run a locally built copy of the Repro.exe program.

### Expected Behavior

`ReadConsoleOutputCharacterW` should:
1. Fill the out param `lpNumberOfCharsRead` with the number of characters read (e.g. the width of the console).
2. Fill the out param `lpCharacter` with characters read from the console.
3. Return true (success).

The demo program should first write 4 lines, then verify that reading the lines back matches what was originally written.
Each line contains Unicode codepoints that correspond to certain [nerdfonts](https://nerdfonts.com) icons.

### Sample expected output:

```plain
OUTPUT:

-a--- 17k 13 Mar 14:38  Aaa.cpp
-a--- 11k 13 Mar 14:44 󱗀 Bbb.xml
-a--- 10k 13 Mar 14:43 󰅲 Ccc.lisp
-a--- 11k 13 Mar 14:44  Ddd.zip

RESULTS:

Line 1 len 120 matches : -a--- 17k 13 Mar 14:38  Aaa.cpp
Line 2 len 120 matches : -a--- 11k 13 Mar 14:44 󱗀 Bbb.xml
Line 3 len 120 matches : -a--- 10k 13 Mar 14:43 󰅲 Ccc.lisp
Line 4 len 120 matches : -a--- 11k 13 Mar 14:44  Ddd.zip
```

### Actual Behavior

Only in Windows Terminal (all versions; 1.19, 1.20, 1.20 canary):
1. Fills the out param `lpNumberOfCharsRead` with 0.
2. Does not fill the out param `lpCharacter`.
3. Returns true (success).

**Problem 1:**  It reads nothing; it should read text successfully, the same as in legacy conhost.
**Problem 2:**  It reports success; that's inaccurate since it failed to read the text that was present.

### Sample actual output:

```plain
OUTPUT:

-a--- 17k 13 Mar 14:38  Aaa.cpp
-a--- 11k 13 Mar 14:44 󱗀 Bbb.xml
-a--- 10k 13 Mar 14:43 󰅲 Ccc.lisp
-a--- 11k 13 Mar 14:44  Ddd.zip

RESULTS:

Line 1 len 120 matches : -a--- 17k 13 Mar 14:38  Aaa.cpp
Line 2 len 0   DIFFERS :
Line 3 len 0   DIFFERS :
Line 4 len 120 matches : -a--- 11k 13 Mar 14:44  Ddd.zip
```


Discussion / hints:
I'm thinking this is in the neighborhood of #13339
I could potentially understand cutting the ability to read such lines. Although Windows has always been able to access the console buffer, *nix has never been able to.

At a minimum, though, the API should return failure (false) when it fails.  The main problem in the case at hand wasn't even the failure to read the console text; the main issue was the API reported that it _succeeded_ but it had actually failed.

Returning failure would have avoided the real world usage problem linked above, since the code was already actively checking for failures.  Ideally the API would succeed and behave how it did in legacy conhost -- but if it won't, then returning that there was a failure lets the app handle fallback appropriately.
The regression is not intentional and unrelated to the intentional ones in #13339, but it is in the neighborhood. I think it's caused by #13626 which added proper support for surrogate pairs but changed the behavior of `TextBufferCellIterator`. It now iterates in columns instead of characters and this breaks single-cell surrogate pairs. The fact that this ever worked is mostly a coincidence because the iterator code is based on UCS2 and not UTF16, but still somewhat dealt with surrogate pairs (albeit poorly).

This internal regression causes the buffer to be filled with (for instance) 121 chars in a 120 column terminal, but you're only giving a 120 char buffer in your repro. The following code then drops the returned text and still returns `S_OK`. It has existed long before my changes to support surrogate pairs:
https://github.com/microsoft/terminal/blob/ba34abb7b3cdc00c943ea4d033e7310e11087bd2/src/host/directio.cpp#L816-L821

I've had a brief glance at the original conhost v1 code and it doesn't seem like this behavior existed there, so I believe this is another regression that was introduced in the original conhost v2 release.

I'm submitting a PR to fix this issue soon. It'll then correctly count the number of characters again.
@lhecker ah ok that makes sense.

Thank you for working on a fix quickly!

<br/>
<br/>
<br/>

It sounds like you're aware of everything I'm about to add, but for completeness, I'll still call these out overtly:

The trouble with 120 vs 121, is that the API signature uses the `nLength` param to mean two different things:
1. The size of the buffer to receive the read content.
2. The number of cells to read.

So, if the screen width is 120 and a caller passes 121, then on a line with no surrogate pairs the 121st character will be read from the next line.  And that requires a caller to post-process the results to figure out how much of the returned content corresponds to the target line.

A caller that has a robust Unicode iterator could ask for e.g. 2 * the screen width and then parse the returned content, measuring widths (e.g. with the wcwidth functions) until they exceed the width of the screen.  But in a line that contains zero width joiners (or emoji qualifiers or etc), it could require much more than 2 * the screen width.  (And my projects do have a robust Unicode iterator, so anywhere they need the full actual content, they can indeed use iterative calls -- but for the most part they just want to know if a line is empty, i.e. contains only spaces.)

I recognize that it's problematic to fit Unicode into the console APIs whose original signatures aren't detailed enough to accommodate surrogates and joiners (or anything else beyond UCS2).  I really like the balanced compromises that the API has made for that over the years.  For example, a caller can iteratively call and analyze the returned output and piece together an accurate representation over multiple calls (or a single extra-large calls and then parse the results), regardless of how many WCHARs are required to fetch a full screen line of text.  That's pretty cool, albeit complex, and gives callers the ability to operate to whatever degree of "correctness" or "thoroughness" that they wish to.

Thanks again for the speedy response and follow-up!
I've also been wondering if we should just replace surrogate pairs with U+FFFD to make this behave more consistent. While I also agree with your point ("gives callers the ability to [...]"), the support for surrogate pairs makes this API different from the `CHAR_INFO` APIs which don't ever support them.
> I've also been wondering if we should just replace surrogate pairs with U+FFFD to make this behave more consistent. While I also agree with your point ("gives callers the ability to [...]"), the support for surrogate pairs makes this API different from the `CHAR_INFO` APIs which don't ever support them.

Have there been complaints about the existing behavior over the past 20+ years that it's behaved the current way?

It would be a breaking change.  Personally, I'm very happy about how it's been working for years.

But since English is my primary language, surrogate pairs are an outlier for me and only appear when certain nerdfont icons or emoji are present.  It might be useful to gather opinions from software developers for whom surrogate pairs are common in normal language text.

If there are neither complaints nor feedback, then I would lean towards preserving compatibility with previous behavior.
@lhecker also, if surrogate pairs would be stripped, then I assume the intent would also be to strip zero width joiners, and all combining marks, right?  There's a lot of room for complications and impactful side effects.
> Have there been complaints about the existing behavior over the past 20+ years that it's behaved the current way?
>
> It would be a breaking change. [...]

That's true, sort of. The console so far was (in pseudocode terms) a `TCHAR[height][width]` buffer for the longest time. Even when `TCHAR` got replaced with `WCHAR` in conhost v2, surrogate pair support was rudimentary and only worked properly in a few select spots. The underlying buffer was still a rectangular matrix.

The reason your repro case worked at all is because that broken UTF16 implementation coincidentally counted the narrow cell with a surrogate pair as a wide cell and thus the iterator stepped over both the leading and trailing surrogate pair. In other words, before I "fixed" it, it actually thought that the row had 121 columns (as an example), even though the buffer is only 120 wide. This caused a ton of issues and regressions elsewhere.

So, while it's definitely a breaking change by definition (= change in behavior causing failures in other components), I don't believe that the exact contents that can be read via these APIs are properly specced out yet and never were. Sure, `ReadConsoleOutputCharacterW` happened to work coincidentally, but what about the other Win32 APIs and VT sequences?

But let me get to a much more concerning point...

> @lhecker also, if surrogate pairs would be stripped, then I assume the intent would also be to strip zero width joiners, and all combining marks, right? There's a lot of room for complications and impactful side effects.

This is exactly my biggest concern right now. I'm planning to add proper Unicode support with extended grapheme clusters in the near term. That is, support for ZWJs, VS15/16, combining marks, and so on. I'm rather worried that this will make `ReadConsoleOutputCharacterW` behave extremely erratic, in particular in respect to the `CHAR_INFO` APIs which must return U+FFFD for such complex graphemes.

Thoughts?
@lhecker that all makes sense.

I can confirm that for my own purposes, the proposed change to RCOCW() would not cause any negative side effects in any of my projects (not even for users in locales where surrogate pairs are common).  And specifically, [Clink](https://github.com/chrisant996/clink) will not encounter problems from such a change.  Because none of my projects do anything more than look for empty lines, anymore (well, Clink can "scrape" the screen for text to use in completions, but it's ok for that to miss some things).

<br/>
<br/>

### _Something to consider for the future:_

It would be fantastic to have some way to save/restore the terminal screen (or a region within the terminal screen), and preserve arbitrarily complex text and colors and styles.  I don't care whether it's an escape code or an API, and it would be perfectly fine for the saved state to be opaque (e.g. if an API filled an out-param buffer with some opaque payload that only the API knows how to unpack and restore -- though of course someone would try to "crack" the payload, and the API would need to guard against fuzzing, etc).

The _Alternate Screen Buffer_ private mode codes (e.g. `CSI ? 1049 h` and `CSI ? 1049 l`) are almost suitable, except that there's no way to copy the Normal Screen Buffer to the Alternate Screen Buffer, so while the save/restore part works they also clear the Alternate Screen Buffer and thus are useless for showing a "popup" or "overlay" over the existing terminal screen.

FWIW, I stopped using ReadConsoleOutputW once it couldn't preserve colors and styles beyond the 4-bit VGA colors.  But, I strongly wish there were a way to save and restore either the whole screen or a region of the screen (including colors, styles, and full Unicode text).  The problem with the guidance "apps should keep track of what they print" is that sometimes an app wants to show a temporary popup window in a region of the screen that was printed by some other app.  PowerShell's progress bars is one example, the Azure CLI installer is another example (but maybe it's just another PowerShell progress bar, I'm not sure), and there are several places in my own projects where I want to save/restore a region of the screen, but no longer can.  In the case of Clink, it scrolls the screen to make room for the popup at the bottom of the screen where it doesn't overlay anything else -- it's functional, but it yields an unpolished and less pleasant experience.
For better or worse, the question about whether to use U+FFFD or not has been answered...
The DBCS implementation has some fairly exhaustive unit tests and those tests are failing now if I properly support narrow-cell surrogate pairs. The unit tests make is very clear that the `nLength` parameter in `ReadConsoleOutputCharacterW` is intended as a column count.

I'll make it return U+FFFD for now as that was my plan for the `CHAR_INFO` API anyways, which have the same restriction (`CHAR_INFO` is explicitly per-column/row after all).
throwaway thought before I leave for lunch: #10810 sounds a lot like what you want

Fix patch:
diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt
index e4d2d676eb4..32dd37878d7 100644
--- a/.github/actions/spelling/expect/expect.txt
+++ b/.github/actions/spelling/expect/expect.txt
@@ -91,7 +91,6 @@ backgrounding
 backported
 backstory
 barbaz
-Batang
 Bazz
 BBDM
 bbwe
@@ -180,7 +179,6 @@ changelists
 chaof
 charinfo
 CHARSETINFO
-chcbpat
 chh
 chshdng
 CHT
@@ -198,7 +196,6 @@ cloudconsole
 cloudvault
 CLSCTX
 clsids
-CLUSTERMAP
 cmatrix
 cmder
 CMDEXT
@@ -214,7 +211,6 @@ codepages
 codepath
 coinit
 colorizing
-COLORMATRIX
 COLORREFs
 colorschemes
 colorspec
@@ -222,7 +218,6 @@ colortable
 colortbl
 colortest
 colortool
-COLR
 combaseapi
 comctl
 commandline
@@ -367,11 +362,9 @@ DBGFONTS
 DBGOUTPUT
 dbh
 dblclk
-DBlob
 DColor
 DCOLORVALUE
 dcommon
-dcompile
 dcompiler
 DComposition
 dde
@@ -479,7 +472,6 @@ depersist
 deprioritized
 deserializers
 desktopwindowxamlsource
-DESTINATIONNAME
 devicecode
 Dext
 DFactory
@@ -539,7 +531,6 @@ DWORDs
 dwrite
 dxgi
 dxgidwm
-dxguid
 dxinterop
 dxsm
 dxttbmp
@@ -607,7 +598,7 @@ FEEF
 fesb
 FFAF
 FFDE
-FFrom
+FFFDb
 fgbg
 FGCOLOR
 FGHIJ
@@ -719,7 +710,6 @@ GETWAITTOKILLTIMEOUT
 GETWHEELSCROLLCHARACTERS
 GETWHEELSCROLLCHARS
 GETWHEELSCROLLLINES
-GFEh
 Gfun
 gfx
 GGI
@@ -867,7 +857,6 @@ INLINEPREFIX
 inproc
 Inputkeyinfo
 INPUTPROCESSORPROFILE
-inputrc
 Inputreadhandledata
 INSERTMODE
 INTERACTIVITYBASE
@@ -908,10 +897,6 @@ KAttrs
 kawa
 Kazu
 kazum
-kcub
-kcud
-kcuf
-kcuu
 kernelbase
 kernelbasestaging
 KEYBDINPUT
@@ -923,7 +908,6 @@ Keymapping
 keyscan
 keystate
 keyups
-khome
 KILLACTIVE
 KILLFOCUS
 kinda
@@ -1061,7 +1045,6 @@ MBUTTON
 MBUTTONDBLCLK
 MBUTTONDOWN
 MBUTTONUP
-Mbxy
 mdmerge
 MDs
 MEASUREITEM
@@ -1086,7 +1069,6 @@ minkernel
 MINMAXINFO
 minwin
 minwindef
-Mip
 MMBB
 mmcc
 MMCPL
@@ -1123,8 +1105,8 @@ msix
 msrc
 MSVCRTD
 MTSM
-munges
 Munged
+munges
 murmurhash
 muxes
 myapplet
@@ -1221,7 +1203,6 @@ ntdll
 ntifs
 ntlpcapi
 ntm
-nto
 ntrtl
 ntstatus
 NTSYSCALLAPI
@@ -1297,7 +1278,6 @@ parentable
 parms
 PATCOPY
 pathcch
-Pathto
 PATTERNID
 pcat
 pcb
@@ -1456,7 +1436,6 @@ pwsz
 pythonw
 Qaabbcc
 QUERYOPEN
-QUESTIONMARK
 quickedit
 QUZ
 QWER
@@ -1489,8 +1468,6 @@ READCONSOLE
 READCONSOLEOUTPUT
 READCONSOLEOUTPUTSTRING
 READMODE
-reallocs
-reamapping
 rectread
 redef
 redefinable
@@ -1518,7 +1495,6 @@ repositorypath
 Requiresx
 rerasterize
 rescap
-Resequence
 RESETCONTENT
 resheader
 resmimetype
@@ -1551,7 +1527,6 @@ RRRGGGBB
 rsas
 rtcore
 RTEXT
-RTFTo
 RTLREADING
 Rtn
 ruleset
@@ -1699,7 +1674,6 @@ srcsrv
 SRCSRVTRG
 srctool
 srect
-srv
 srvinit
 srvpipe
 ssa
@@ -1731,7 +1705,6 @@ SUA
 subcompartment
 subkeys
 SUBLANG
-subresource
 subsystemconsole
 subsystemwindows
 swapchain
@@ -1835,7 +1808,6 @@ tosign
 touchpad
 Tpp
 Tpqrst
-tracelog
 tracelogging
 traceviewpp
 trackbar
@@ -1846,7 +1818,6 @@ Trd
 TREX
 triaged
 triaging
-TRIANGLESTRIP
 Tribool
 TRIMZEROHEADINGS
 trx
@@ -1888,8 +1859,8 @@ UINTs
 ul
 ulcch
 uld
-uldb
 uldash
+uldb
 ulwave
 Unadvise
 unattend
@@ -1900,7 +1871,6 @@ unhosted
 UNICODETEXT
 UNICRT
 Unintense
-Uniscribe
 unittesting
 unittests
 unk
@@ -1912,7 +1882,6 @@ untextured
 UPDATEDISPLAY
 UPDOWN
 UPKEY
-UPSS
 upss
 uregex
 URegular
@@ -1996,7 +1965,6 @@ VTRGBTo
 vtseq
 vtterm
 vttest
-waitable
 WANSUNG
 WANTARROWS
 WANTTAB
@@ -2121,7 +2089,6 @@ wrkstr
 wrl
 wrp
 WRunoff
-WScript
 wsl
 WSLENV
 wstr
@@ -2156,13 +2123,11 @@ XBUTTONDOWN
 XBUTTONUP
 XCast
 XCENTER
-XColors
 xcopy
 XCount
 xdy
 XEncoding
 xes
-xff
 XFG
 XFile
 XFORM
@@ -2171,7 +2136,6 @@ xinchaof
 xinxinchaof
 XManifest
 XMath
-XMFLOAT
 xorg
 XResource
 xsi
diff --git a/src/host/ft_host/CJK_DbcsTests.cpp b/src/host/ft_host/CJK_DbcsTests.cpp
index 68798b700fd..9d96e470276 100644
--- a/src/host/ft_host/CJK_DbcsTests.cpp
+++ b/src/host/ft_host/CJK_DbcsTests.cpp
@@ -164,6 +164,8 @@ class DbcsTests
     BEGIN_TEST_METHOD(TestInvalidTrailer)
         TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
     END_TEST_METHOD()
+
+    TEST_METHOD(TestNarrowSurrogate);
 };
 
 bool DbcsTests::DbcsTestSetup()
@@ -2183,3 +2185,18 @@ void DbcsTests::TestInvalidTrailer()
 
     DbcsWriteRead::Verify(expected, output);
 }
+
+// The various console APIs that read back from the buffer are generally incompatible with UTF16 and surrogate pairs.
+// ReadConsoleOutputCharacterW in particular has a nLength parameter which is a column count but also the buffer size.
+// This makes it impossible to reliably return arbitrarily long graphemes per-cell in the output buffer.
+// The test ensures that we replace them with U+FFFD which makes the behavior more consistent for the caller.
+void DbcsTests::TestNarrowSurrogate()
+{
+    const auto out = GetStdHandle(STD_OUTPUT_HANDLE);
+    wchar_t buf[3];
+    DWORD read;
+
+    VERIFY_WIN32_BOOL_SUCCEEDED(WriteConsoleOutputCharacterW(out, L"a\U00010000b", 4, {}, &read));
+    VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterW(out, &buf[0], ARRAYSIZE(buf), {}, &read));
+    VERIFY_ARE_EQUAL(std::wstring_view(L"a\U0000FFFDb"), std::wstring_view(&buf[0], read));
+}
diff --git a/src/host/output.cpp b/src/host/output.cpp
index bd168fce9cd..bf1e0a91040 100644
--- a/src/host/output.cpp
+++ b/src/host/output.cpp
@@ -222,7 +222,12 @@ std::wstring ReadOutputStringW(const SCREEN_INFORMATION& screenInfo,
             // Otherwise, add anything that isn't a trailing cell. (Trailings are duplicate copies of the leading.)
             if (it->DbcsAttr() != DbcsAttribute::Trailing)
             {
-                retVal += it->Chars();
+                auto chars = it->Chars();
+                if (chars.size() > 1)
+                {
+                    chars = { &UNICODE_REPLACEMENT, 1 };
+                }
+                retVal += chars;
             }
         }
 


Test patch: