fix: send-counter wrap guard (nonce reuse) + warning sweep, tidy-gated - #25
Merged
Conversation
CI ran clang-tidy but did not gate on it (.clang-tidy has
WarningsAsErrors: ''), so 26 diagnostics rode along in every build log.
Root-cause each rather than suppress:
- performance-enum-size (12 enums): give each scoped decision/state enum an
explicit std::uint8_t base. Wire values live in Models/Protocol.h as
separate std::uint8_t/uint16_t constants, so this only shrinks the enums
and never touches the wire encoding.
- performance-move-const-arg (AppModel::rebuild): QHash::insert takes
const T&, so std::move was a no-op; move the sender in via operator[].
- performance-no-automatic-move (ConnectionStore): drop const on the
returned QString locals so they move out instead of copying.
- performance-unnecessary-value-param: take the shared_ptr in
markConnected by const ref; move the response callback into the inner
lambda capture in makeHooks.
- bugprone-unchecked-optional-access (runReconcile): read connectionId()
once into a local, check it, then deref the local. The mutating
setReconcileInFlight() call between the old check and deref is what
defeated the flow analysis.
- modernize-return-braced-init-list (SessionCrypto): return {hex}.
- cert-err34-c (MdnsDiscovery): parse TXT integers with std::from_chars
instead of std::atoi (checked, no silent-failure path).
- cert-err33-c (main): explicitly discard the best-effort stderr fprintf
on the fatal libsodium-init path.
- cert-dcl37-c/cert-dcl51-cpp (SDLGamepadBridge): the SDL-dictated
_SDL_GameController forward declaration's NOLINT named only
bugprone-reserved-identifier, but cert-* is enabled so clang-tidy
reports under the cert alias; name all three aliases so the existing
documented suppression actually applies.
Catch2's expression-decomposer templates (ITransientExpression, BinaryExpr) carry virtual functions with a non-virtual destructor by design, tripping -Wnon-virtual-dtor from dish_warnings at every TEST_CASE (88 warnings per CI build). Mark the FetchContent Catch2 targets' include dirs SYSTEM -- the same treatment satellite gives its vendored libraries -- so the noise clears without dropping -Wnon-virtual-dtor from our own warning set. A system-installed Catch2 (the find_package branch) is already imported as SYSTEM by CMake.
Add timeout-minutes: 30 (matching satellite's linux-ci lane) so a hung build/test/tidy step is reclaimed instead of burning the six-hour default on the billed private-repo runner.
The clang-tidy sweep had silenced performance-move-const-arg at the four rebuild() inserts by switching QHash::insert(k, std::move(v)) to operator[] = std::move(v). But operator[] takes QHash through its detach->rebuild path, which g++ -O2 flags with -Walloc-size-larger-than inside qhash.h (2 new build warnings, absent on main). QHash::insert has no rvalue overload, so the original move was always a no-op copy anyway; dropping the move clears the tidy finding with zero build warnings and matches main's actual runtime behavior.
The send counter was a uint32 fetch_add with no exhaustion guard: at 2^32 packets in one unbroken session it wrapped and kept sealing ciphertexts under reused (key, nonce) pairs — an on-wire keystream-reuse leak of input reports (contract §Crypto forbids exactly this; ~50 days at 1 kHz). The counterNeedsRepush reducer existed but had zero call sites. Mirror dish-mac's G4 design: - sendEncrypted draws from a 64-bit AtomicCounter and goes SILENT past 2^32-1 instead of wrapping; sendCounter() clamps so the poll keeps reading re-PUT needed. - The 1 Hz alive tick fires a new SessionHooks::rekey (single-shot per approach) once the counter crosses 0xF0000000; the manager's runRekey re-PUTs the session and installs the fresh token/salt/key on the SAME socket — counters restart at 1, the hot path never blips, and a stale response cannot re-arm a replaced client. Tests drive a real loopback socket across the exhaustion boundary (no wrapped packet on the wire, no counter value ever repeats under one key) and drive the tick wiring through a test seam (fires once, re-arms only after the re-key lands). Lands on the warning-sweep branch by maintainer direction (single-branch preference) rather than a separate fix/counter-wrap-guard branch.
The atoi→from_chars swap in the warning sweep was not strictly behavior-preserving: from_chars rejects leading whitespace and an explicit '+' that atoi accepted. Keep the strict parse — the live responder emits bare std::to_string digits (satellite mdns_responder.cpp) so nothing real is lost — and pin the exact accept/reject set: bare digits and a numeric prefix parse, whitespace/'+'/empty fall back to the default port. The PR body's no-behavioral-change claim is corrected alongside.
The sweep cleared all 26 findings but left WarningsAsErrors '' so they could silently regress. Pass --warnings-as-errors='*' at the CI and ci_local invocations instead of editing .clang-tidy — the config file is fleet-canonical (shared with satellite/dish-android) and stays untouched; the flag gates exactly the curated, now-clean check set.
The proactive re-key makes setConnectionParams run against a LIVE client for the first time — the SDL/heartbeat senders and the receive loop read key_/token_/counters concurrently, so an unguarded swap could pair an old key with a fresh counter (a mis-sequenced nonce the server replay guard then chokes on), tear the 32-byte key mid-copy, or stamp a stale replay mark onto the fresh session. materialMtx_ now guards the material: one hold draws (key, token, counter) together on the send path (dish-mac's nextSendMaterial design), the receive loop snapshots the same way and only advances the replay mark if no re-key raced its decrypt. Pinned by a hammer test: two sender threads across 40 re-key generations; every wire packet must decrypt under the key its token selects and no (token, counter) pair may repeat. Probabilistic pre-fix (a race), it failed 2/10 runs against an unlocked draw on a 4-core host; 0/10 with the lock.
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
Two strands on one branch (single-branch fold by maintainer direction):
uint32fetch_addwith no exhaustion guard: at 2^32 packets in one unbroken session it wrapped and kept sealing ChaCha20-Poly1305 ciphertexts under reused (key, nonce) pairs — an on-wire keystream-reuse confidentiality leak (~50 days at a sustained 1 kHz). ThecounterNeedsRepushreducer existed with zero call sites. Now wired, mirroring dish-mac's G4 design: proactive re-key past0xF0000000+ hard go-silent backstop at exhaustion.src/, leaves the tree warning-clean under the CI's g++-Werrorbuild, and now gates the tidy step so findings cannot silently regress.What changed
8942d1efix(linux): clear the clang-tidy findings acrosssrc/— resolves the 26 findings the CI clang-tidy step reports onmain(performance-move-const-arg,performance-enum-size,performance-no-automatic-move,modernize-return-braced-init-list,cert-*, etc.).8c253bftest(linux): mark vendored Catch2 headers SYSTEM — the vendored test framework stops contributing third-party diagnostics to the tidy/warning surface.bd144dfchore(ci): cap the linux-ci job withtimeout-minutes— a hung job self-cancels instead of squatting the 6-hour default.b12b127fix(linux): drop uselessstd::moveon QHash insert instead ofoperator[]— see the corrected note below.2f79d9fstyle: trim warning-sweep comments to terse why-only.a6dbac6fix(linux): never wrap the UDP send counter into ChaCha20 nonce reuse —sendEncrypteddraws from a 64-bit counter and goes silent past 2^32−1 instead of wrapping; the 1 Hz alive tick fires a single-shotSessionHooks::rekeyonce the counter crosses0xF0000000; the manager'srunRekeyre-PUTs and installs the fresh token/salt/key on the same socket (counters restart at 1, no state blip, stale responses can't re-arm a replaced client). Tests drive a real loopback socket across the exhaustion boundary (no wrapped packet on the wire; no counter value ever repeats under one key) and drive the tick wiring through a test seam (fires once per approach; re-arms only after the re-key lands).8082360test(linux): pin the intentionally strict TXT port parse — corrects this PR's earlier "no behavioral change" claim:from_charsrejects leading whitespace (udp= 9443) and an explicit+(udp=+9443) thatatoiaccepted. Kept strict deliberately — the live responder emits barestd::to_stringdigits (satellitemdns_responder.cpp), so nothing real is lost. The exact accept/reject set is pinned by tests (bare digits and numeric prefixes parse; whitespace/+/empty fall back to defaults).10c9ed9chore(ci): fail the clang-tidy gate on any reintroduced finding —--warnings-as-errors='*'at the CI +ci_local.shinvocations..clang-tidyitself stays untouched: it is the fleet-canonical config shared with satellite/dish-android, so the gate lives in the invocation, scoped to the curated (now clean) check set. Verified by probe: a reintroducedmodernize-use-nullptrfinding fails the step.cc82d58fix(linux): draw session material atomically for the live re-key — the proactive re-key makessetConnectionParamsrun against a live client for the first time;materialMtx_now makes the (key, token, counter, replay-mark) swap atomic vs the SDL/heartbeat senders and the receive loop (dish-mac'snextSendMaterialdesign). Pinned by a two-thread × 40-generation hammer test: every wire packet must decrypt under the key its token selects, no (token, counter) pair repeats (failed 2/10 runs against an unlocked draw; 0/10 with the lock).Note on the QHash inserts (corrected)
An earlier revision of this note claimed "
QHash::inserthas no rvalue overload, the move was always a no-op". That claim is wrong on Qt 6 —QHash::insert(const Key&, T&&)exists (qhash.h), so the moves were real. The code change stands for the right reasons anyway:operator[]took QHash through its detach→new Span[]path, which g++-O2flags with-Walloc-size-larger-than=inside qhash.h (a Qt/GCC false positive; clang never emits it), and dropping a move of a copyablestd::functiontypedef in a UI-rate rebuild is harmless.insertstays; the tidy finding stays cleared; zero g++ build warnings.Test evidence
Authoritative ubuntu-24.04 container gate on the branch head (
cc82d58), mirroring linux-ci under g++:-Werror--warnings-as-errors='*'gate: clean, and the probe (deliberately reintroduced finding) fails the step as intendedLocal macOS
scripts/ci_local.sh(clang) also green end-to-end; new-test flake check: 10 consecutive clean runs of the hammer, 5 of the wiring tests.🤖 Generated with Claude Code