Merged PR 6034984: Fix a crash caused by improper buffer management w/ multiple clients

Until there's a "Wait", there's usually only one API message inflight at
a time.

In our quest for performance, we put that single API message in charge
of its own buffer management: instead of allocating buffers on the heap
and deleting them later (storing pointers to them at the far corners of
the earth), it would instead allocate them from small internal pools (if
possible) and only heap allocate (transparently) if necessary. The
pointers flung to the corners of the earth would be pointers (1) back
into the API_MSG or (2) to a heap block owned by boost::small_vector.

It took us months to realize that those bare pointers were being held by
COOKED_READ and RAW_READ and not actually being updated when the API
message was _copied_ as it was shuffled off to the background to become
a "Wait" message.

It turns out that it's trivially possible to crash the console by
sending two API calls--one that waits and one that completes
immediately--when the waiting message or the "wait completer" has a
bunch of dangling pointers in it. It further turns out that some
accessibility software (like JAWS) attaches directly to the console
session, much like winpty and ConEmu and friends. They're trying to read
out the buffer (API call!) and sometimes there's a shell waiting for
input (API call!). Oops.

In this commit, we fix up the message's internal pointers (in lieu of
giving it a proper copy constructor; see GH-10076) and then tell the
wait completion routine (which is going to be a COOKED_READ, RAW_READ,
DirectRead or WriteData) about the new buffer location.

This is a scoped fix that should be replaced (TODO GH-10076) with a
final one after Ask mode.

Retrieved from https://microsoft.visualstudio.com os.2020 OS official/rs_wdx_dxp_windev eca0875950fd3a9735662474613405e2dc06f485

References GH-10076

Fixes MSFT-33127449
Fixes GH-9692

src/host/readData.hpp

@@ -36,6 +36,7 @@ public:
     ReadData& operator=(const ReadData&) & = delete;
     ReadData& operator=(ReadData&&) & = delete;
 
+    virtual void MigrateUserBuffersOnTransitionToBackgroundWait(const void* oldBuffer, void* newBuffer) = 0;
     virtual bool Notify(const WaitTerminationReason TerminationReason,
                         const bool fIsUnicode,
                         _Out_ NTSTATUS* const pReplyStatus,

src/host/readDataCooked.cpp

@@ -1197,3 +1197,12 @@ void COOKED_READ_DATA::SavePendingInput(const size_t index, const bool multiline
     }
     return STATUS_SUCCESS;
 }
+
+void COOKED_READ_DATA::MigrateUserBuffersOnTransitionToBackgroundWait(const void* oldBuffer, void* newBuffer)
+{
+    // See the comment in WaitBlock.cpp for more information.
+    if (_userBuffer == reinterpret_cast<const wchar_t*>(oldBuffer))
+    {
+        _userBuffer = reinterpret_cast<wchar_t*>(newBuffer);
+    }
+}

src/host/readDataCooked.hpp

@@ -48,6 +48,8 @@ public:
 
     bool AtEol() const noexcept;
 
+    void MigrateUserBuffersOnTransitionToBackgroundWait(const void* oldBuffer, void* newBuffer) override;
+
     bool Notify(const WaitTerminationReason TerminationReason,
                 const bool fIsUnicode,
                 _Out_ NTSTATUS* const pReplyStatus,

src/host/readDataDirect.cpp

@@ -190,3 +190,8 @@ bool DirectReadData::Notify(const WaitTerminationReason TerminationReason,
     }
     return retVal;
 }
+
+void DirectReadData::MigrateUserBuffersOnTransitionToBackgroundWait(const void* /*oldBuffer*/, void* /*newBuffer*/)
+{
+    // Direct read doesn't hold API message buffers
+}

src/host/readDataDirect.hpp

@@ -40,6 +40,7 @@ public:
 
     ~DirectReadData() override;
 
+    void MigrateUserBuffersOnTransitionToBackgroundWait(const void* oldBuffer, void* newBuffer) override;
     bool Notify(const WaitTerminationReason TerminationReason,
                 const bool fIsUnicode,
                 _Out_ NTSTATUS* const pReplyStatus,

src/host/readDataRaw.cpp

@@ -218,3 +218,12 @@ bool RAW_READ_DATA::Notify(const WaitTerminationReason TerminationReason,
     }
     return RetVal;
 }
+
+void RAW_READ_DATA::MigrateUserBuffersOnTransitionToBackgroundWait(const void* oldBuffer, void* newBuffer)
+{
+    // See the comment in WaitBlock.cpp for more information.
+    if (_BufPtr == reinterpret_cast<const wchar_t*>(oldBuffer))
+    {
+        _BufPtr = reinterpret_cast<wchar_t*>(newBuffer);
+    }
+}

src/host/readDataRaw.hpp

@@ -37,6 +37,7 @@ public:
 
     RAW_READ_DATA(RAW_READ_DATA&&) = default;
 
+    void MigrateUserBuffersOnTransitionToBackgroundWait(const void* oldBuffer, void* newBuffer) override;
     bool Notify(const WaitTerminationReason TerminationReason,
                 const bool fIsUnicode,
                 _Out_ NTSTATUS* const pReplyStatus,

src/host/writeData.cpp

@@ -189,3 +189,8 @@ bool WriteData::Notify(const WaitTerminationReason TerminationReason,
     *pReplyStatus = Status;
     return true;
 }
+
+void WriteData::MigrateUserBuffersOnTransitionToBackgroundWait(const void* /*oldBuffer*/, void* /*newBuffer*/)
+{
+    // WriteData does not hold API message buffers across a wait
+}

src/host/writeData.hpp

@@ -36,6 +36,7 @@ public:
 
     void SetUtf8ConsumedCharacters(const size_t cchUtf8Consumed);
 
+    void MigrateUserBuffersOnTransitionToBackgroundWait(const void* oldBuffer, void* newBuffer) override;
     bool Notify(const WaitTerminationReason TerminationReason,
                 const bool fIsUnicode,
                 _Out_ NTSTATUS* const pReplyStatus,

src/server/ApiMessage.cpp

@@ -194,3 +194,22 @@ void _CONSOLE_API_MSG::SetReplyInformation(const ULONG_PTR pInformation)
 {
     Complete.IoStatus.Information = pInformation;
 }
+
+void _CONSOLE_API_MSG::UpdateUserBufferPointers()
+{
+    // There are some instances where an API message may get copied.
+    // Because it is infeasible to write a copy constructor for this class
+    // without rewriting large swaths of conhost (because of the unnamed union)
+    // we have chosen to introduce a "post-copy" step.
+    // This makes sure the buffers in State are in sync with the actual
+    // buffers in the object.
+    if (State.InputBuffer)
+    {
+        State.InputBuffer = _inputBuffer.data();
+    }
+
+    if (State.OutputBuffer)
+    {
+        State.OutputBuffer = _outputBuffer.data();
+    }
+}

src/server/ApiMessage.h

@@ -82,6 +82,13 @@ public:
     void SetReplyStatus(const NTSTATUS Status);
     void SetReplyInformation(const ULONG_PTR pInformation);
 
+    // MSFT-33127449, GH#9692
+    // We are not writing a copy constructor for this class
+    // so as to scope a fix as narrowly as possible to the
+    // "invalid user buffer" crash.
+    // TODO GH#10076: remove this.
+    void UpdateUserBufferPointers();
+
     // DO NOT PUT MORE FIELDS DOWN HERE.
     // The tail end of this structure will have a console driver packet
     // copied over it and it will overwrite any fields declared here.

src/server/IWaitRoutine.h

@@ -39,6 +39,8 @@ public:
     {
     }
 
+    virtual void MigrateUserBuffersOnTransitionToBackgroundWait(const void* oldBuffer, void* newBuffer) = 0;
+
     virtual bool Notify(const WaitTerminationReason TerminationReason,
                         const bool fIsUnicode,
                         _Out_ NTSTATUS* const pReplyStatus,

src/server/WaitBlock.cpp

@@ -32,6 +32,44 @@ ConsoleWaitBlock::ConsoleWaitBlock(_In_ ConsoleWaitQueue* const pProcessQueue,
 {
     _WaitReplyMessage = *pWaitReplyMessage;
 
+    // MSFT-33127449, GH#9692
+    // Until there's a "Wait", there's only one API message inflight at a time. In our
+    // quest for performance, we put that single API message in charge of its own
+    // buffer management- instead of allocating buffers on the heap and deleting them
+    // later (storing pointers to them at the far corners of the earth), it would
+    // instead allocate them from small internal pools (if possible) and only heap
+    // allocate (transparently) if necessary. The pointers flung to the corners of the
+    // earth would be pointers (1) back into the API_MSG or (2) to a heap block owned
+    // by boost::small_vector.
+    //
+    // It took us months to realize that those bare pointers were being held by
+    // COOKED_READ and RAW_READ and not actually being updated when the API message was
+    // _copied_ as it was shuffled off to the background to become a "Wait" message.
+    //
+    // It turns out that it's trivially possible to crash the console by sending two
+    // API calls -- one that waits and one that completes immediately -- when the
+    // waiting message or the "wait completer" has a bunch of dangling pointers in it.
+    // Oops.
+    //
+    // Here, we fix up the message's internal pointers (in lieu of giving it a proper
+    // copy constructor; see GH#10076) and then tell the wait completion routine (which
+    // is going to be a COOKED_READ, RAW_READ, DirectRead or WriteData) about the new
+    // buffer location.
+    //
+    // This is a scoped fix that should be replaced (TODO GH#10076) with a final one
+    // after Ask mode.
+    _WaitReplyMessage.UpdateUserBufferPointers();
+
+    if (pWaitReplyMessage->State.InputBuffer)
+    {
+        _pWaiter->MigrateUserBuffersOnTransitionToBackgroundWait(pWaitReplyMessage->State.InputBuffer, _WaitReplyMessage.State.InputBuffer);
+    }
+
+    if (pWaitReplyMessage->State.OutputBuffer)
+    {
+        _pWaiter->MigrateUserBuffersOnTransitionToBackgroundWait(pWaitReplyMessage->State.OutputBuffer, _WaitReplyMessage.State.OutputBuffer);
+    }
+
     // We will write the original message back (with updated out parameters/payload) when the request is finally serviced.
     if (pWaitReplyMessage->Complete.Write.Data != nullptr)
     {