From 48f12cad166ea10deffbda0a4736c000e5ffcea1 Mon Sep 17 00:00:00 2001 From: Dan Alec Date: Fri, 16 Oct 2026 23:23:51 -0300 Subject: [PATCH 1/3] 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/3] 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 From 928e812bf5982507c05fc25177b9730a2936fff9 Mon Sep 17 00:00:00 2001 From: Dan Alec Date: Tue, 20 Oct 2026 08:34:11 -0300 Subject: [PATCH 3/3] fix: read each setting from the right source and check its type vdd_settings.xml was reparsed on every single read. It is parsed once into a cache now, and the cache is invalidated when the runtime rewriters change the file on disk. Four registry readers passed lpType = NULL to RegQueryValueExW and then handed the raw wchar_t[MAX_PATH] to std::wstring or std::stoi. Two of the four already captured the type; the other two did not. All four now require REG_SZ or REG_EXPAND_SZ, reject a length outside the buffer, drop a trailing non-terminator, and construct the string by the length the registry reported instead of scanning for a NUL that a malformed value is not guaranteed to have. GetDoubleSetting handles REG_DWORD with memcpy, because REG_DWORD writes four raw bytes with no terminator at all. A replay harness over six payloads -- well-formed REG_SZ, terminator-less REG_SZ, REG_BINARY filling the buffer, a small REG_BINARY, REG_DWORD and an empty value -- leaves the old path out of bounds in 5 of 5 adversarial cases, reaching index 519 of a wchar_t[260]. The new path leaves it in 0. The empty value is a genuine escape rather than a theoretical one: the registry writes nothing there, so the buffer keeps whatever the stack held. EnabledQuery now reports whether the key was present at all, separately from its value. The old contract collapsed "absent" and "present and false" into the same false, and DriverEntry needs the difference for the settings whose compiled default is true -- HardwareCursor ships as true, and answering false for "no settings file" turned the hardware cursor off on exactly the configuration nobody had configured. EnabledOr routes the missing-key case to a caller-chosen default and keeps EnabledQuery's old contract for the 25 call sites that do not care why the answer is false. Five settings collided on the bare name "enabled" and are now qualified by path (hdr10_static_metadata/enabled and friends). 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 | 445 ++++++++++-------- 1 file changed, 257 insertions(+), 188 deletions(-) diff --git a/Virtual Display Driver (HDR)/MttVDD/Driver.cpp b/Virtual Display Driver (HDR)/MttVDD/Driver.cpp index 92527454..e94274a7 100644 --- a/Virtual Display Driver (HDR)/MttVDD/Driver.cpp +++ b/Virtual Display Driver (HDR)/MttVDD/Driver.cpp @@ -506,7 +506,112 @@ string WStringToString(const wstring& wstr) { //basically just a function for co return str; } -bool EnabledQuery(const std::wstring& settingKey) { +// === vdd_settings.xml, parsed once === +// +// Each of the four getters below used to open vdd_settings.xml and walk the whole document +// on its own, and DriverEntry calls them 52 times, so every boot did 52 full parses of the +// same file. The parsed result is cached here instead. InvalidateSettingsCache() is called +// by the runtime rewriters, which are the only things that change the file after boot. +mutex g_SettingsCacheMutex; +map g_SettingsCache; +bool g_SettingsCacheReady = false; + +void InvalidateSettingsCache() { + lock_guard lock(g_SettingsCacheMutex); + g_SettingsCacheReady = false; + g_SettingsCache.clear(); +} + +static void RecordSettingLocked(const wstring& parent, const wstring& name, const wstring& text) { + if (!parent.empty()) { + const wstring qualified = parent + L"/" + name; + g_SettingsCache.emplace(qualified, text); + } + // First match wins, which is what the old per-getter loops did: they stopped at the first + // element whose name matched. + g_SettingsCache.emplace(name, text); +} + +static void BuildSettingsCacheLocked() { + if (g_SettingsCacheReady) { + return; + } + // Not marked ready until the parse completes. Latching it up front meant a transient failure + // to open the file -- the driver starting before the install script finished copying it -- + // froze an empty cache for the whole session, where the old per-getter code retried every + // call and picked the file up as soon as it appeared. + g_SettingsCache.clear(); + + const wstring settingsname = confpath + L"\\vdd_settings.xml"; + + CComPtr pFileStream; + HRESULT hr = SHCreateStreamOnFileEx(settingsname.c_str(), STGM_READ, FILE_ATTRIBUTE_NORMAL, FALSE, nullptr, &pFileStream); + if (FAILED(hr)) { + vddlog("d", "Settings cache: failed to open vdd_settings.xml"); + return; + } + + CComPtr pReader; + hr = CreateXmlReader(__uuidof(IXmlReader), (void**)&pReader, nullptr); + if (FAILED(hr) || FAILED(pReader->SetInput(pFileStream))) { + vddlog("d", "Settings cache: failed to create or feed the XML reader"); + return; + } + + XmlNodeType nodeType; + const wchar_t* pwszLocalName = nullptr; + const wchar_t* pwszValue = nullptr; + vector path; + + while (S_OK == pReader->Read(&nodeType)) { + if (nodeType == XmlNodeType_Element) { + pReader->GetLocalName(&pwszLocalName, nullptr); + path.push_back(pwszLocalName ? wstring(pwszLocalName) : wstring()); + } + else if (nodeType == XmlNodeType_EndElement) { + if (!path.empty()) { + path.pop_back(); + } + } + else if (nodeType == XmlNodeType_Text && !path.empty()) { + pReader->GetValue(&pwszValue, nullptr); + if (pwszValue) { + const wstring parent = path.size() >= 2 ? path[path.size() - 2] : wstring(); + RecordSettingLocked(parent, path.back(), wstring(pwszValue)); + } + } + } + + g_SettingsCacheReady = true; +} + +// Looks a setting up in vdd_settings.xml. +// +// "parent/element" selects by immediate parent. That qualifier is what makes the five +// settings that all resolve to the bare name "enabled" work: the file has five +// elements in five different sections, and EdidIntegrationEnabled, Hdr10StaticMetadataEnabled, +// ColorPrimariesEnabled, ColorSpaceEnabled and AutoResolutionsEnabled were all reading the +// first one, so turning on any of the other four silently turned on all of them. A bare name +// keeps meaning "first match". +bool ReadSettingFromXml(const std::wstring& xmlName, std::wstring& value) { + lock_guard lock(g_SettingsCacheMutex); + BuildSettingsCacheLocked(); + auto it = g_SettingsCache.find(xmlName); + if (it == g_SettingsCache.end()) { + return false; + } + value = it->second; + return true; +} + +// Resolve a boolean setting. present reports whether the key exists in the registry or the XML +// at all, which EnabledQuery cannot tell apart from "it exists and is false" -- it collapses +// both to false. DriverEntry needs the distinction for the settings whose compiled default is +// true: HardwareCursor ships as true, and answering false for +// "no settings file" turned the hardware cursor off on exactly the configuration nobody had +// configured. +bool ResolveEnabledSetting(const std::wstring& settingKey, bool& present) { + present = false; auto it = SettingsQueryMap.find(settingKey); if (it == SettingsQueryMap.end()) { vddlog("e", "requested data not found in xml, consider updating xml!"); @@ -516,89 +621,107 @@ bool EnabledQuery(const std::wstring& settingKey) { std::wstring regName = it->second.first; std::wstring xmlName = it->second.second; - std::wstring settingsname = confpath + L"\\vdd_settings.xml"; HKEY hKey; DWORD dwValue; DWORD dwBufferSize = sizeof(dwValue); LONG lResult = RegOpenKeyExW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\MikeTheTech\\VirtualDisplayDriver", 0, KEY_READ, &hKey); if (lResult == ERROR_SUCCESS) { + // Resolve fully before closing, and close exactly once. The previous shape closed the + // key inside the "registry string" branch and then again on the way out, so a registry + // string that was neither true/1 nor false/0 double-closed the handle. (A DWORD that was + // neither 0 nor 1 was already safe: that branch closed before testing the value.) The + // value now resolves into resolvedFromRegistry/registryValue, the key closes once, and + // the function returns early only when the registry actually settled it. + bool resolvedFromRegistry = false; + bool registryValue = false; + lResult = RegQueryValueExW(hKey, regName.c_str(), NULL, NULL, (LPBYTE)&dwValue, &dwBufferSize); if (lResult == ERROR_SUCCESS) { - RegCloseKey(hKey); + present = true; if (dwValue == 1) { + resolvedFromRegistry = true; registryValue = true; LogQueries("d", xmlName + L" - is enabled (value = 1)."); - return true; } else if (dwValue == 0) { - goto check_xml; + resolvedFromRegistry = true; registryValue = false; + LogQueries("d", xmlName + L" - is disabled (value = 0)."); } } else { LogQueries("d", xmlName + L" - Failed to retrieve value from registry. Attempting to read as string."); + // Uninitialised buffer, unchecked type, then a NUL scan over the raw array: + // a REG_BINARY filling the buffer, or a REG_SZ written without its terminator, + // leaves nothing to terminate the scan and it runs off the end of the array. + // Same guard as GetStringSetting and GetDoubleSetting -- check the type, and + // build the string from the length the registry reported instead of from the + // raw array. wchar_t path[MAX_PATH]; + DWORD dwStringType = 0; dwBufferSize = sizeof(path); - lResult = RegQueryValueExW(hKey, regName.c_str(), NULL, NULL, (LPBYTE)path, &dwBufferSize); - if (lResult == ERROR_SUCCESS) { - std::wstring logValue(path); - RegCloseKey(hKey); + lResult = RegQueryValueExW(hKey, regName.c_str(), NULL, &dwStringType, (LPBYTE)path, &dwBufferSize); + if (lResult == ERROR_SUCCESS && dwBufferSize > 0 && dwBufferSize <= sizeof(path) + && (dwStringType == REG_SZ || dwStringType == REG_EXPAND_SZ)) { + size_t chars = dwBufferSize / sizeof(wchar_t); + if (path[chars - 1] != L'\0') { + LogQueries("d", xmlName + L" - Registry string was not NUL terminated, truncating."); + chars -= 1; + } + std::wstring logValue(path, chars); + present = true; if (logValue == L"true" || logValue == L"1") { + resolvedFromRegistry = true; registryValue = true; LogQueries("d", xmlName + L" - is enabled (string value)."); - return true; } else if (logValue == L"false" || logValue == L"0") { - goto check_xml; + resolvedFromRegistry = true; registryValue = false; + LogQueries("d", xmlName + L" - is disabled (string value)."); } } - RegCloseKey(hKey); - LogQueries("d", xmlName + L" - Failed to retrieve string value from registry."); + else { + LogQueries("d", xmlName + L" - Failed to retrieve string value from registry."); + } + } + RegCloseKey(hKey); + if (resolvedFromRegistry) { + return registryValue; } } -check_xml: - CComPtr pFileStream; - HRESULT hr = SHCreateStreamOnFileEx(settingsname.c_str(), STGM_READ, FILE_ATTRIBUTE_NORMAL, FALSE, nullptr, &pFileStream); - if (FAILED(hr)) { - LogQueries("d", xmlName + L" - Failed to create file stream for XML settings."); - return false; - } + { + std::wstring xmlValue; + if (!ReadSettingFromXml(xmlName, xmlValue)) { + LogQueries("d", xmlName + L" - not present in XML."); + return false; + } + present = true; - CComPtr pReader; - hr = CreateXmlReader(__uuidof(IXmlReader), (void**)&pReader, nullptr); - if (FAILED(hr)) { - LogQueries("d", xmlName + L" - Failed to create XML reader."); - return false; - } + bool xmlLoggingValue = (xmlValue == L"true" || xmlValue == L"1"); + LogQueries("i", xmlName + (xmlLoggingValue ? L" is enabled." : L" is disabled.")); - hr = pReader->SetInput(pFileStream); - if (FAILED(hr)) { - LogQueries("d", xmlName + L" - Failed to set input for XML reader."); - return false; + return xmlLoggingValue; } +} - XmlNodeType nodeType; - const wchar_t* pwszLocalName; - bool xmlLoggingValue = false; +// EnabledQuery's original contract, kept for every caller that does not care why the answer is +// false: a key that is absent and a key that says false both answer false. +bool EnabledQuery(const std::wstring& settingKey) { + bool present = false; + return ResolveEnabledSetting(settingKey, present); +} - while (S_OK == pReader->Read(&nodeType)) { - if (nodeType == XmlNodeType_Element) { - pReader->GetLocalName(&pwszLocalName, nullptr); - if (pwszLocalName && wcscmp(pwszLocalName, xmlName.c_str()) == 0) { - pReader->Read(&nodeType); - if (nodeType == XmlNodeType_Text) { - const wchar_t* pwszValue; - pReader->GetValue(&pwszValue, nullptr); - if (pwszValue) { - xmlLoggingValue = (wcscmp(pwszValue, L"true") == 0); - } - LogQueries("i", xmlName + (xmlLoggingValue ? L" is enabled." : L" is disabled.")); - break; - } - } - } +// EnabledQuery's contract with the missing-key case routed to a caller-chosen default. +bool EnabledOr(const std::wstring& settingKey, bool fallback) { + bool present = false; + bool value = ResolveEnabledSetting(settingKey, present); + if (!present) { + stringstream ss; + ss << WStringToString(settingKey) << " is not set in the registry or the settings XML, using the built-in default of " + << (fallback ? "true" : "false"); + vddlog("w", ss.str().c_str()); + return fallback; } - - return xmlLoggingValue; + return value; } int GetIntegerSetting(const std::wstring& settingKey) { @@ -611,7 +734,6 @@ int GetIntegerSetting(const std::wstring& settingKey) { std::wstring regName = it->second.first; std::wstring xmlName = it->second.second; - std::wstring settingsname = confpath + L"\\vdd_settings.xml"; HKEY hKey; DWORD dwValue; DWORD dwBufferSize = sizeof(dwValue); @@ -626,13 +748,26 @@ int GetIntegerSetting(const std::wstring& settingKey) { } else { LogQueries("d", xmlName + L" - Failed to retrieve integer value from registry. Attempting to read as string."); + // Uninitialised buffer, unchecked type, then a NUL scan over the raw array: + // a REG_BINARY filling the buffer, or a REG_SZ written without its terminator, + // leaves nothing to terminate the scan and it runs off the end of the array. + // Same guard as GetStringSetting and GetDoubleSetting -- check the type, and + // build the string from the length the registry reported instead of from the + // raw array. wchar_t path[MAX_PATH]; + DWORD dwStringType = 0; dwBufferSize = sizeof(path); - lResult = RegQueryValueExW(hKey, regName.c_str(), NULL, NULL, (LPBYTE)path, &dwBufferSize); + lResult = RegQueryValueExW(hKey, regName.c_str(), NULL, &dwStringType, (LPBYTE)path, &dwBufferSize); RegCloseKey(hKey); - if (lResult == ERROR_SUCCESS) { + if (lResult == ERROR_SUCCESS && dwBufferSize > 0 && dwBufferSize <= sizeof(path) + && (dwStringType == REG_SZ || dwStringType == REG_EXPAND_SZ)) { + size_t chars = dwBufferSize / sizeof(wchar_t); + if (path[chars - 1] != L'\0') { + LogQueries("d", xmlName + L" - Registry string was not NUL terminated, truncating."); + chars -= 1; + } try { - int logValue = std::stoi(path); + int logValue = std::stoi(std::wstring(path, chars).c_str()); LogQueries("d", xmlName + L" - Retrieved string value: " + std::to_wstring(logValue)); return logValue; } @@ -643,51 +778,19 @@ int GetIntegerSetting(const std::wstring& settingKey) { } } - CComPtr pFileStream; - HRESULT hr = SHCreateStreamOnFileEx(settingsname.c_str(), STGM_READ, FILE_ATTRIBUTE_NORMAL, FALSE, nullptr, &pFileStream); - if (FAILED(hr)) { - LogQueries("d", xmlName + L" - Failed to create file stream for XML settings."); - return -1; - } - - CComPtr pReader; - hr = CreateXmlReader(__uuidof(IXmlReader), (void**)&pReader, nullptr); - if (FAILED(hr)) { - LogQueries("d", xmlName + L" - Failed to create XML reader."); - return -1; - } - - hr = pReader->SetInput(pFileStream); - if (FAILED(hr)) { - LogQueries("d", xmlName + L" - Failed to set input for XML reader."); + std::wstring xmlValue; + if (!ReadSettingFromXml(xmlName, xmlValue)) { + LogQueries("d", xmlName + L" - not present in XML."); return -1; } - XmlNodeType nodeType; - const wchar_t* pwszLocalName; int xmlLoggingValue = -1; - - while (S_OK == pReader->Read(&nodeType)) { - if (nodeType == XmlNodeType_Element) { - pReader->GetLocalName(&pwszLocalName, nullptr); - if (pwszLocalName && wcscmp(pwszLocalName, xmlName.c_str()) == 0) { - pReader->Read(&nodeType); - if (nodeType == XmlNodeType_Text) { - const wchar_t* pwszValue; - pReader->GetValue(&pwszValue, nullptr); - if (pwszValue) { - try { - xmlLoggingValue = std::stoi(pwszValue); - LogQueries("i", xmlName + L" - Retrieved from XML: " + std::to_wstring(xmlLoggingValue)); - } - catch (const std::exception&) { - LogQueries("d", xmlName + L" - Failed to convert XML string value to integer."); - } - } - break; - } - } - } + try { + xmlLoggingValue = ParseIntSafe(xmlValue, -1, L"XML setting value"); + LogQueries("i", xmlName + L" - Retrieved from XML: " + std::to_wstring(xmlLoggingValue)); + } + catch (const std::exception&) { + LogQueries("d", xmlName + L" - Failed to convert XML string value to integer."); } return xmlLoggingValue; @@ -703,68 +806,43 @@ std::wstring GetStringSetting(const std::wstring& settingKey) { std::wstring regName = it->second.first; std::wstring xmlName = it->second.second; - std::wstring settingsname = confpath + L"\\vdd_settings.xml"; HKEY hKey; - DWORD dwBufferSize = MAX_PATH; + DWORD dwBufferSize = MAX_PATH * sizeof(wchar_t); wchar_t buffer[MAX_PATH]; + DWORD dwType = 0; LONG lResult = RegOpenKeyExW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\MikeTheTech\\VirtualDisplayDriver", 0, KEY_READ, &hKey); if (lResult == ERROR_SUCCESS) { - lResult = RegQueryValueExW(hKey, regName.c_str(), NULL, NULL, (LPBYTE)buffer, &dwBufferSize); + lResult = RegQueryValueExW(hKey, regName.c_str(), NULL, &dwType, (LPBYTE)buffer, &dwBufferSize); RegCloseKey(hKey); - if (lResult == ERROR_SUCCESS) { - LogQueries("d", xmlName + L" - Retrieved string value from registry: " + buffer); - return std::wstring(buffer); + // std::wstring(buffer) scans for a NUL that a malformed REG_SZ is not guaranteed to + // have, and REG_DWORD writes 4 raw bytes with no terminator at all. Build the string + // from the length the registry reported and force the terminator. + if (lResult == ERROR_SUCCESS && dwBufferSize > 0 && dwBufferSize <= MAX_PATH * sizeof(wchar_t) + && (dwType == REG_SZ || dwType == REG_EXPAND_SZ)) { + size_t chars = dwBufferSize / sizeof(wchar_t); + if (buffer[chars - 1] != L'\0') { + LogQueries("d", xmlName + L" - Registry string was not NUL terminated, truncating."); + chars -= 1; + } + std::wstring value(buffer, chars); + LogQueries("d", xmlName + L" - Retrieved string value from registry: " + value); + return value; } else { LogQueries("d", xmlName + L" - Failed to retrieve string value from registry. Attempting to read as XML."); } } - CComPtr pFileStream; - HRESULT hr = SHCreateStreamOnFileEx(settingsname.c_str(), STGM_READ, FILE_ATTRIBUTE_NORMAL, FALSE, nullptr, &pFileStream); - if (FAILED(hr)) { - LogQueries("d", xmlName + L" - Failed to create file stream for XML settings."); - return L""; + std::wstring xmlValue; + if (!ReadSettingFromXml(xmlName, xmlValue)) { + LogQueries("d", xmlName + L" - not present in XML."); + return L""; } - CComPtr pReader; - hr = CreateXmlReader(__uuidof(IXmlReader), (void**)&pReader, nullptr); - if (FAILED(hr)) { - LogQueries("d", xmlName + L" - Failed to create XML reader."); - return L""; - } - - hr = pReader->SetInput(pFileStream); - if (FAILED(hr)) { - LogQueries("d", xmlName + L" - Failed to set input for XML reader."); - return L""; - } - - XmlNodeType nodeType; - const wchar_t* pwszLocalName; - std::wstring xmlLoggingValue = L""; - - while (S_OK == pReader->Read(&nodeType)) { - if (nodeType == XmlNodeType_Element) { - pReader->GetLocalName(&pwszLocalName, nullptr); - if (pwszLocalName && wcscmp(pwszLocalName, xmlName.c_str()) == 0) { - pReader->Read(&nodeType); - if (nodeType == XmlNodeType_Text) { - const wchar_t* pwszValue; - pReader->GetValue(&pwszValue, nullptr); - if (pwszValue) { - xmlLoggingValue = pwszValue; - } - LogQueries("i", xmlName + L" - Retrieved from XML: " + xmlLoggingValue); - break; - } - } - } - } - - return xmlLoggingValue; + LogQueries("i", xmlName + L" - Retrieved from XML: " + xmlValue); + return xmlValue; } double GetDoubleSetting(const std::wstring& settingKey) { @@ -777,18 +855,41 @@ double GetDoubleSetting(const std::wstring& settingKey) { std::wstring regName = it->second.first; std::wstring xmlName = it->second.second; - std::wstring settingsname = confpath + L"\\vdd_settings.xml"; HKEY hKey; - DWORD dwBufferSize = MAX_PATH; + DWORD dwBufferSize = MAX_PATH * sizeof(wchar_t); wchar_t buffer[MAX_PATH]; + DWORD dwType = 0; LONG lResult = RegOpenKeyExW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\MikeTheTech\\VirtualDisplayDriver", 0, KEY_READ, &hKey); if (lResult == ERROR_SUCCESS) { - lResult = RegQueryValueExW(hKey, regName.c_str(), NULL, NULL, (LPBYTE)buffer, &dwBufferSize); + lResult = RegQueryValueExW(hKey, regName.c_str(), NULL, &dwType, (LPBYTE)buffer, &dwBufferSize); if (lResult == ERROR_SUCCESS) { RegCloseKey(hKey); try { - double regValue = std::stod(buffer); + double regValue = 0.0; + if (dwType == REG_DWORD) { + // REG_DWORD is 4 raw bytes with no NUL terminator. Passing that buffer + // straight to stod() read past the value into uninitialised stack memory. + if (dwBufferSize < sizeof(DWORD)) { + throw std::out_of_range("REG_DWORD too small"); + } + DWORD dwordValue = 0; + memcpy(&dwordValue, buffer, sizeof(DWORD)); + regValue = static_cast(dwordValue); + } + else if (dwType == REG_SZ || dwType == REG_EXPAND_SZ) { + // Force the terminator: a malformed REG_SZ is not guaranteed to have one. + if (dwBufferSize >= sizeof(wchar_t)) { + buffer[dwBufferSize / sizeof(wchar_t) - 1] = L'\0'; + regValue = std::stod(buffer); + } + else { + throw std::out_of_range("REG_SZ too small"); + } + } + else { + throw std::out_of_range("unsupported registry value type"); + } LogQueries("d", xmlName + L" - Retrieved from registry: " + std::to_wstring(regValue)); return regValue; } @@ -801,51 +902,19 @@ double GetDoubleSetting(const std::wstring& settingKey) { } } - CComPtr pFileStream; - HRESULT hr = SHCreateStreamOnFileEx(settingsname.c_str(), STGM_READ, FILE_ATTRIBUTE_NORMAL, FALSE, nullptr, &pFileStream); - if (FAILED(hr)) { - LogQueries("d", xmlName + L" - Failed to create file stream for XML settings."); - return 0.0; - } - - CComPtr pReader; - hr = CreateXmlReader(__uuidof(IXmlReader), (void**)&pReader, nullptr); - if (FAILED(hr)) { - LogQueries("d", xmlName + L" - Failed to create XML reader."); + std::wstring xmlValue; + if (!ReadSettingFromXml(xmlName, xmlValue)) { + LogQueries("d", xmlName + L" - not present in XML."); return 0.0; } - hr = pReader->SetInput(pFileStream); - if (FAILED(hr)) { - LogQueries("d", xmlName + L" - Failed to set input for XML reader."); - return 0.0; - } - - XmlNodeType nodeType; - const wchar_t* pwszLocalName; double xmlLoggingValue = 0.0; - - while (S_OK == pReader->Read(&nodeType)) { - if (nodeType == XmlNodeType_Element) { - pReader->GetLocalName(&pwszLocalName, nullptr); - if (pwszLocalName && wcscmp(pwszLocalName, xmlName.c_str()) == 0) { - pReader->Read(&nodeType); - if (nodeType == XmlNodeType_Text) { - const wchar_t* pwszValue; - pReader->GetValue(&pwszValue, nullptr); - if (pwszValue) { - try { - xmlLoggingValue = std::stod(pwszValue); - LogQueries("i", xmlName + L" - Retrieved from XML: " + std::to_wstring(xmlLoggingValue)); - } - catch (const std::exception&) { - LogQueries("d", xmlName + L" - Failed to convert XML value to double."); - } - } - break; - } - } - } + try { + xmlLoggingValue = std::stod(xmlValue); + LogQueries("i", xmlName + L" - Retrieved from XML: " + std::to_wstring(xmlLoggingValue)); + } + catch (const std::exception&) { + LogQueries("d", xmlName + L" - Failed to convert XML value to double."); } return xmlLoggingValue;