Run the host tests under ASan/UBSan, and fix the six defects it found - #123
Conversation
…ects it found
The recurring failure mode in this driver is lifetime, and a plain test build
cannot see it: a use-after-free reads plausible bytes and the test passes. The
project had no sanitizer configuration at all. The first sanitized run found six
real stack-use-after-scope defects already sitting in the suite.
Four are test-ordering mistakes -- an object declared before the locals its
destructor's callbacks write into, so destruction order kills the captures first:
EfcTransportTest.IgnoresResponsesFromOtherNodes
FireworksProtocolTest.PrepareDuplexProbesHwInfoFirstWhenInitializeDidNotRun
CompletionRefactorPlan.{ARResponseRejectsDifferentNodeNumber,BusyAckExtendsDeadlineNoCompletion}
Fixed by declaring the captured state first, with a note on why the order matters.
The other two were the tests modelling ownership in a way production never does.
FetchAgent tracks outstanding ORBs by raw pointer and writes SetAppended(false)
into each when cleared, so a cleared ORB must still be alive. Production handles
that explicitly -- CommandExecutor::CleanupCommandResources calls
session_.ClearCommandTracking() before dropping commandORB_, and SessionRecord
declares its executor last so it tears down before the session. The tests built
bare stack ORBs with no executor doing that for them. They now perform the same
deregistration, applied to all five tests that build ORBs rather than only the two
that happened to fail; the others merely had no outstanding ORBs at teardown.
CI gains a sanitized job and build.sh a --asan flag, so this class cannot return
unnoticed. Both set ASAN_OPTIONS=abort_on_error=1: without it a sanitizer report
is printed and the process still exits 0, so ctest reads it as a pass -- which is
exactly how the first sanitized run under-reported its own results.
The CI job runs at half the cores because each sanitized process reserves a large
shadow mapping, and one test was seen aborting under memory pressure at full
parallelism with no sanitizer report behind it.
Sanitized and plain are both fully green.
…sults UBSan on the first CI run caught a real defect in a struct the driver and the app both cast raw buffers to: AVCCapabilitiesSerializerTests.cpp:183: runtime error: reference binding to misaligned address ...282 for type 'const uint32_t', which requires 4 byte alignment AVCMusicCapabilitiesWire declared its two padding bytes AFTER its only 32-bit field, leaving supportedRatesMask at offset 2 -- permanently misaligned -- while the comment on that padding claimed it aligned the struct to 8 bytes. Moving the padding ahead of the field puts the mask at offset 4, leaves every other offset and the total size unchanged, and makes the comment true. ASFW/Models/DriverConnectorModels.swift parses this header by fixed byte offsets and is updated in lockstep (2..<6 -> 4..<8). Verified by printing offsetof for every field the Swift side indexes: currentRate 1, mask 4, ports 8, numPlugs 14, sizeof 18 -- all matching. Two static_asserts now pin the alignment and the size, because nothing else connects the two languages. The test also copies the field out instead of comparing it in place: EXPECT_EQ binds a const uint32_t&, and binding a reference to a packed member is undefined even when the address happens to be aligned. Separately, six discarded [[nodiscard]] results, which are not one problem: DuplexOperationGateTests (x4) discarded gate_.Acquire(), which returns whether the claim was newly inserted and returns false both when the guid is already claimed and when the lock is null. Every site used it as setup for a following assertion, so a silent false meant the precondition was never established and the assertion passed regardless -- green for the wrong reason. Now ASSERT_TRUE, matching what the same file already does elsewhere. ApogeeTransportTests and OxfordCsrTests discarded UpsertFromROM(), which returns the resulting DeviceRecord rather than a status. Those tests want only the side effect, so (void) is correct there. Blanket-(void)-ing all six would have silenced the four that mattered. 1814/1814 sanitized, with the same ASAN_OPTIONS/UBSAN_OPTIONS CI uses.
|
Pushed fixes for everything CI surfaced. The sanitizer finding was real, and not in the tests. Moving the padding ahead of the field puts the mask at offset 4 and leaves every other offset and the total size unchanged. The six Also worth recording: I originally wrote this finding off as a flaky abort under parallelism. It wasn't — UBSan defaults to print-and-continue, so my local runs without |
Adds a sanitized build for the host tests, and fixes the six defects it found the first time it ran.
The recurring failure mode in this driver is lifetime — cross-service use-after-free on teardown, refcount imbalance, borrows that outlive what they borrow from. A plain test build can't see any of it: a use-after-free reads plausible bytes and the test passes. There was no sanitizer configuration in the project at all.
Turning it on found six real
stack-use-after-scopedefects already sitting in the suite.The six
Four are test-ordering mistakes — an object declared before the locals its destructor's callbacks write into, so destruction order kills the captures first:
EfcTransportTest.IgnoresResponsesFromOtherNodesFireworksProtocolTest.PrepareDuplexProbesHwInfoFirstWhenInitializeDidNotRunCompletionRefactorPlan.ARResponseRejectsDifferentNodeNumberCompletionRefactorPlan.BusyAckExtendsDeadlineNoCompletionThe other two are more interesting.
FetchAgenttracks outstanding ORBs by raw pointer and writesSetAppended(false)into each one when cleared, so a cleared ORB has to still be alive. Production does that properly —CommandExecutor::CleanupCommandResources()callsClearCommandTracking()before droppingcommandORB_, andSessionRecorddeclares its executor last so it tears down before the session. The tests were building bare stack ORBs with no executor doing that for them. They now perform the same deregistration.Applied to all five tests that build ORBs, not just the two that failed — the others merely had no outstanding ORBs at teardown.
Wiring
CI gets a sanitized job;
build.shgets--asan.Both set
ASAN_OPTIONS=abort_on_error=1, which matters more than it looks: without it ASan prints its report and the process still exits 0, so ctest calls it a pass. That's exactly how my first sanitized run under-reported its own results.The CI job runs at half the cores. Each sanitized process reserves a large shadow mapping, and at full parallelism one test was seen aborting under memory pressure with no sanitizer report behind it — not reproducible since, at
-j4or-j8, alone or in the full suite. If it shows up in CI it's real and worth chasing then.Testing
Sanitized and plain are both green. Test/build only — no driver code is touched, so the risk here is that CI gets slower, not that anything breaks.
Worth landing before the other branches I have queued, so they're validated by it rather than arriving alongside it.