test(cwx): fix teardown heap-use-after-free in drift-test Fixture (#3740) by jensenpat · Pull Request #3799 · aethersdr/AetherSDR

jensenpat · 2026-06-24T16:21:16Z

Summary

Fixes the weekly ASan/UBSan job failure in cwx_local_keyer_drift_test (#3740): a heap-use-after-free at Fixture teardown.

This is a test-only lifetime bug — no production code change.

Root cause

Fixture members destruct in reverse declaration order. With keyer declared before states, states is freed first:

struct Fixture {
    TestKeyer keyer;          // declared first  -> destroyed LAST
    std::vector<int> states;  // declared last   -> destroyed FIRST  (freed)
    Fixture() { keyer.setOnKeyDownChange([this](bool d){ states.push_back(d?1:0); }); }
};

When a test leaves the keyer key-down at teardown, the keyer's destructor still fires the callback into the already-freed vector:

~CwxLocalKeyer() (CwxLocalKeyer.cpp:48) → keyUpIfDown() (:235) → emitKeyDown(false) (:229) → installed callback → states.push_back(0) into freed memory.

emitKeyDown early-returns when the requested state already matches m_lastEmittedKeyDown, so only a test that ends key-down trips it. testLateTicksShortenNextWait starts "S", ticks twice, and never drains or stops — leaving the key down — which is exactly the case the ASan trace points at.

Fix

Reorder the two members so states is declared first (destroyed last) and outlives keyer. The destructor callback now writes into a live vector. Robust against any future test that leaves the keyer keyed-down.

Why production is unaffected

MainWindow resets the keyer before the audio sink it calls into.
The sidetone callback is null-guarded.

So no runtime UAF is reachable — this only broke the sanitizer job (priority: low).

Proof — agent automation bridge N/A; this is a sanitizer/unit-test bug

The automation bridge drives the running app and can't observe a unit-test teardown UAF, so the proof here is a direct before → after ASan run. The keyer's worker thread makes the live test's key-up/key-down state at teardown timing-dependent (hence a weekly flake); to make the proof deterministic I replicated the exact synchronous TestKeyer(spawnWorker=false) path and the testLateTicksShortenNextWait sequence (which leaves states at size=3, cap=4 — matching the CI trace's "12 bytes inside of 16-byte region"):

Before (buggy states-freed-first order) — reproduces the CI stack frame-for-frame:

==ERROR: AddressSanitizer: heap-use-after-free ... WRITE of size 4
  #9  AetherSDR::CwxLocalKeyer::emitKeyDown(bool)  CwxLocalKeyer.cpp:229
  #10 AetherSDR::CwxLocalKeyer::keyUpIfDown()      CwxLocalKeyer.cpp:235
  #11 AetherSDR::CwxLocalKeyer::~CwxLocalKeyer()   CwxLocalKeyer.cpp:48
SUMMARY: AddressSanitizer: heap-use-after-free in std::vector<int>::emplace_back
exit=134

After (fixed states-outlives-keyer order) — clean:

states=3 key DOWN; teardown fires keyUpIfDown into LIVE states
OK: clean teardown
exit=0

Real test, fixed, built with -fsanitize=address: 54 OK, 0 FAIL, no ASan report, All tests passed.

…thersdr#3740) The weekly ASan/UBSan job trips a heap-use-after-free in cwx_local_keyer_drift_test at Fixture teardown. Members destruct in reverse declaration order, so with `keyer` declared before `states`, `states` is freed first. When a test leaves the keyer key-down (e.g. testLateTicksShortenNextWait, which starts "S", ticks twice, and never drains or stops), ~CwxLocalKeyer -> keyUpIfDown() -> emitKeyDown(false) fires the installed callback, which writes into the already-freed `states` vector. Reorder the two members so `states` is declared first and therefore destroyed last, outliving `keyer`. The keyer's destructor callback now writes into a live vector. This is robust against any future test that leaves the keyer keyed-down at teardown. Test-only; no production code change. Production already orders this safely (MainWindow resets the keyer before the audio sink, and the sidetone callback is null-guarded), so no runtime UAF is reachable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

aethersdr-agent

Reviewed against the source — this is a clean, correct fix. ✅

Verified the root-cause analysis holds:

The current fixture (cwx_local_keyer_drift_test.cpp:91-92) declares keyer before states, so reverse-order destruction frees states first while keyer still lives — exactly the lifetime inversion described.
The production teardown chain is real: ~CwxLocalKeyer() (CwxLocalKeyer.cpp:48) → keyUpIfDown() (:235) → emitKeyDown(false) (:229) → m_onKeyDownChange, which is the fixture's states.push_back(...). With the buggy order that write lands in freed memory.
emitKeyDown's if (down == m_lastEmittedKeyDown) return; guard confirms the claim that only a test ending key-down trips it, and testLateTicksShortenNextWait does exactly that — starts "S", ticks twice, never drains or stops — so the keyer is still keyed at teardown. Matches the ASan trace.

On the fix itself:

Reordering so states (declared first → destroyed last) outlives keyer is the right, minimal correction, and it's robust against any future test that leaves the keyer keyed-down rather than special-casing the one that does today.
Constructing states before keyer is also safe on the construction side — the ctor body runs after both members exist, so setOnKeyDownChange still wires up correctly.
The load-bearing-order comment is genuinely valuable here; member-ordering bugs like this are easy to silently reintroduce, and the comment names the failure mode and the issue.

Test-only change, no production behavior affected, and the "why production is unaffected" reasoning (keyer reset before the sink, null-guarded sidetone callback) is consistent with the lifetime argument.

Thanks for the thorough writeup and the deterministic before/after repro, @jensenpat — nicely scoped fix.

_{🤖 aethersdr-agent · cost: $4.0867 · model: claude-opus-4-8}

ten9876

Correct, minimal fix for the member-destruction-order heap-UAF: reordering so states (declared first) outlives keyer (declared last) means the keyer dtor's keyUpIfDown → emitKeyDown → callback → states.push_back writes into a live vector. The load-bearing-order comment guards against re-regression. Test-only, no production surface (MainWindow resets the keyer before the sink + null-guarded callback), ASan trace reproduced frame-for-frame and the fixed test passes clean under -fsanitize=address. Thanks @jensenpat.

jensenpat marked this pull request as ready for review June 24, 2026 18:34

jensenpat requested a review from a team as a code owner June 24, 2026 18:34

aethersdr-agent Bot reviewed Jun 25, 2026

View reviewed changes

ten9876 approved these changes Jun 26, 2026

View reviewed changes

ten9876 merged commit 13b2421 into aethersdr:main Jun 26, 2026
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test(cwx): fix teardown heap-use-after-free in drift-test Fixture (#3740)#3799

test(cwx): fix teardown heap-use-after-free in drift-test Fixture (#3740)#3799
ten9876 merged 1 commit into
aethersdr:mainfrom
jensenpat:fix/3740-cwx-keyer-fixture-order

jensenpat commented Jun 24, 2026

Uh oh!

aethersdr-agent Bot left a comment

Uh oh!

ten9876 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jensenpat commented Jun 24, 2026

Summary

Root cause

Fix

Why production is unaffected

Proof — agent automation bridge N/A; this is a sanitizer/unit-test bug

Related

Uh oh!

aethersdr-agent Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ten9876 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants