From 48f12cad166ea10deffbda0a4736c000e5ffcea1 Mon Sep 17 00:00:00 2001 From: Dan Alec Date: Fri, 16 Oct 2026 23:23:51 -0300 Subject: [PATCH 1/2] fix: check the two return values Code Analysis flags Two unchecked return values. On MSVC 18 both stop the build; on the pinned CI toolset neither surfaces. Neither defect is cosmetic. AvSetMmThreadCharacteristicsW returns NULL on failure, and that NULL went straight into AvRevertMmThreadCharacteristics, which does not accept it. Reported as C28183 and promoted to C2220 by TreatWarningAsError. The revert now happens only when the handle is valid; when the characteristics were never set there is nothing to revert, so that branch is a log and no call. CoCreateGuid had its HRESULT discarded (C6031 -> C2220). MonitorInfo is zero-initialised, so a failure left MonitorContainerId as GUID_NULL, which was then handed to IddCxMonitorCreate as if it were valid. The result is now checked, and on failure the driver logs and skips creating that monitor rather than registering it under GUID_NULL. Note how this presents: the first C2220 aborts the compile before the next warning is emitted, so silencing C28183 does not give a green build, it only moves the error to the next line. Both had to be fixed to find out whether anything was behind them. Verified: Debug and Release on x64 and ARM64, 0 errors, all four producing MttVDD.dll, MttVDD.inf and a signed mttvdd.cat. --- Virtual Display Driver (HDR)/MttVDD/Driver.cpp | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/Virtual Display Driver (HDR)/MttVDD/Driver.cpp b/Virtual Display Driver (HDR)/MttVDD/Driver.cpp index 3145b04a..9423f10b 100644 --- a/Virtual Display Driver (HDR)/MttVDD/Driver.cpp +++ b/Virtual Display Driver (HDR)/MttVDD/Driver.cpp @@ -3248,7 +3248,13 @@ void SwapChainProcessor::Run() /* AvRevertMmThreadCharacteristics(AvTaskHandle); */ //error handling when reversing multimedia thread characteristics - if (AvRevertMmThreadCharacteristics(AvTaskHandle)) + if (!AvTaskHandle) + { + logStream.str(""); + logStream << "Skipping revert: multimedia thread characteristics were never set."; + vddlog("d", logStream.str().c_str()); + } + else if (AvRevertMmThreadCharacteristics(AvTaskHandle)) { logStream.str(""); logStream << "Multimedia thread characteristics reverted successfully."; @@ -3832,7 +3838,13 @@ void IndirectDeviceContext::CreateMonitor(unsigned int index) { // ============================== // Create a container ID - CoCreateGuid(&MonitorInfo.MonitorContainerId); + HRESULT hrContainerId = CoCreateGuid(&MonitorInfo.MonitorContainerId); + if (FAILED(hrContainerId)) { + // MonitorInfo is zero-initialised, so MonitorContainerId would stay GUID_NULL + // and be handed to IddCxMonitorCreate as if it were valid. Bail out instead. + vddlog("e", "Failed to create container ID for monitor, skipping monitor creation"); + return; + } vddlog("d", "Created container ID"); IDARG_IN_MONITORCREATE MonitorCreate = {}; From 95ce18fa4cfef752aa40be02c862a2b49bb46157 Mon Sep 17 00:00:00 2001 From: Dan Alec Date: Tue, 20 Oct 2026 08:11:32 -0300 Subject: [PATCH 2/2] fix: guard shared state and add the checked parse helpers Two things the rest of the driver builds on: the shared state is actually guarded, and there is a way to parse a user-supplied number that does not throw out of a UMDF callback. No behaviour change. g_Running becomes std::atomic: the pipe server thread flips it from a command while the main path reads it. g_pipeHandle is only written through SetPipeHandle()/ClearPipeHandle(), under a mutex that is deliberately not g_Mutex. vddlog() is called from inside the pipe server while that thread holds g_Mutex, so SendToPipe() cannot take g_Mutex without deadlocking on it. ClearPipeHandle() returns the previous value so the caller can close it, and the handle is withdrawn before the close. s_KnownMonitorModesMutex protects a std::vector that two IddCx callbacks can rewrite at the same time -- one clears and refills it while the other indexes it. That is a live data race, not a stale read. Readers take a snapshot under the lock instead of holding it across the IddCx calls that follow. g_MetadataMutex guards the HDR and gamma stores, which the settings loader and the IddCx callbacks both write. vdd_settings.xml, option.txt and monitor_profile.xml are user-editable, and loadSettings() runs inside the DeviceAdd callback. std::stoi and std::stof throw on a malformed value, and an exception leaving a UMDF callback crashes the host. ParseIntSafe, ParseFloatSafe and ParseDoubleSafe log the offending value and return the caller's fallback instead, and they reject trailing garbage. ParseDoubleSafe exists because the EDID chromaticity fields are double: redX 0.640 read as float and stored back as double is 0.63999998569488525390625, which then differs from the 0.640 the same file computes elsewhere. ClampToUINT16 exists because a light level above 65535 wrapped silently on the cast. Verified: Debug and Release on x64 and ARM64, 0 errors, all four producing MttVDD.dll, MttVDD.inf and a signed mttvdd.cat. --- .../MttVDD/Driver.cpp | 182 +++++++++++++++++- 1 file changed, 177 insertions(+), 5 deletions(-) diff --git a/Virtual Display Driver (HDR)/MttVDD/Driver.cpp b/Virtual Display Driver (HDR)/MttVDD/Driver.cpp index 9423f10b..92527454 100644 --- a/Virtual Display Driver (HDR)/MttVDD/Driver.cpp +++ b/Virtual Display Driver (HDR)/MttVDD/Driver.cpp @@ -26,6 +26,8 @@ Copyright (c) Microsoft Corporation #include #include #include +#include +#include #include #include #include @@ -50,16 +52,153 @@ Copyright (c) Microsoft Corporation #pragma comment(lib, "shlwapi.lib") HANDLE hPipeThread = NULL; -bool g_Running = true; + +// How long the pipe server waits for a client to connect, and how long it waits for that +// client to send a command, before looping back to re-check g_Running. Both used to block +// forever, which is what made EvtDriverUnload hang: it waits on the server thread. +constexpr DWORD PIPE_CONNECT_POLL_MS = 1000; +constexpr DWORD PIPE_READ_POLL_MS = 5000; +// Upper bound on waiting for the server thread to wind down at unload. Past this the thread is +// left to finish on its own; it will notice g_Running within PIPE_CONNECT_POLL_MS. +constexpr DWORD PIPE_SHUTDOWN_WAIT_MS = 5000; + +std::atomic g_Running{ true }; // written under g_Mutex but read without it in the server loop mutex g_Mutex; +// Guards g_pipeHandle only. A separate mutex from g_Mutex on purpose: vddlog() is called from +// inside the pipe server while that thread holds g_Mutex, so SendToPipe() cannot take g_Mutex +// without deadlocking on it. +mutex g_PipeMutex; HANDLE g_pipeHandle = INVALID_HANDLE_VALUE; +// Publish or withdraw the pipe handle. The only sanctioned writers of g_pipeHandle. +void SetPipeHandle(HANDLE hPipe) { + lock_guard lock(g_PipeMutex); + g_pipeHandle = hPipe; +} + +// Withdraw the pipe handle and hand the previous value back so the caller can close it. +HANDLE ClearPipeHandle() { + lock_guard lock(g_PipeMutex); + HANDLE previous = g_pipeHandle; + g_pipeHandle = INVALID_HANDLE_VALUE; // Whether data goes to the pipe or just the log file + return previous; +} + +// Guards g_GammaRampStore and g_HdrMetadataStore. They are written by the settings loader and +// by the IddCx HDR/gamma callbacks, and std::map is not safe to read while another thread +// inserts. Distinct from g_Mutex because those callbacks call vddlog() along the way. +mutex g_MetadataMutex; + using namespace std; using namespace Microsoft::IndirectDisp; using namespace Microsoft::WRL; void vddlog(const char* type, const char* message); +// Defined below, but needed by the EDID mode generator which runs earlier in this file. +void float_to_vsync(float refresh_rate, int& num, int& den); + +// vdd_settings.xml, option.txt and monitor_profile.xml are user-editable files, and loadSettings() +// runs inside the DeviceAdd callback. std::stoi/stof throw std::invalid_argument or +// std::out_of_range on a malformed value, and an exception leaving a UMDF callback crashes the +// host. Parse defensively instead: log the offending value and keep the caller's fallback. +// These light levels come straight from vdd_settings.xml as int. A value above 65535 wrapped +// silently on the cast, so a typo in the XML turned into a plausible-looking but wrong number. +UINT16 ClampToUINT16(int value) { + if (value < 0) { return 0; } + if (value > 65535) { return 65535; } + return static_cast(value); +} + +// vdd_settings.xml and monitor_profile.xml are wstring, option.txt is std::string. +template +int ParseIntSafe(const T& text, int fallback, const wchar_t* what) { + try { + size_t consumed = 0; + int value = stoi(text, &consumed); + if (consumed != text.size()) { + stringstream ss; + ss << "Trailing garbage in " << WStringToString(what) << ", using fallback"; + vddlog("e", ss.str().c_str()); + return fallback; + } + return value; + } + catch (const exception&) { + stringstream ss; + ss << "Malformed " << WStringToString(what) << " value, using fallback"; + vddlog("e", ss.str().c_str()); + return fallback; + } +} + +template +float ParseFloatSafe(const T& text, float fallback, const wchar_t* what) { + try { + size_t consumed = 0; + float value = stof(text, &consumed); + if (consumed != text.size()) { + stringstream ss; + ss << "Trailing garbage in " << WStringToString(what) << ", using fallback"; + vddlog("e", ss.str().c_str()); + return fallback; + } + return value; + } + catch (const exception&) { + stringstream ss; + ss << "Malformed " << WStringToString(what) << " value, using fallback"; + vddlog("e", ss.str().c_str()); + return fallback; + } +} + +// Same contract as ParseFloatSafe, but in double. The EdidProfileData chromaticity and +// luminance fields are double, and ParseFloatSafe would quietly round them: redX 0.640 read +// as float and stored back as double is 0.63999998569488525390625, which then differs from the +// 0.640 the same file computes elsewhere. +template +double ParseDoubleSafe(const T& text, double fallback, const wchar_t* what) { + try { + size_t consumed = 0; + double value = stod(text, &consumed); + if (consumed != text.size()) { + stringstream ss; + ss << "Trailing garbage in " << WStringToString(what) << ", using fallback"; + vddlog("e", ss.str().c_str()); + return fallback; + } + return value; + } + catch (const exception&) { + stringstream ss; + ss << "Malformed " << WStringToString(what) << " value, using fallback"; + vddlog("e", ss.str().c_str()); + return fallback; + } +} + +// option.txt is a hand-editable file and its mode lines are written "640, 480, 60", with +// spaces after the commas and whatever else a person types. ParseIntSafe and ParseFloatSafe +// count anything after the number as trailing garbage and fall back -- stof(" 480 ") consumes +// the leading space and stops at the trailing one, so a stray trailing space silently turns a +// hand-written refresh rate into the 60 Hz default and logs an error per mode. Trim here rather +// than loosening the trailing-garbage check in the two parsers, which the XML and +// monitor_profile paths use to catch genuinely malformed values. +// +// Note the CR is not one of these: the file is CRLF, and std::ifstream in text mode (the +// default, and what this code opens) translates CRLF to LF, so getline never sees it. Opening +// the file with ios::binary is what would reintroduce it. +string TrimAsciiSpace(const string& text) { + const char* blanks = " \t\r\n"; + size_t first = text.find_first_not_of(blanks); + if (first == string::npos) { + return string(); + } + size_t last = text.find_last_not_of(blanks); + return text.substr(first, last - first + 1); +} + extern "C" DRIVER_INITIALIZE DriverEntry; EVT_WDF_DRIVER_DEVICE_ADD VirtualDisplayDriverDeviceAdd; @@ -89,18 +228,37 @@ struct } Options; vector> monitorModes; vector< DISPLAYCONFIG_VIDEO_SIGNAL_INFO> s_KnownMonitorModes2; +// Guards s_KnownMonitorModes2. Both VirtualDisplayDriverParseMonitorDescription and +// VirtualDisplayDriverParseMonitorDescription2 are IddCx callbacks, and both call +// RebuildKnownMonitorModesCache. IddCx can run them concurrently, so one can clear and refill +// this vector while the other is indexing it -- a live data race on a std::vector, which is UB, +// not merely a stale read. Readers take a snapshot under this lock rather than holding it across +// the IddCx calls that follow. +mutex s_KnownMonitorModesMutex; UINT numVirtualDisplays; +// How many virtual displays one adapter may carry. numVirtualDisplays is a UINT feeding +// AdapterCaps.MaxMonitorsSupported and a CreateMonitor() loop, so a count taken from XML or +// from the pipe has to be bounded before either sees it. One display is what the driver +// ships with; a handful is what IddCx realistically carries on one indirect adapter. +constexpr UINT MIN_VIRTUAL_DISPLAYS = 1; +constexpr UINT MAX_VIRTUAL_DISPLAYS = 16; wstring gpuname; wstring confpath = L"C:\\VirtualDisplayDriver"; -bool logsEnabled = false; -bool debugLogs = false; +// Atomic because the pipe thread flips these from a command while every other thread reads +// them on each log line. They were plain bool, which is a data race in the formal model and +// in practice can hand vddlog a torn decision about whether to log at all. +std::atomic logsEnabled{ false }; +std::atomic debugLogs{ false }; bool HDRPlus = false; bool SDR10 = false; bool customEdid = false; -bool hardwareCursor = false; +// The compiled value only matters on the path with no vdd_settings.xml, because DriverEntry +// overwrites it unconditionally on every other path. HardwareCursor ships as true in that file, +// so true is what this has to say; EnabledOr below is what makes it survive a missing key. +bool hardwareCursor = true; bool preventManufacturerSpoof = false; bool edidCeaOverride = false; -bool sendLogsThroughPipe = true; +std::atomic sendLogsThroughPipe{ true }; constexpr DISPLAYCONFIG_VIDEO_SIGNAL_INFO dispinfo(UINT32 h, UINT32 v, UINT32 rn, UINT32 rd); @@ -108,6 +266,7 @@ namespace { void RebuildKnownMonitorModesCache() { + lock_guard lock(s_KnownMonitorModesMutex); s_KnownMonitorModes2.clear(); s_KnownMonitorModes2.reserve(monitorModes.size()); @@ -121,6 +280,19 @@ namespace std::get<3>(mode))); } } + + // The two readers below are IddCx callbacks that go on to fill IddCx-owned buffers, so the + // lock is held only for the copy. A snapshot is a few dozen DISPLAYCONFIG_VIDEO_SIGNAL_INFO + // at most, which is cheap next to the rest of the callback. + vector SnapshotKnownMonitorModes() { + lock_guard lock(s_KnownMonitorModesMutex); + return s_KnownMonitorModes2; + } + + size_t KnownMonitorModesCount() { + lock_guard lock(s_KnownMonitorModesMutex); + return s_KnownMonitorModes2.size(); + } } //Mouse settings