Give the host DriverKit stubs real refcount semantics - #126
Merged
Merged
Conversation
HostDriverKitStubs stubbed OSObject::retain()/release() as empty bodies, so the
~160 retain/release/AdoptRetained sites in driver code were never exercised on
the host: leaks, double-releases and use-after-release were all invisible.
Nine stub classes derive from OSObject (IOMemoryDescriptor, IODispatchQueue,
IODMACommand, the dispatch sources), so this covers most of the ownership
surface the tests touch.
The stub also carried two lifetime models at once. OSSharedPtr was implemented
over std::shared_ptr while the object's own retain/release did nothing, and
detach() leaked a strong reference on purpose to reconcile them:
// Stub OSObject::release() is a no-op, so keep one strong ref alive
// (intentional leak) instead of letting the shared_ptr destroy an object
// the caller still holds.
OSSharedPtr is now a thin wrapper over the object's refcount like the real one,
so detach() simply hands over its reference and the leak is gone. retain() and
release() are const, matching OSObject.iig where they are declared const.
Turning this on immediately caught a real one, fixed in the next commit.
With real refcounting in the stub, eleven HardwareInterfaceOrderTests failed -- not on their assertions, which passed, but because gmock found the mock provider leaked at exit. A leaked mock is never destroyed, and gmock verifies expectations at destruction, so those eleven tests had been passing without their expectations ever being checked. The driver is right. HardwareInterface::Attach takes its OWN reference (OSSharedPtr(..., OSRetain)), because in production the provider belongs to IOKit which holds one too. The fixture stands in for IOKit, created the mock with new (count 1), let Attach retain it (count 2), and then dropped nothing -- while its comment claimed the device was 'Owned by hardware_ after Attach'. Release the fixture's own reference in TearDown and correct the comment.
Two gaps in the refcounting change, found reviewing it before opening a PR. The stubs had no tests of their own, despite every other test depending on them and despite their being the thing that decides whether an imbalance in driver code surfaces or stays hidden. Seven cases now pin the semantics that matter: a new object starts at one reference, release destroys only at zero, OSNoRetain adopts while OSRetain adds, copy retains while move does not, detach hands over the reference without releasing or leaking, and reset releases what it replaced. Over-release gets a tripwire, deliberately described as no more than that. release() destroys on the 1->0 edge, so by the time a count could go negative the object is already freed and the fetch_sub has itself touched dangling memory -- the assert can only fire through undefined behaviour, and it is compiled out under NDEBUG anyway. AddressSanitizer catches the same case precisely (heap-use-after-free on that fetch_sub, verified), and this project has no sanitizer build at all. That is the real gap; the assert is a cheap stopgap and says so where someone would otherwise trust it. Also noted on the class that instances must be heap-allocated: free() deletes, so a stack-allocated stub released to zero deletes a stack address. That was harmless while release() did nothing. Nothing does it today; the trap is new.
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
1 participant
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.
The host DriverKit stubs did not model object lifetime at all, so the suite could not see a whole class of refcount bug — and the mocking work that prompted this found real teardown problems the moment the stubs started behaving.
Commits
test(driverkit): give the host OSObject stub a real refcount—retain/releasewere no-ops.OSSharedPtris rewritten over the object's own refcount rather than tracking its own, and thedetach()leak is gone.test(core): drop the HardwareInterface fixture's own provider reference— the fixture held an extra reference that the production path does not, which is exactly the kind of thing a no-op refcount hides.test(driverkit): pin the stubs' own semantics, and tripwire over-release— the stubs are now load-bearing, so their behaviour needs its own tests, and an over-release trips loudly instead of passing silently.Context for the DriverKit question behind this: subclassing
OSObjectdoes require.iig(OSObject.iig:254-257marksretain/releaseLOCALONLY), which is why the host side needs a stub with real semantics rather than the production type.Rebased onto main after #123; the opt-in ASan/UBSan CMake commit that used to sit here landed with #123 and git dropped it as already upstream.