Repository: microsoft/terminal (language: cpp)

Issue:
interaction of _read on fd 0 in terminal
### Windows Terminal version

1.19.2682.0

### Windows build number

Version 10.0.22621.2428

### Other Software

_No response_

### Steps to reproduce

Compile the attached, run, and  press enter multiple times. Behavior is the same with MSVC in Visual studio 2022 and with latest MinGW with GCC using 64-bit UCRT.
```
#include <io.h>
#include <stdio.h>
#include <errno.h>


int main()
{
  int fd = 0;
  printf("fd = %d\n", fd);

  int ret;
  char buf[128];

  while (1)
    {
      errno = 0;
      ret = _read (fd, buf, 1);
      printf("ret = %d, errno = %d, char = %u\n", ret, errno, buf[0]);
    }
  return 0;
}
```


### Expected Behavior

I expected the behavior found on MinGW's mintty, when I press the enter key multiple times.

```
fd = 0

ret = 1, errno = 0, char = 10

ret = 1, errno = 0, char = 10

ret = 1, errno = 0, char = 10

ret = 1, errno = 0, char = 10
```

### Actual Behavior

This is what happens in Terminal when I press the enter key multiple times on a recent Terminal
```
fd = 0


ret = 1, errno = 0, char = 13
ret = -1, errno = 13, char = 13


ret = 1, errno = 0, char = 13
ret = -1, errno = 13, char = 13


ret = 1, errno = 0, char = 13
ret = -1, errno = 13, char = 13


ret = 1, errno = 0, char = 13
ret = -1, errno = 13, char = 13
```


Discussion / hints:
Apparently something having to do with handling of CR LF line endings in terminal, but, that `_read` errors with `EACCES` is unexpected.
I'm gonna reckon that this maybe regressed due to #15783. I'll tag @lhecker to investigate. Thanks for the minimal repro!
maybe #15662?
I noticed that if you change the `_read` call to `_read(fd, buf, 2)` it works fine and always returns 1 character of  ASCII 10..  It is like the `_read` needs a two character buffer to hold (i presume) "\r\n" before some sort of cooked logic turns it into "\n" before storing it into `buf`.
I believe this is a duplicate of #16223. I haven't checked how `_read()` is implemented but I bet it's a wrapper around `ReadFile()`...

Fix patch:
diff --git a/src/host/inputBuffer.cpp b/src/host/inputBuffer.cpp
index 3a4e556e440..5150a21427d 100644
--- a/src/host/inputBuffer.cpp
+++ b/src/host/inputBuffer.cpp
@@ -4,13 +4,13 @@
 #include "precomp.h"
 #include "inputBuffer.hpp"
 
-#include "stream.h"
-#include "../types/inc/GlyphWidth.hpp"
-
 #include <til/bytes.h>
+#include <til/unicode.h>
 
 #include "misc.h"
+#include "stream.h"
 #include "../interactivity/inc/ServiceLocator.hpp"
+#include "../types/inc/GlyphWidth.hpp"
 
 #define INPUT_BUFFER_DEFAULT_INPUT_MODE (ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT | ENABLE_ECHO_INPUT | ENABLE_MOUSE_INPUT)
 
@@ -87,10 +87,10 @@ void InputBuffer::Consume(bool isUnicode, std::wstring_view& source, std::span<c
         {
             size_t read = 0;
 
-            for (const auto& wch : source)
+            for (const auto& s : til::utf16_iterator{ source })
             {
                 char buffer[8];
-                const auto length = WideCharToMultiByte(cp, 0, &wch, 1, &buffer[0], sizeof(buffer), nullptr, nullptr);
+                const auto length = WideCharToMultiByte(cp, 0, s.data(), gsl::narrow_cast<int>(s.size()), &buffer[0], sizeof(buffer), nullptr, nullptr);
                 THROW_LAST_ERROR_IF(length <= 0);
 
                 std::string_view slice{ &buffer[0], gsl::narrow_cast<size_t>(length) };
@@ -98,10 +98,24 @@ void InputBuffer::Consume(bool isUnicode, std::wstring_view& source, std::span<c
 
                 ++read;
 
-                if (!slice.empty())
+                // The _cached members store characters in `target`'s encoding that didn't fit into
+                // the client's buffer. So, if slice.empty() == false, then we'll store `slice` there.
+                //
+                // But it would be incorrect to test for slice.empty() == false, because the exit
+                // condition is actually "if the target has no space left" and that's subtly different.
+                // This difference can be seen when `source` contains "abc" and `target` is 1 character large.
+                // Testing for `target.empty() will ensure we:
+                // * exit right after copying "a"
+                // * don't store anything in `_cachedTextA`
+                // * leave "bc" in the `source` string, for the caller to handle
+                // Otherwise we'll copy "a", store "b" and return "c", which is wrong. See GH#16223.
+                if (target.empty())
                 {
-                    _cachedTextA = slice;
-                    _cachedTextReaderA = _cachedTextA;
+                    if (!slice.empty())
+                    {
+                        _cachedTextA = slice;
+                        _cachedTextReaderA = _cachedTextA;
+                    }
                     break;
                 }
             }


Test patch: