Repository navigation
Conversation
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.
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<bool>: 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.
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 <HardwareCursor>true</ HardwareCursor>, 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.
GetIntegerSetting answers -1 for a key that is in neither the registry nor the settings XML, and GetDoubleSetting answers 0.0. Neither answer is inert in the place it is used. -1 is rejected by the upper bound of the mode filter, so it works as a sentinel by accident, and only for that one consumer. Everything else that reads an integer setting has to know that -1 means "not set" and not a number. 0.0 is worse: it is inside [0,1], so a colour primary read from a key that does not exist becomes a point sitting on the corner of the triangle and passes validation without a word. The monitor then comes up with the wrong colour and nothing is logged. positiveOr() now applies the caller's fallback for both, and the settings that ship a compiled default of true are read with EnabledOr() so that "no settings file" is answered with the default rather than with false. The compiled defaults are reconciled with what vdd_settings.xml actually ships: gammaCorrection 2.4 -> 2.2 force_bit_depth "auto" -> 8 hardwareCursor false -> true maxResolutionWidth 7680 -> 10240 colour primaries BT.2020 -> Rec.709 The XML always wins at runtime, so a divergence between the two only showed up on the path with no settings file -- which is exactly the path nobody had configured. option.txt ships 10240x4320 and 8640 modes, and a ceiling of 7680 cut them off; vdd_settings.xml annotates the primaries as "sRGB color space (safe defaults)", and the BT.2020 numbers were the ones the file did not use. The settings that nothing ever read are removed rather than kept: fp16_surface_support, wide_color_gamut, hdr_tone_mapping, sdr_white_level and all eight Monitor Emulation entries were parsed and then dropped on the floor. A value in vdd_settings.xml for any of them did nothing, silently. Five settings collided on the bare name "enabled" and are now qualified by path (hdr10_static_metadata/enabled and friends), so a key that means one thing in one section cannot be read as another. Verified: Debug and Release on x64 and ARM64, 0 errors, all four producing MttVDD.dll, MttVDD.inf and a signed mttvdd.cat.
The two metadata stores were plain std::map globals, written by the settings loader and read by the IddCx HDR and gamma callbacks. A std::map is not safe to read while another thread inserts, so every access now goes through a locked accessor, under a mutex that is distinct from g_Mutex because those callbacks call vddlog() along the way. The caches are keyed by IDDCX_MONITOR, a value the OS hands out afresh on every adapter restart, and the HDR callback only ever added entries for the monitor it was called with. A stored template therefore outlived the monitor it described, and the callback would apply a stale template to a different one. A template key now stands in for the monitor, and the per-monitor entry is looked up first. The light levels coming out of vdd_settings.xml are cast to UINT16 without a range check, so a value above 65535 wrapped silently and a typo in the XML turned into a plausible-looking but wrong number. They are clamped now. overrideManualSettings is the only thing that decides whether the manual HDR settings apply, and its documented meaning in vdd_settings.xml is exactly that. The code also asked "is this still the default?", comparing maxDisplayMasteringLuminance against 1000.0, minDisplayMasteringLuminance against 0.05, redX against 0.708 and primaryColorSpace against L"sRGB". A comparison against a default cannot tell a default from a value someone typed, and vdd_settings.xml ships 1000.0, 0.05 and sRGB -- so three of the four were always true and the fourth was never true. The comparisons are gone. This changes behaviour: with override_manual_settings on, hdr10_static_metadata, color_primaries and color_space now have no effect. That is what the documentation has always said, but it is not what the code did. The gamma callback stays deliberately unregistered. Registering it in IDD_CX_CLIENT_CONFIG declares gamma support, and EndPointDiagnostics.GammaSupport is IDDCX_FEATURE_IMPLEMENTATION_NONE with no ramp ever programmed. There is no scan-out pipeline in this driver, so there is nothing to apply a ramp to. Verified: Debug and Release on x64 and ARM64, 0 errors, all four producing MttVDD.dll, MttVDD.inf and a signed mttvdd.cat.
Three defects in how the mode list is built, all of which end at the same place: the monitor reports modes that do not exist. The 32-bit product that computes pixelRate used to wrap before it reached the headline modes: 8K at 240 Hz is 240 * 7680 * 4324, which is wider than UINT32. The same product appears in the vertical total, where it wrapped to a value that was then used as a divisor. The preferred mode was inserted at the front of the list and the list was sorted afterwards, which moved it away from the front. The sort also treated the refresh rate as an integer field, so 59.94 and 60 were different modes. The vsync fraction is now what identifies a rate, and the duplicate removal runs before the preferred mode goes in rather than after. An unrecognised value used to fall out of the chain and leave the merged list empty. There is now an else that says so. option.txt offered refresh rates that no mode reported: 480 and 500 named no mode at all, and 30 and 75 named modes only under assumptions that do not hold. The rates the script offers are now the ones the driver can report -- <g_refresh_rate> in vdd_settings.xml ships 60/90/120/144/165/240, and option.txt, the fallback when no XML is present, carries 60/120/144/240. The EDID profile parser used stoi and stod on values straight out of monitor_profile.xml. A malformed value threw out of a UMDF callback, which crashes the host. The checked parsers are used instead. option.txt is read as: the first line that carries a count, not necessarily the first line, since '#' lines are comments. The count used to be read from a line that might not exist, and the display count was bounded only at the point where it was already used. Verified: Debug and Release on x64 and ARM64, 0 errors, all four producing MttVDD.dll, MttVDD.inf and a signed mttvdd.cat.
The log file was reopened, written and closed for every single line. The handle is opened once and reused; it is opened with FILE_SHARE_DELETE so the log can be rotated or deleted while the driver is running, and it is closed when the driver unloads. The refresh rate conversion multiplied in single precision and then rounded, so a rate that overflowed the product produced undefined behaviour on the conversion to int, and a rate large enough to overflow the product fed whatever came out of it into the vsync fraction. The product is computed in double and the result is bounded. The three runtime rewriters of vdd_settings.xml -- the one that sets the GPU name, the one that sets the friendly name and the one that sets the monitor count -- dropped every attribute of the element they rewrote, and they ignored an empty target element: an empty element produces no text node, so the flag that says "this element was written" survived past it and the *next* element in the document was the one that got rewritten. The attributes are preserved, and the empty element is handled. When a rewriter changes the file, the parsed copy the getters read from is stale, so the settings cache is invalidated. The pipe handle is no longer read without the lock in the log path, so a thread that is closing the pipe cannot see a handle that is already gone. Verified: Debug and Release on x64 and ARM64, 0 errors, all four producing MttVDD.dll, MttVDD.inf and a signed mttvdd.cat.
danalec
force-pushed
the
fix/driver-7-monitor
branch
from
October 20, 2026 13:04
2c5f3c1 to
1cf0de9
Compare
…nging Two defects, both on paths that only fail under a race or under a rare condition, which is why they survived review. CoCreateGuid had its HRESULT discarded. MonitorInfo is zero-initialised, so a failed CoCreateGuid left MonitorContainerId as GUID_NULL, and that was then handed to IddCxMonitorCreate as if it were a valid container ID. The result is checked now, and on failure the driver logs and does not create that monitor. The frame loop's wait list is built conditionally: the buffer event goes in only when it is neither null nor INVALID_HANDLE_VALUE, and the terminate event only when its handle exists. The terminate event therefore has no fixed index. Testing WAIT_OBJECT_0 for "buffer available" first meant that a signalled terminate event at index 0 took the continue branch, and the thread re-entered IddCxSwapChainReleaseAndAcquireBuffer forever -- a hang in EvtDriverUnload. The wait result is decided by handle identity now. The invariant is that the loop exits in finitely many steps, not that it exits on the first wait. With both handles signalled, WaitForMultipleObjects returns the lower index, which is the buffer; the acquire resets that event, and the next wait returns terminate. Also here: the hardware cursor event is now closed by the driver. IddCx signals it but does not own it, and the driver used to leak the previous one on reassignment for the same monitor and the last one on unassign. s_KnownMonitorEdid is read under a lock now, because maincalc() reassigns it on another thread while CreateMonitor reads it. The EDID base block is 128 bytes and calculateChecksum() reads it unconditionally, so a short EDID was read past the end. AssignSwapChain can run on more than one thread, so its counter is atomic. And the device is validated before it is dereferenced: the check used to sit below the As() call, so a null m_Device was already dereferenced by the time it ran. Verified: Debug and Release on x64 and ARM64, 0 errors, all four producing MttVDD.dll, MttVDD.inf and a signed mttvdd.cat. Note: the two fixes that make the current master compile on a newer toolset (AvTaskHandle and CoCreateGuid) are in VirtualDrivers#519 and are not repeated here.
Author
|
Closing this without merging. The work is real and it is verified -- every branch builds clean in Debug and The code stays available on the fork, split into reviewable pieces: fix/code-analysis-errors 1 file +14/-2 If any of it is ever wanted, the branches are there and this PR can be |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The monitor's container ID and the frame loop. Two files, +127 / −35.
Two defects, both on paths that only fail under a race or under a rare
condition, which is why they survived review.
The monitor could be created with a
GUID_NULLcontainer IDCoCreateGuidhad itsHRESULTdiscarded.MonitorInfois zero-initialised,so a failed
CoCreateGuidleftMonitorContainerIdasGUID_NULL, and thatwas then handed to
IddCxMonitorCreateas if it were a valid container ID. Theresult is checked now, and on failure the driver logs and does not create that
monitor.
The frame loop could hang forever
The wait list is built conditionally: the buffer event goes in only when it is
neither null nor
INVALID_HANDLE_VALUE, and the terminate event only when itshandle exists. The terminate event therefore has no fixed index.
Testing
WAIT_OBJECT_0for "buffer available" first meant that a signalledterminate event at index 0 took the
continuebranch, and the threadre-entered
IddCxSwapChainReleaseAndAcquireBufferforever — a hang inEvtDriverUnload. The wait result is decided by handle identity now.The invariant is that the loop exits in finitely many steps, not that it
exits on the first wait. With both handles signalled,
WaitForMultipleObjectsreturns the lower index, which is the buffer; the acquire resets that event,
and the next wait returns terminate.
Also here
does not own it, and the driver used to leak the previous one on reassignment
for the same monitor and the last one on unassign.
s_KnownMonitorEdidis read under a lock, becausemaincalc()reassigns iton another thread while
CreateMonitorreads it.calculateChecksum()reads itunconditionally, so a short EDID was read past the end.
AssignSwapChaincan run on more than one thread, so its counter is atomic.the
As()call, so a nullm_Devicewas already dereferenced by the time itran.
Verification
Debug and Release on x64 and ARM64 — four configurations, each producing
MttVDD.dll,MttVDD.infand a signedmttvdd.cat, withTreatWarningAsErroron.Not verified on hardware. The frame-loop hang needs a real unload to
reproduce, and the
GUID_NULLcontainer needsCoCreateGuidto fail, whichdoes not happen on a healthy machine.
The two fixes that make the current
mastercompile on a newer toolset(
AvTaskHandleandCoCreateGuid) are in #519 and are not repeated here.