Skip to content

Commit

Permalink
Remove CONSOLE_API_MSG::UpdateUserBufferPointers hack (#10326)
Browse files Browse the repository at this point in the history 
## Summary of the Pull Request

This commit introduces a copy constructor/operator for
`_CONSOLE_API_MSG`. The change is not trivial as the struct contains a
union of unnamed structs that cannot be copied using regular language
features. As such a copy operator using `memcpy` was implemented.
Additionally all access specifiers were removed, as those allow a C++
compiler to reorder struct members. This would break message passing.
This commit is a good opportunity to prevent such miscompilations
proactively.

## Validation Steps Performed

* Command prompts of WSL2 fish-shell and pwsh still work ✔️

Closes #10076
  • Loading branch information
@lhecker
lhecker authored Jun 14, 2021
1 parent 1cc383f commit 296037a
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 84 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/allow/apis.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ ntprivapi
oaidl
ocidl
ODR
offsetof
osver
OSVERSIONINFOEXW
otms
Expand Down
74 changes: 44 additions & 30 deletions src/server/ApiMessage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,51 @@
#include "ApiMessage.h"
#include "DeviceComm.h"

_CONSOLE_API_MSG::_CONSOLE_API_MSG() :
Complete{ 0 },
State{ 0 },
_pDeviceComm(nullptr),
_pApiRoutines(nullptr),
_inputBuffer{},
_outputBuffer{},
Descriptor{ 0 },
CreateObject{ 0 },
CreateScreenBuffer{ 0 },
msgHeader{ 0 }
constexpr size_t structPacketDataSize = sizeof(_CONSOLE_API_MSG) - offsetof(_CONSOLE_API_MSG, Descriptor);

_CONSOLE_API_MSG::_CONSOLE_API_MSG()
{
// A union cannot have more than one initializer,
// but it isn't exactly clear which union case is the largest.
// --> Just memset() the entire thing.
memset(&Descriptor, 0, structPacketDataSize);
}

_CONSOLE_API_MSG::_CONSOLE_API_MSG(const _CONSOLE_API_MSG& other)
{
*this = other;
}

_CONSOLE_API_MSG& _CONSOLE_API_MSG::operator=(const _CONSOLE_API_MSG& other)
{
Complete = other.Complete;
State = other.State;
_pDeviceComm = other._pDeviceComm;
_pApiRoutines = other._pApiRoutines;
_inputBuffer = other._inputBuffer;
_outputBuffer = other._outputBuffer;

// Since this struct uses anonymous unions and thus cannot
// explicitly reference it, we have to a bit cheeky to copy it.
// --> Just memcpy() the entire thing.
memcpy(&Descriptor, &other.Descriptor, structPacketDataSize);

if (State.InputBuffer)
{
State.InputBuffer = _inputBuffer.data();
}

if (State.OutputBuffer)
{
State.OutputBuffer = _outputBuffer.data();
}

if (Complete.Write.Data)
{
Complete.Write.Data = &u;
}

return *this;
}

ConsoleProcessHandle* _CONSOLE_API_MSG::GetProcessHandle() const
Expand Down Expand Up @@ -194,22 +227,3 @@ 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();
}
}
64 changes: 31 additions & 33 deletions src/server/ApiMessage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,42 @@ typedef struct _CONSOLE_API_MSG
{
_CONSOLE_API_MSG();

CD_IO_COMPLETE Complete;
CONSOLE_API_STATE State;
_CONSOLE_API_MSG(_CONSOLE_API_MSG&&) = delete;
_CONSOLE_API_MSG& operator=(_CONSOLE_API_MSG&&) = delete;

IDeviceComm* _pDeviceComm;
IApiRoutines* _pApiRoutines;
_CONSOLE_API_MSG(const _CONSOLE_API_MSG& other);
_CONSOLE_API_MSG& operator=(const _CONSOLE_API_MSG&);

ConsoleProcessHandle* GetProcessHandle() const;
ConsoleHandleData* GetObjectHandle() const;

[[nodiscard]] HRESULT ReadMessageInput(const ULONG cbOffset, _Out_writes_bytes_(cbSize) PVOID pvBuffer, const ULONG cbSize);
[[nodiscard]] HRESULT GetAugmentedOutputBuffer(const ULONG cbFactor,
_Outptr_result_bytebuffer_(*pcbSize) PVOID* ppvBuffer,
_Out_ PULONG pcbSize);
[[nodiscard]] HRESULT GetOutputBuffer(_Outptr_result_bytebuffer_(*pcbSize) void** const ppvBuffer, _Out_ ULONG* const pcbSize);
[[nodiscard]] HRESULT GetInputBuffer(_Outptr_result_bytebuffer_(*pcbSize) void** const ppvBuffer, _Out_ ULONG* const pcbSize);

[[nodiscard]] HRESULT ReleaseMessageBuffers();

void SetReplyStatus(const NTSTATUS Status);
void SetReplyInformation(const ULONG_PTR pInformation);

// DO NOT PUT ACCESS SPECIFIERS HERE.
// The tail end of this structure is overwritten with console driver packet.
// It's important that we have a deterministic, C-like field ordering
// as this ensures that the packet data fields are last.
// Putting access specifiers here ("private:") would break this.

CD_IO_COMPLETE Complete{};
CONSOLE_API_STATE State{};

IDeviceComm* _pDeviceComm{ nullptr };
IApiRoutines* _pApiRoutines{ nullptr };

private:
boost::container::small_vector<BYTE, 128> _inputBuffer;
boost::container::small_vector<BYTE, 128> _outputBuffer;

public:
// From here down is the actual packet data sent/received.
CD_IO_DESCRIPTOR Descriptor;
union
Expand All @@ -62,33 +87,6 @@ typedef struct _CONSOLE_API_MSG
};
// End packet data

public:
// 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.

ConsoleProcessHandle* GetProcessHandle() const;
ConsoleHandleData* GetObjectHandle() const;

[[nodiscard]] HRESULT ReadMessageInput(const ULONG cbOffset, _Out_writes_bytes_(cbSize) PVOID pvBuffer, const ULONG cbSize);
[[nodiscard]] HRESULT GetAugmentedOutputBuffer(const ULONG cbFactor,
_Outptr_result_bytebuffer_(*pcbSize) PVOID* ppvBuffer,
_Out_ PULONG pcbSize);
[[nodiscard]] HRESULT GetOutputBuffer(_Outptr_result_bytebuffer_(*pcbSize) void** const ppvBuffer, _Out_ ULONG* const pcbSize);
[[nodiscard]] HRESULT GetInputBuffer(_Outptr_result_bytebuffer_(*pcbSize) void** const ppvBuffer, _Out_ ULONG* const pcbSize);

[[nodiscard]] HRESULT ReleaseMessageBuffers();

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.
Expand Down
25 changes: 4 additions & 21 deletions src/server/WaitBlock.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,9 @@ ConsoleWaitBlock::ConsoleWaitBlock(_In_ ConsoleWaitQueue* const pProcessQueue,
_In_ IWaitRoutine* const pWaiter) :
_pProcessQueue(THROW_HR_IF_NULL(E_INVALIDARG, pProcessQueue)),
_pObjectQueue(THROW_HR_IF_NULL(E_INVALIDARG, pObjectQueue)),
_WaitReplyMessage(*pWaitReplyMessage),
_pWaiter(THROW_HR_IF_NULL(E_INVALIDARG, pWaiter))
{
_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
Expand All @@ -51,14 +50,8 @@ ConsoleWaitBlock::ConsoleWaitBlock(_In_ ConsoleWaitQueue* const pProcessQueue,
// 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();
// Here, we tell the wait completion routine (which is going to be a COOKED_READ,
// RAW_READ, DirectRead or WriteData) about the new buffer location.

if (pWaitReplyMessage->State.InputBuffer)
{
Expand All @@ -69,12 +62,6 @@ ConsoleWaitBlock::ConsoleWaitBlock(_In_ ConsoleWaitQueue* const pProcessQueue,
{
_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)
{
_WaitReplyMessage.Complete.Write.Data = &_WaitReplyMessage.u;
}
}

// Routine Description:
Expand All @@ -85,11 +72,7 @@ ConsoleWaitBlock::~ConsoleWaitBlock()
{
_pProcessQueue->_blocks.erase(_itProcessQueue);
_pObjectQueue->_blocks.erase(_itObjectQueue);

if (_pWaiter != nullptr)
{
delete _pWaiter;
}
delete _pWaiter;
}

// Routine Description:
Expand Down

0 comments on commit 296037a

Please sign in to comment.