Repository: microsoft/terminal (language: cpp)

Issue:
Investigate why the `COOKED_READ_DATA` ctor takes a `string_view` for `wchar_t` data?
`COOKED_READ_DATA` takes an `initialData` parameter of a `string_view`
https://github.com/microsoft/terminal/blob/d6cae40d26d2f4ba249d3960e8478c04681c8e1e/src/host/readDataCooked.cpp#L46-L54

However, when it `memcpy`'s it, it copies it as a `wchar_t` buffer:
https://github.com/microsoft/terminal/blob/d6cae40d26d2f4ba249d3960e8478c04681c8e1e/src/host/readDataCooked.cpp#L97-L103

As you can see here, `_bufPtr` is a `wchar_t*` which is initialized to `_bufPtr = reinterpret_cast<wchar_t*>(_buffer.get())`
https://github.com/microsoft/terminal/blob/d6cae40d26d2f4ba249d3960e8478c04681c8e1e/src/host/readDataCooked.hpp#L124-L136

As I'm writing tests that use some cooked read data for #1856, I'm finding that the string I'm initializing the cooked read with is getting horribly mangled, because I'm passing a `char` buffer in.

Why is it like this? Does this magically just normally work when users call `ReadConsole` with `wchar_t` data in the buffer? I couldn't really figure it out in just a morning, so I'm filing this to make sure to follow up. 

The test in question is `ConptyRoundtripTests::TestResizeWithCookedRead`. 


Discussion / hints:
AFAIK this is related to how we share cooked read between A and W APIs?
Actually, yeah, that doesn't make sense.

Fix patch:
diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index 9f267746511..76065f93c95 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
@@ -2891,8 +2891,7 @@ void ConptyRoundtripTests::TestResizeWithCookedRead()
     m_state->PrepareReadHandle();
     // TODO GH#5618: This string will get mangled, but we don't really care
     // about the buffer contents in this test, so it doesn't really matter.
-    const std::string_view cookedReadContents{ "This is some cooked read data" };
-    m_state->PrepareCookedReadData(cookedReadContents);
+    m_state->PrepareCookedReadData(L"This is some cooked read data");
 
     Log::Comment(L"Painting the frame");
     VERIFY_SUCCEEDED(renderer.PaintFrame());
diff --git a/src/host/ApiRoutines.h b/src/host/ApiRoutines.h
index 3492ae234a8..a46f1397ec6 100644
--- a/src/host/ApiRoutines.h
+++ b/src/host/ApiRoutines.h
@@ -57,27 +57,17 @@ class ApiRoutines : public IApiRoutines
                                               const bool IsPeek,
                                               std::unique_ptr<IWaitRoutine>& waiter) noexcept override;
 
-    [[nodiscard]] HRESULT ReadConsoleAImpl(IConsoleInputObject& context,
-                                           std::span<char> buffer,
-                                           size_t& written,
-                                           std::unique_ptr<IWaitRoutine>& waiter,
-                                           const std::string_view initialData,
-                                           const std::wstring_view exeName,
-                                           INPUT_READ_HANDLE_DATA& readHandleState,
-                                           const HANDLE clientHandle,
-                                           const DWORD controlWakeupMask,
-                                           DWORD& controlKeyState) noexcept override;
-
-    [[nodiscard]] HRESULT ReadConsoleWImpl(IConsoleInputObject& context,
-                                           std::span<char> buffer,
-                                           size_t& written,
-                                           std::unique_ptr<IWaitRoutine>& waiter,
-                                           const std::string_view initialData,
-                                           const std::wstring_view exeName,
-                                           INPUT_READ_HANDLE_DATA& readHandleState,
-                                           const HANDLE clientHandle,
-                                           const DWORD controlWakeupMask,
-                                           DWORD& controlKeyState) noexcept override;
+    [[nodiscard]] HRESULT ReadConsoleImpl(IConsoleInputObject& context,
+                                          std::span<char> buffer,
+                                          size_t& written,
+                                          std::unique_ptr<IWaitRoutine>& waiter,
+                                          const std::wstring_view initialData,
+                                          const std::wstring_view exeName,
+                                          INPUT_READ_HANDLE_DATA& readHandleState,
+                                          const bool IsUnicode,
+                                          const HANDLE clientHandle,
+                                          const DWORD controlWakeupMask,
+                                          DWORD& controlKeyState) noexcept override;
 
     [[nodiscard]] HRESULT WriteConsoleAImpl(IConsoleOutputObject& context,
                                             const std::string_view buffer,
diff --git a/src/host/VtApiRoutines.cpp b/src/host/VtApiRoutines.cpp
index ff71f8c3a5b..fe7d0b7060e 100644
--- a/src/host/VtApiRoutines.cpp
+++ b/src/host/VtApiRoutines.cpp
@@ -119,42 +119,19 @@ void VtApiRoutines::_SynchronizeCursor(std::unique_ptr<IWaitRoutine>& waiter) no
     return hr;
 }
 
-[[nodiscard]] HRESULT VtApiRoutines::ReadConsoleAImpl(IConsoleInputObject& context,
-                                                      std::span<char> buffer,
-                                                      size_t& written,
-                                                      std::unique_ptr<IWaitRoutine>& waiter,
-                                                      const std::string_view initialData,
-                                                      const std::wstring_view exeName,
-                                                      INPUT_READ_HANDLE_DATA& readHandleState,
-                                                      const HANDLE clientHandle,
-                                                      const DWORD controlWakeupMask,
-                                                      DWORD& controlKeyState) noexcept
-{
-    const auto hr = m_pUsualRoutines->ReadConsoleAImpl(context, buffer, written, waiter, initialData, exeName, readHandleState, clientHandle, controlWakeupMask, controlKeyState);
-    // If we're about to tell the caller to wait, let's synchronize the cursor we have with what
-    // the terminal is presenting in case there's a cooked read going on.
-    // TODO GH10001: we only need to do this in cooked read mode.
-    if (clientHandle)
-    {
-        m_listeningForDSR = true;
-        (void)m_pVtEngine->_ListenForDSR();
-        (void)m_pVtEngine->RequestCursor();
-    }
-    return hr;
-}
-
-[[nodiscard]] HRESULT VtApiRoutines::ReadConsoleWImpl(IConsoleInputObject& context,
-                                                      std::span<char> buffer,
-                                                      size_t& written,
-                                                      std::unique_ptr<IWaitRoutine>& waiter,
-                                                      const std::string_view initialData,
-                                                      const std::wstring_view exeName,
-                                                      INPUT_READ_HANDLE_DATA& readHandleState,
-                                                      const HANDLE clientHandle,
-                                                      const DWORD controlWakeupMask,
-                                                      DWORD& controlKeyState) noexcept
-{
-    const auto hr = m_pUsualRoutines->ReadConsoleWImpl(context, buffer, written, waiter, initialData, exeName, readHandleState, clientHandle, controlWakeupMask, controlKeyState);
+[[nodiscard]] HRESULT VtApiRoutines::ReadConsoleImpl(IConsoleInputObject& context,
+                                                     std::span<char> buffer,
+                                                     size_t& written,
+                                                     std::unique_ptr<IWaitRoutine>& waiter,
+                                                     const std::wstring_view initialData,
+                                                     const std::wstring_view exeName,
+                                                     INPUT_READ_HANDLE_DATA& readHandleState,
+                                                     const bool IsUnicode,
+                                                     const HANDLE clientHandle,
+                                                     const DWORD controlWakeupMask,
+                                                     DWORD& controlKeyState) noexcept
+{
+    const auto hr = m_pUsualRoutines->ReadConsoleImpl(context, buffer, written, waiter, initialData, exeName, readHandleState, IsUnicode, clientHandle, controlWakeupMask, controlKeyState);
     // If we're about to tell the caller to wait, let's synchronize the cursor we have with what
     // the terminal is presenting in case there's a cooked read going on.
     // TODO GH10001: we only need to do this in cooked read mode.
diff --git a/src/host/VtApiRoutines.h b/src/host/VtApiRoutines.h
index 8c3cbf917ef..7dc77f78b09 100644
--- a/src/host/VtApiRoutines.h
+++ b/src/host/VtApiRoutines.h
@@ -60,27 +60,17 @@ class VtApiRoutines : public IApiRoutines
                                               const bool IsPeek,
                                               std::unique_ptr<IWaitRoutine>& waiter) noexcept override;
 
-    [[nodiscard]] HRESULT ReadConsoleAImpl(IConsoleInputObject& context,
-                                           std::span<char> buffer,
-                                           size_t& written,
-                                           std::unique_ptr<IWaitRoutine>& waiter,
-                                           const std::string_view initialData,
-                                           const std::wstring_view exeName,
-                                           INPUT_READ_HANDLE_DATA& readHandleState,
-                                           const HANDLE clientHandle,
-                                           const DWORD controlWakeupMask,
-                                           DWORD& controlKeyState) noexcept override;
-
-    [[nodiscard]] HRESULT ReadConsoleWImpl(IConsoleInputObject& context,
-                                           std::span<char> buffer,
-                                           size_t& written,
-                                           std::unique_ptr<IWaitRoutine>& waiter,
-                                           const std::string_view initialData,
-                                           const std::wstring_view exeName,
-                                           INPUT_READ_HANDLE_DATA& readHandleState,
-                                           const HANDLE clientHandle,
-                                           const DWORD controlWakeupMask,
-                                           DWORD& controlKeyState) noexcept override;
+    [[nodiscard]] HRESULT ReadConsoleImpl(IConsoleInputObject& context,
+                                          std::span<char> buffer,
+                                          size_t& written,
+                                          std::unique_ptr<IWaitRoutine>& waiter,
+                                          const std::wstring_view initialData,
+                                          const std::wstring_view exeName,
+                                          INPUT_READ_HANDLE_DATA& readHandleState,
+                                          const bool IsUnicode,
+                                          const HANDLE clientHandle,
+                                          const DWORD controlWakeupMask,
+                                          DWORD& controlKeyState) noexcept override;
 
     [[nodiscard]] HRESULT WriteConsoleAImpl(IConsoleOutputObject& context,
                                             const std::string_view buffer,
diff --git a/src/host/readDataCooked.cpp b/src/host/readDataCooked.cpp
index 6da7ed899ab..75e57b55963 100644
--- a/src/host/readDataCooked.cpp
+++ b/src/host/readDataCooked.cpp
@@ -50,7 +50,7 @@ COOKED_READ_DATA::COOKED_READ_DATA(_In_ InputBuffer* const pInputBuffer,
                                    _In_ char* UserBuffer,
                                    _In_ ULONG CtrlWakeupMask,
                                    _In_ const std::wstring_view exeName,
-                                   _In_ const std::string_view initialData,
+                                   _In_ const std::wstring_view initialData,
                                    _In_ ConsoleProcessHandle* const pClientProcess) :
     ReadData(pInputBuffer, pInputReadHandleData),
     _screenInfo{ screenInfo },
@@ -97,11 +97,11 @@ COOKED_READ_DATA::COOKED_READ_DATA(_In_ InputBuffer* const pInputBuffer,
 
     if (!initialData.empty())
     {
-        memcpy_s(_bufPtr, _bufferSize, initialData.data(), initialData.size());
+        memcpy_s(_bufPtr, _bufferSize, initialData.data(), initialData.size() * 2);
 
-        _bytesRead += initialData.size();
+        _bytesRead += initialData.size() * 2;
 
-        const auto cchInitialData = initialData.size() / sizeof(wchar_t);
+        const auto cchInitialData = initialData.size();
         VisibleCharCount() = cchInitialData;
         _bufPtr += cchInitialData;
         _currentPosition = cchInitialData;
diff --git a/src/host/readDataCooked.hpp b/src/host/readDataCooked.hpp
index a263303e45d..be2e8645bc1 100644
--- a/src/host/readDataCooked.hpp
+++ b/src/host/readDataCooked.hpp
@@ -40,7 +40,7 @@ class COOKED_READ_DATA final : public ReadData
                      _In_ char* UserBuffer,
                      _In_ ULONG CtrlWakeupMask,
                      _In_ const std::wstring_view exeName,
-                     _In_ const std::string_view initialData,
+                     _In_ const std::wstring_view initialData,
                      _In_ ConsoleProcessHandle* const pClientProcess);
 
     ~COOKED_READ_DATA() override;
diff --git a/src/host/stream.cpp b/src/host/stream.cpp
index 9ecccd2fadb..c98ef9eaf08 100644
--- a/src/host/stream.cpp
+++ b/src/host/stream.cpp
@@ -354,7 +354,7 @@ NT_CATCH_RETURN()
                                             std::span<char> buffer,
                                             size_t& bytesRead,
                                             DWORD& controlKeyState,
-                                            const std::string_view initialData,
+                                            const std::wstring_view initialData,
                                             const DWORD ctrlWakeupMask,
                                             INPUT_READ_HANDLE_DATA& readHandleState,
                                             const std::wstring_view exeName,
@@ -492,7 +492,7 @@ NT_CATCH_RETURN()
                                      std::span<char> buffer,
                                      size_t& bytesRead,
                                      ULONG& controlKeyState,
-                                     const std::string_view initialData,
+                                     const std::wstring_view initialData,
                                      const DWORD ctrlWakeupMask,
                                      INPUT_READ_HANDLE_DATA& readHandleState,
                                      const std::wstring_view exeName,
@@ -552,60 +552,29 @@ NT_CATCH_RETURN()
     CATCH_RETURN();
 }
 
-[[nodiscard]] HRESULT ApiRoutines::ReadConsoleAImpl(IConsoleInputObject& context,
-                                                    std::span<char> buffer,
-                                                    size_t& written,
-                                                    std::unique_ptr<IWaitRoutine>& waiter,
-                                                    const std::string_view initialData,
-                                                    const std::wstring_view exeName,
-                                                    INPUT_READ_HANDLE_DATA& readHandleState,
-                                                    const HANDLE clientHandle,
-                                                    const DWORD controlWakeupMask,
-                                                    DWORD& controlKeyState) noexcept
+[[nodiscard]] HRESULT ApiRoutines::ReadConsoleImpl(IConsoleInputObject& context,
+                                                   std::span<char> buffer,
+                                                   size_t& written,
+                                                   std::unique_ptr<IWaitRoutine>& waiter,
+                                                   const std::wstring_view initialData,
+                                                   const std::wstring_view exeName,
+                                                   INPUT_READ_HANDLE_DATA& readHandleState,
+                                                   const bool IsUnicode,
+                                                   const HANDLE clientHandle,
+                                                   const DWORD controlWakeupMask,
+                                                   DWORD& controlKeyState) noexcept
 {
-    try
-    {
-        return HRESULT_FROM_NT(DoReadConsole(context,
-                                             clientHandle,
-                                             buffer,
-                                             written,
-                                             controlKeyState,
-                                             initialData,
-                                             controlWakeupMask,
-                                             readHandleState,
-                                             exeName,
-                                             false,
-                                             waiter));
-    }
-    CATCH_RETURN();
-}
-
-[[nodiscard]] HRESULT ApiRoutines::ReadConsoleWImpl(IConsoleInputObject& context,
-                                                    std::span<char> buffer,
-                                                    size_t& written,
-                                                    std::unique_ptr<IWaitRoutine>& waiter,
-                                                    const std::string_view initialData,
-                                                    const std::wstring_view exeName,
-                                                    INPUT_READ_HANDLE_DATA& readHandleState,
-                                                    const HANDLE clientHandle,
-                                                    const DWORD controlWakeupMask,
-                                                    DWORD& controlKeyState) noexcept
-{
-    try
-    {
-        return HRESULT_FROM_NT(DoReadConsole(context,
-                                             clientHandle,
-                                             buffer,
-                                             written,
-                                             controlKeyState,
-                                             initialData,
-                                             controlWakeupMask,
-                                             readHandleState,
-                                             exeName,
-                                             true,
-                                             waiter));
-    }
-    CATCH_RETURN();
+    return HRESULT_FROM_NT(DoReadConsole(context,
+                                         clientHandle,
+                                         buffer,
+                                         written,
+                                         controlKeyState,
+                                         initialData,
+                                         controlWakeupMask,
+                                         readHandleState,
+                                         exeName,
+                                         IsUnicode,
+                                         waiter));
 }
 
 void UnblockWriteConsole(const DWORD dwReason)
diff --git a/src/server/ApiDispatchers.cpp b/src/server/ApiDispatchers.cpp
index ce3b81b8212..01f4bcac8df 100644
--- a/src/server/ApiDispatchers.cpp
+++ b/src/server/ApiDispatchers.cpp
@@ -276,17 +276,23 @@ static DWORD TraceGetThreadId(CONSOLE_API_MSG* const m)
     const std::wstring_view exeView(pwsExeName.get(), cchExeName);
 
     // 2. Existing data in the buffer that was passed in.
-    const auto cbInitialData = a->InitialNumBytes;
     std::unique_ptr<char[]> pbInitialData;
+    std::wstring_view initialData;
 
     try
     {
+        const auto cbInitialData = a->InitialNumBytes;
         if (cbInitialData > 0)
         {
+            // InitialNumBytes is only supported for ReadConsoleW (via CONSOLE_READCONSOLE_CONTROL::nInitialChars).
+            RETURN_HR_IF(E_INVALIDARG, !a->Unicode);
+
             pbInitialData = std::make_unique<char[]>(cbInitialData);
 
             // This parameter starts immediately after the exe name so skip by that many bytes.
             RETURN_IF_FAILED(m->ReadMessageInput(cbExeName, pbInitialData.get(), cbInitialData));
+
+            initialData = { reinterpret_cast<const wchar_t*>(pbInitialData.get()), cbInitialData / sizeof(wchar_t) };
         }
     }
     CATCH_RETURN();
@@ -301,37 +307,18 @@ static DWORD TraceGetThreadId(CONSOLE_API_MSG* const m)
     std::unique_ptr<IWaitRoutine> waiter;
     size_t cbWritten;
 
-    HRESULT hr;
-    if (a->Unicode)
-    {
-        const std::string_view initialData(pbInitialData.get(), cbInitialData);
-        const std::span<char> outputBuffer(reinterpret_cast<char*>(pvBuffer), cbBufferSize);
-        hr = m->_pApiRoutines->ReadConsoleWImpl(*pInputBuffer,
-                                                outputBuffer,
-                                                cbWritten, // We must set the reply length in bytes.
-                                                waiter,
-                                                initialData,
-                                                exeView,
-                                                *pInputReadHandleData,
-                                                hConsoleClient,
-                                                a->CtrlWakeupMask,
-                                                a->ControlKeyState);
-    }
-    else
-    {
-        const std::string_view initialData(pbInitialData.get(), cbInitialData);
-        const std::span<char> outputBuffer(reinterpret_cast<char*>(pvBuffer), cbBufferSize);
-        hr = m->_pApiRoutines->ReadConsoleAImpl(*pInputBuffer,
+    const std::span<char> outputBuffer(reinterpret_cast<char*>(pvBuffer), cbBufferSize);
+    auto hr = m->_pApiRoutines->ReadConsoleImpl(*pInputBuffer,
                                                 outputBuffer,
                                                 cbWritten, // We must set the reply length in bytes.
                                                 waiter,
                                                 initialData,
                                                 exeView,
                                                 *pInputReadHandleData,
+                                                a->Unicode,
                                                 hConsoleClient,
                                                 a->CtrlWakeupMask,
                                                 a->ControlKeyState);
-    }
 
     LOG_IF_FAILED(SizeTToULong(cbWritten, &a->NumBytes));
 
diff --git a/src/server/IApiRoutines.h b/src/server/IApiRoutines.h
index a5d691bf149..87830795c6c 100644
--- a/src/server/IApiRoutines.h
+++ b/src/server/IApiRoutines.h
@@ -74,27 +74,17 @@ class IApiRoutines
                                                       const bool IsPeek,
                                                       std::unique_ptr<IWaitRoutine>& waiter) noexcept = 0;
 
-    [[nodiscard]] virtual HRESULT ReadConsoleAImpl(IConsoleInputObject& context,
-                                                   std::span<char> buffer,
-                                                   size_t& written,
-                                                   std::unique_ptr<IWaitRoutine>& waiter,
-                                                   const std::string_view initialData,
-                                                   const std::wstring_view exeName,
-                                                   INPUT_READ_HANDLE_DATA& readHandleState,
-                                                   const HANDLE clientHandle,
-                                                   const DWORD controlWakeupMask,
-                                                   DWORD& controlKeyState) noexcept = 0;
-
-    [[nodiscard]] virtual HRESULT ReadConsoleWImpl(IConsoleInputObject& context,
-                                                   std::span<char> buffer,
-                                                   size_t& written,
-                                                   std::unique_ptr<IWaitRoutine>& waiter,
-                                                   const std::string_view initialData,
-                                                   const std::wstring_view exeName,
-                                                   INPUT_READ_HANDLE_DATA& readHandleState,
-                                                   const HANDLE clientHandle,
-                                                   const DWORD controlWakeupMask,
-                                                   DWORD& controlKeyState) noexcept = 0;
+    [[nodiscard]] virtual HRESULT ReadConsoleImpl(IConsoleInputObject& context,
+                                                  std::span<char> buffer,
+                                                  size_t& written,
+                                                  std::unique_ptr<IWaitRoutine>& waiter,
+                                                  const std::wstring_view initialData,
+                                                  const std::wstring_view exeName,
+                                                  INPUT_READ_HANDLE_DATA& readHandleState,
+                                                  const bool IsUnicode,
+                                                  const HANDLE clientHandle,
+                                                  const DWORD controlWakeupMask,
+                                                  DWORD& controlKeyState) noexcept = 0;
 
     [[nodiscard]] virtual HRESULT WriteConsoleAImpl(IConsoleOutputObject& context,
                                                     const std::string_view buffer,


Test patch:
diff --git a/src/inc/test/CommonState.hpp b/src/inc/test/CommonState.hpp
index 942d0c6007e..9a0e1272b7d 100644
--- a/src/inc/test/CommonState.hpp
+++ b/src/inc/test/CommonState.hpp
@@ -144,7 +144,7 @@ class CommonState
         delete gci.pInputBuffer;
     }
 
-    void PrepareCookedReadData(const std::string_view initialData = {})
+    void PrepareCookedReadData(const std::wstring_view initialData = {})
     {
         CONSOLE_INFORMATION& gci = Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().getConsoleInformation();
         auto* readData = new COOKED_READ_DATA(gci.pInputBuffer,