maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Convert DeviceComm into an interface and add handle exchange (#8367)

Browse source 
This commit replaces DeviceComm with the interface IDeviceComm and the
concrete implementation type ConDrvDeviceComm. This work is done in
preparation for different device backends.

In addition to separating out ConDrv-specific behavior, I've introduced
a "handle exchange" interface.

HANDLE EXCHANGE
---------------

There are points where we give ConDrv opaque handle identifiers to our
input buffer, output buffer and process data. The exact content of the
opaque identifier is meaningless to ConDrv: the driver's only
interaction with these identifiers is to hold onto them and send back
whichever are pertinent for each API call.

Because of that, we used the raw register-width pointer value of the
input buffer, output buffer or process data _as_ the opaque handle
value.

This works very well for ConDrv <-> conhost using the ConDrvDeviceComm.
It works less well for something like the "logging" DeviceComm that will
log packets to a file: those packets *cannot* contain pointer values (!)

To address this, and to afford flexibility to DeviceComm implementers,
I've introduced a two-member complement of handle management functions:

* `ULONG_PTR PutHandle(void*)` registers an object with the DeviceComm
  and returns an opaque identifier.
* `void* GetHandle(ULONG_PTR)` takes an opaque identifier and returns
  the original object.

ConDrvDeviceComm implements PutHandle and GetHandle by casting the
object pointer to the "opaque handle value", which maintains wire format
compatibility[1] with revisions of conhost prior to this change.

Simpler DeviceComm implementations that require handle tracking but
cannot bear raw pointers can implement these functions by returning an
autoincrementing ID (or similar) and registering the raw object pointer
in a mapping.

I've audited all existing handle exchanges with the driver and updated
them to use Put/GetHandle.

(I intended to add DestroyHandle, but we are not equipped for handle
removal at the moment. ConsoleHandleData/ConsoleProcessHandle are
destroyed during wait routine completion, on client disconnect, etc.
This does mean that an id<->pointer mapping will grow without bound, but
at a cost of ~8 bytes per entry and a short-lived console session I am
not too concerned about the cost.)

[1] Wire format compatibility is not required, and later we may want to
switch ConDrvDeviceComm to `EncodePointer` and `DecodePointer` just to
insulate us against a spurious ConDrv packet allowing for an arbitrary
4/8-byte read and subsequent liftoff into space.
This commit is contained in:
Dustin L. Howett 2020-12-15 15:07:43 -08:00 committed by GitHub
parent b140299e50
commit fb3d772615
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
14 changed files with 131 additions and 55 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
.github/actions/spell-check/expect/expect.txt vendored
View file

@ -1042,6 +1042,7 @@ IData
IDCANCEL
IDD
IDesktop
IDevice
IDictionary
IDISHWND
IDisposable

3
src/host/globals.h
View file

@ -28,6 +28,7 @@ Revision History:
#include "../renderer/inc/IFontDefaultList.hpp"
#include "../server/DeviceComm.h"
#include "../server/ConDrvDeviceComm.h"
#include <TraceLoggingProvider.h>
#include <winmeta.h>
@ -45,7 +46,7 @@ public:
CONSOLE_INFORMATION& getConsoleInformation();
DeviceComm* pDeviceComm;
IDeviceComm* pDeviceComm;
wil::unique_event_nothrow hInputEvent;

2
src/host/srvinit.cpp
View file

@ -37,7 +37,7 @@ const UINT CONSOLE_LPC_PORT_FAILURE_ID = 21791;
try
{
Globals.pDeviceComm = new DeviceComm(Server);
Globals.pDeviceComm = new ConDrvDeviceComm(Server);
Globals.launchArgs = *args;

4
src/server/ApiMessage.cpp
View file

@ -17,12 +17,12 @@ _CONSOLE_API_MSG::_CONSOLE_API_MSG() :
ConsoleProcessHandle* _CONSOLE_API_MSG::GetProcessHandle() const
{
return reinterpret_cast<ConsoleProcessHandle*>(Descriptor.Process);
return reinterpret_cast<ConsoleProcessHandle*>(_pDeviceComm->GetHandle(Descriptor.Process));
}
ConsoleHandleData* _CONSOLE_API_MSG::GetObjectHandle() const
{
return reinterpret_cast<ConsoleHandleData*>(Descriptor.Object);
return reinterpret_cast<ConsoleHandleData*>(_pDeviceComm->GetHandle(Descriptor.Object));
}
// Routine Description:

4
src/server/ApiMessage.h
View file

@ -23,7 +23,7 @@ Revision History:
class ConsoleProcessHandle;
class ConsoleHandleData;
class DeviceComm;
class IDeviceComm;
typedef struct _CONSOLE_API_MSG
{
@ -32,7 +32,7 @@ typedef struct _CONSOLE_API_MSG
CD_IO_COMPLETE Complete;
CONSOLE_API_STATE State;
DeviceComm* _pDeviceComm;
IDeviceComm* _pDeviceComm;
IApiRoutines* _pApiRoutines;
// From here down is the actual packet data sent/received.

53
src/server/DeviceComm.cpp → src/server/ConDrvDeviceComm.cpp
View file

@ -2,15 +2,15 @@
// Licensed under the MIT license.
#include "precomp.h"
#include "DeviceComm.h"
#include "ConDrvDeviceComm.h"
DeviceComm::DeviceComm(_In_ HANDLE Server) :
ConDrvDeviceComm::ConDrvDeviceComm(_In_ HANDLE Server) :
_Server(Server)
{
THROW_HR_IF(E_HANDLE, Server == INVALID_HANDLE_VALUE);
}
DeviceComm::~DeviceComm()
ConDrvDeviceComm::~ConDrvDeviceComm()
{
}
@ -22,7 +22,7 @@ DeviceComm::~DeviceComm()
// - pServerInfo - Structure containing information required to initialize driver state for this console connection.
// Return Value:
// - HRESULT S_OK or suitable error.
[[nodiscard]] HRESULT DeviceComm::SetServerInformation(_In_ CD_IO_SERVER_INFORMATION* const pServerInfo) const
[[nodiscard]] HRESULT ConDrvDeviceComm::SetServerInformation(_In_ CD_IO_SERVER_INFORMATION* const pServerInfo) const
{
return _CallIoctl(IOCTL_CONDRV_SET_SERVER_INFORMATION,
pServerInfo,
@ -38,8 +38,8 @@ DeviceComm::~DeviceComm()
// - pMessage - A structure to hold the message data retrieved from the driver.
// Return Value:
// - HRESULT S_OK or suitable error.
[[nodiscard]] HRESULT DeviceComm::ReadIo(_In_opt_ PCONSOLE_API_MSG const pReplyMsg,
_Out_ CONSOLE_API_MSG* const pMessage) const
[[nodiscard]] HRESULT ConDrvDeviceComm::ReadIo(_In_opt_ PCONSOLE_API_MSG const pReplyMsg,
_Out_ CONSOLE_API_MSG* const pMessage) const
{
HRESULT hr = _CallIoctl(IOCTL_CONDRV_READ_IO,
pReplyMsg == nullptr ? nullptr : &pReplyMsg->Complete,
@ -62,7 +62,7 @@ DeviceComm::~DeviceComm()
// - pCompletion - Completion structure from the previous activity (can be used in lieu of calling CompleteIo separately.)
// Return Value:
// - HRESULT S_OK or suitable error.
[[nodiscard]] HRESULT DeviceComm::CompleteIo(_In_ CD_IO_COMPLETE* const pCompletion) const
[[nodiscard]] HRESULT ConDrvDeviceComm::CompleteIo(_In_ CD_IO_COMPLETE* const pCompletion) const
{
return _CallIoctl(IOCTL_CONDRV_COMPLETE_IO,
pCompletion,
@ -78,7 +78,7 @@ DeviceComm::~DeviceComm()
// to hold retrieved buffered input data from the client application.
// Return Value:
// - HRESULT S_OK or suitable error.
[[nodiscard]] HRESULT DeviceComm::ReadInput(_In_ CD_IO_OPERATION* const pIoOperation) const
[[nodiscard]] HRESULT ConDrvDeviceComm::ReadInput(_In_ CD_IO_OPERATION* const pIoOperation) const
{
return _CallIoctl(IOCTL_CONDRV_READ_INPUT,
pIoOperation,
@ -94,7 +94,7 @@ DeviceComm::~DeviceComm()
// to hold buffered output data to be sent to the client application.
// Return Value:
// - HRESULT S_OK or suitable error.
[[nodiscard]] HRESULT DeviceComm::WriteOutput(_In_ CD_IO_OPERATION* const pIoOperation) const
[[nodiscard]] HRESULT ConDrvDeviceComm::WriteOutput(_In_ CD_IO_OPERATION* const pIoOperation) const
{
return _CallIoctl(IOCTL_CONDRV_WRITE_OUTPUT,
pIoOperation,
@ -110,7 +110,7 @@ DeviceComm::~DeviceComm()
// - <none>
// Return Value:
// - HRESULT S_OK or suitable error.
[[nodiscard]] HRESULT DeviceComm::AllowUIAccess() const
[[nodiscard]] HRESULT ConDrvDeviceComm::AllowUIAccess() const
{
return _CallIoctl(IOCTL_CONDRV_ALLOW_VIA_UIACCESS,
nullptr,
@ -130,11 +130,11 @@ DeviceComm::~DeviceComm()
// - cbOutBufferSize - The length in bytes of the optional output buffer.
// Return Value:
// - HRESULT S_OK or suitable error.
[[nodiscard]] HRESULT DeviceComm::_CallIoctl(_In_ DWORD dwIoControlCode,
_In_reads_bytes_opt_(cbInBufferSize) PVOID pInBuffer,
_In_ DWORD cbInBufferSize,
_Out_writes_bytes_opt_(cbOutBufferSize) PVOID pOutBuffer,
_In_ DWORD cbOutBufferSize) const
[[nodiscard]] HRESULT ConDrvDeviceComm::_CallIoctl(_In_ DWORD dwIoControlCode,
_In_reads_bytes_opt_(cbInBufferSize) PVOID pInBuffer,
_In_ DWORD cbInBufferSize,
_Out_writes_bytes_opt_(cbOutBufferSize) PVOID pOutBuffer,
_In_ DWORD cbOutBufferSize) const
{
// See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa363216(v=vs.85).aspx
// Written is unused but cannot be nullptr because we aren't using overlapped.
@ -150,3 +150,26 @@ DeviceComm::~DeviceComm()
return S_OK;
}
// Routine Description:
// - Implements IDeviceComm handle exchange for ConDrv.
// - "Translates" a pointer to an object into a handle value
// that the driver can use to identify objects in a console
// session.
// - The opposite of GetHandle
[[nodiscard]] ULONG_PTR ConDrvDeviceComm::PutHandle(const void* handle)
{
// ConDrv will pass back whatever large integer we send it, as an opaque data blob
// We'll use that to smuggle the actual pointer value to the handle.
return reinterpret_cast<ULONG_PTR>(handle);
}
// Routine Description:
// - Implements IDeviceComm handle exchange for ConDrv.
// - "Translates" an object handle from ConDrv into
// a pointer to an object
// - The opposite of PutHandle
[[nodiscard]] void* ConDrvDeviceComm::GetHandle(ULONG_PTR handleId) const
{
return reinterpret_cast<void*>(handleId);
}

51
src/server/ConDrvDeviceComm.h Normal file
View file

@ -0,0 +1,51 @@
/*++
Copyright (c) Microsoft Corporation
Licensed under the MIT license.
Module Name:
- ConDrvDeviceComm.h
Abstract:
- This module assists in communicating via IOCTL messages to and from a Device server handle.
Author:
- Michael Niksa (MiNiksa) 14-Sept-2016
Revision History:
2020-04 split into an interface IDeviceComm and a concrete impl for ConDrv
--*/
#pragma once
#include "DeviceComm.h"
#include <wil\resource.h>
class ConDrvDeviceComm : public IDeviceComm
{
public:
ConDrvDeviceComm(_In_ HANDLE Server);
~ConDrvDeviceComm();
[[nodiscard]] HRESULT SetServerInformation(_In_ CD_IO_SERVER_INFORMATION* const pServerInfo) const override;
[[nodiscard]] HRESULT ReadIo(_In_opt_ PCONSOLE_API_MSG const pReplyMsg,
_Out_ CONSOLE_API_MSG* const pMessage) const override;
[[nodiscard]] HRESULT CompleteIo(_In_ CD_IO_COMPLETE* const pCompletion) const override;
[[nodiscard]] HRESULT ReadInput(_In_ CD_IO_OPERATION* const pIoOperation) const override;
[[nodiscard]] HRESULT WriteOutput(_In_ CD_IO_OPERATION* const pIoOperation) const override;
[[nodiscard]] HRESULT AllowUIAccess() const override;
[[nodiscard]] ULONG_PTR PutHandle(const void*) override;
[[nodiscard]] void* GetHandle(ULONG_PTR) const override;
private:
[[nodiscard]] HRESULT _CallIoctl(_In_ DWORD dwIoControlCode,
_In_reads_bytes_opt_(cbInBufferSize) PVOID pInBuffer,
_In_ DWORD cbInBufferSize,
_Out_writes_bytes_opt_(cbOutBufferSize) PVOID pOutBuffer,
_In_ DWORD cbOutBufferSize) const;
wil::unique_handle _Server;
};

35
src/server/DeviceComm.h
View file

@ -6,10 +6,10 @@ Module Name:
- DeviceComm.h
Abstract:
- This module assists in communicating via IOCTL messages to and from a Device server handle.
- This module assists in communicating via IOCTL messages to and from an endpoint
Author:
- Michael Niksa (MiNiksa) 14-Sept-2016
- Dustin Howett (DuHowett) 10-Apr-2020
Revision History:
--*/
@ -18,30 +18,21 @@ Revision History:
#include "../host/conapi.h"
#include <wil/resource.h>
class DeviceComm
class IDeviceComm
{
public:
DeviceComm(_In_ HANDLE Server);
~DeviceComm();
virtual ~IDeviceComm() = default;
[[nodiscard]] HRESULT SetServerInformation(_In_ CD_IO_SERVER_INFORMATION* const pServerInfo) const;
[[nodiscard]] HRESULT ReadIo(_In_opt_ PCONSOLE_API_MSG const pReplyMsg,
_Out_ CONSOLE_API_MSG* const pMessage) const;
[[nodiscard]] HRESULT CompleteIo(_In_ CD_IO_COMPLETE* const pCompletion) const;
[[nodiscard]] virtual HRESULT SetServerInformation(_In_ CD_IO_SERVER_INFORMATION* const pServerInfo) const = 0;
[[nodiscard]] virtual HRESULT ReadIo(_In_opt_ PCONSOLE_API_MSG const pReplyMsg,
_Out_ CONSOLE_API_MSG* const pMessage) const = 0;
[[nodiscard]] virtual HRESULT CompleteIo(_In_ CD_IO_COMPLETE* const pCompletion) const = 0;
[[nodiscard]] HRESULT ReadInput(_In_ CD_IO_OPERATION* const pIoOperation) const;
[[nodiscard]] HRESULT WriteOutput(_In_ CD_IO_OPERATION* const pIoOperation) const;
[[nodiscard]] virtual HRESULT ReadInput(_In_ CD_IO_OPERATION* const pIoOperation) const = 0;
[[nodiscard]] virtual HRESULT WriteOutput(_In_ CD_IO_OPERATION* const pIoOperation) const = 0;
[[nodiscard]] HRESULT AllowUIAccess() const;
[[nodiscard]] virtual HRESULT AllowUIAccess() const = 0;
private:
[[nodiscard]] HRESULT _CallIoctl(_In_ DWORD dwIoControlCode,
_In_reads_bytes_opt_(cbInBufferSize) PVOID pInBuffer,
_In_ DWORD cbInBufferSize,
_Out_writes_bytes_opt_(cbOutBufferSize) PVOID pOutBuffer,
_In_ DWORD cbOutBufferSize) const;
wil::unique_handle _Server;
[[nodiscard]] virtual ULONG_PTR PutHandle(const void*) = 0;
[[nodiscard]] virtual void* GetHandle(ULONG_PTR) const = 0;
};

11
src/server/IoDispatchers.cpp
View file

@ -89,11 +89,13 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleCreateObject(_In_ PCONSOLE_API_MSG pMessa
goto Error;
}
auto deviceComm{ ServiceLocator::LocateGlobals().pDeviceComm };
// Complete the request.
pMessage->SetReplyStatus(STATUS_SUCCESS);
pMessage->SetReplyInformation(reinterpret_cast<ULONG_PTR>(handle.get()));
pMessage->SetReplyInformation(deviceComm->PutHandle(handle.get()));
if (SUCCEEDED(ServiceLocator::LocateGlobals().pDeviceComm->CompleteIo(&pMessage->Complete)))
if (SUCCEEDED(deviceComm->CompleteIo(&pMessage->Complete)))
{
// We've successfully transferred ownership of the handle to the driver. We can release and not free it.
handle.release();
@ -228,11 +230,12 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleHandleConnectionRequest(_In_ PCONSOLE_API
pReceiveMsg->SetReplyStatus(STATUS_SUCCESS);
pReceiveMsg->SetReplyInformation(sizeof(CD_CONNECTION_INFORMATION));
CD_CONNECTION_INFORMATION ConnectionInformation = ProcessData->GetConnectionInformation();
auto deviceComm{ ServiceLocator::LocateGlobals().pDeviceComm };
CD_CONNECTION_INFORMATION ConnectionInformation = ProcessData->GetConnectionInformation(deviceComm);
pReceiveMsg->Complete.Write.Data = &ConnectionInformation;
pReceiveMsg->Complete.Write.Size = sizeof(CD_CONNECTION_INFORMATION);
if (FAILED(ServiceLocator::LocateGlobals().pDeviceComm->CompleteIo(&pReceiveMsg->Complete)))
if (FAILED(deviceComm->CompleteIo(&pReceiveMsg->Complete)))
{
CommandHistory::s_Free((HANDLE)ProcessData);
gci.ProcessHandleList.FreeProcessData(ProcessData);

14
src/server/ProcessHandle.cpp
View file

@ -35,12 +35,18 @@ ConsoleProcessHandle::ConsoleProcessHandle(const DWORD dwProcessId,
}
}
CD_CONNECTION_INFORMATION ConsoleProcessHandle::GetConnectionInformation() const
// Routine Description:
// - Creates a CD_CONNECTION_INFORMATION (packet) that communicates the
// process, input and output handles to the driver as transformed by the
// DeviceComm's handle exchanger.
// Arguments:
// - deviceComm: IDeviceComm implementation with which to exchange handles.
CD_CONNECTION_INFORMATION ConsoleProcessHandle::GetConnectionInformation(IDeviceComm* deviceComm) const
{
CD_CONNECTION_INFORMATION result = { 0 };
result.Process = reinterpret_cast<ULONG_PTR>(this);
result.Input = reinterpret_cast<ULONG_PTR>(pInputHandle.get());
result.Output = reinterpret_cast<ULONG_PTR>(pOutputHandle.get());
result.Process = deviceComm->PutHandle(this);
result.Input = deviceComm->PutHandle(pInputHandle.get());
result.Output = deviceComm->PutHandle(pOutputHandle.get());
return result;
}

2
src/server/ProcessHandle.h
View file

@ -40,7 +40,7 @@ public:
const ConsoleProcessPolicy GetPolicy() const;
const ConsoleShimPolicy GetShimPolicy() const;
CD_CONNECTION_INFORMATION GetConnectionInformation() const;
CD_CONNECTION_INFORMATION GetConnectionInformation(IDeviceComm* deviceComm) const;
private:
ConsoleProcessHandle(const DWORD dwProcessId,

2
src/server/lib/server.vcxproj
View file

@ -15,8 +15,8 @@
<ClCompile Include="..\ApiMessage.cpp" />
<ClCompile Include="..\ApiMessageState.cpp" />
<ClCompile Include="..\ApiSorter.cpp" />
<ClCompile Include="..\ConDrvDeviceComm.cpp" />
<ClCompile Include="..\ConsoleShimPolicy.cpp" />
<ClCompile Include="..\DeviceComm.cpp" />
<ClCompile Include="..\DeviceHandle.cpp" />
<ClCompile Include="..\Entrypoints.cpp" />
<ClCompile Include="..\IoDispatchers.cpp" />

2
src/server/lib/server.vcxproj.filters
View file

@ -27,7 +27,7 @@
<ClCompile Include="..\WinNTControl.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="..\DeviceComm.cpp">
<ClCompile Include="..\ConDrvDeviceComm.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="..\ObjectHeader.cpp">

2
src/server/sources.inc
View file

@ -37,7 +37,7 @@ SOURCES= \
..\ApiMessage.cpp \
..\ApiMessageState.cpp \
..\ApiSorter.cpp \
..\DeviceComm.cpp \
..\ConDrvDeviceComm.cpp \
..\DeviceHandle.cpp \
..\ConsoleShimPolicy.cpp \
..\Entrypoints.cpp \