From c0ab9cb5b55d745a1528c7429615c71e0745ed8d Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Tue, 11 May 2021 16:56:43 +0000 Subject: [PATCH] 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 | 1 + src/host/readDataCooked.cpp | 9 +++++++++ src/host/readDataCooked.hpp | 2 ++ src/host/readDataDirect.cpp | 5 +++++ src/host/readDataDirect.hpp | 1 + src/host/readDataRaw.cpp | 9 +++++++++ src/host/readDataRaw.hpp | 1 + src/host/writeData.cpp | 5 +++++ src/host/writeData.hpp | 1 + src/server/ApiMessage.cpp | 19 +++++++++++++++++++ src/server/ApiMessage.h | 7 +++++++ src/server/IWaitRoutine.h | 2 ++ src/server/WaitBlock.cpp | 38 +++++++++++++++++++++++++++++++++++++ 13 files changed, 100 insertions(+) diff --git a/src/host/readData.hpp b/src/host/readData.hpp index 41471cae2..162116c44 100644 --- a/src/host/readData.hpp +++ b/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, diff --git a/src/host/readDataCooked.cpp b/src/host/readDataCooked.cpp index 60a817f15..641ca12b0 100644 --- a/src/host/readDataCooked.cpp +++ b/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(oldBuffer)) + { + _userBuffer = reinterpret_cast(newBuffer); + } +} diff --git a/src/host/readDataCooked.hpp b/src/host/readDataCooked.hpp index 5080de5d3..47f7bb2e8 100644 --- a/src/host/readDataCooked.hpp +++ b/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, diff --git a/src/host/readDataDirect.cpp b/src/host/readDataDirect.cpp index c8655fc4a..d6a160d50 100644 --- a/src/host/readDataDirect.cpp +++ b/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 +} diff --git a/src/host/readDataDirect.hpp b/src/host/readDataDirect.hpp index 26006db53..d1ad2e503 100644 --- a/src/host/readDataDirect.hpp +++ b/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, diff --git a/src/host/readDataRaw.cpp b/src/host/readDataRaw.cpp index 896f5eb06..706be8768 100644 --- a/src/host/readDataRaw.cpp +++ b/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(oldBuffer)) + { + _BufPtr = reinterpret_cast(newBuffer); + } +} diff --git a/src/host/readDataRaw.hpp b/src/host/readDataRaw.hpp index a62dcce2d..4bb97ab8d 100644 --- a/src/host/readDataRaw.hpp +++ b/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, diff --git a/src/host/writeData.cpp b/src/host/writeData.cpp index 6aede0626..2de6940b2 100644 --- a/src/host/writeData.cpp +++ b/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 +} diff --git a/src/host/writeData.hpp b/src/host/writeData.hpp index b61cd5690..b23e12ad4 100644 --- a/src/host/writeData.hpp +++ b/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, diff --git a/src/server/ApiMessage.cpp b/src/server/ApiMessage.cpp index 439778656..4039a2a0c 100644 --- a/src/server/ApiMessage.cpp +++ b/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(); + } +} diff --git a/src/server/ApiMessage.h b/src/server/ApiMessage.h index 8ff2857f3..828d6358a 100644 --- a/src/server/ApiMessage.h +++ b/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. diff --git a/src/server/IWaitRoutine.h b/src/server/IWaitRoutine.h index bc308f2e4..48082f5ab 100644 --- a/src/server/IWaitRoutine.h +++ b/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, diff --git a/src/server/WaitBlock.cpp b/src/server/WaitBlock.cpp index a23cb52c0..100055f88 100644 --- a/src/server/WaitBlock.cpp +++ b/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) {