fix + chore: send-counter nonce-reuse guard; warning + CI-hygiene sweep - #145
Merged
Conversation
…ings Define HAVE_SYSCONF for the hand-configured libsodium build so it resolves the page size via sysconf(_SC_PAGESIZE) -- the branch ./configure selects on Android, where _SC_PAGESIZE is a macro -- instead of falling through to the '#warning Unknown page size' path and a hardcoded fallback. Adopt CMP0135 (guarded for CMake < 3.24) so the FetchContent release-tarball download stops emitting the DOWNLOAD_EXTRACT_TIMESTAMP developer warning.
Root-cause fixes surfaced by an uncached compile and the lint gate:
- drop redundant safe calls and an always-true guard exposed by K2 smart-casts
(ControllerAdapter, GamepadOverlayActivity)
- name the DefaultLifecycleObserver overrides 'owner' to match the supertype
- bind the reused HID scratch buffer non-null before arraycopy
- replace the no-op combine transform '{ Unit }' with '{ }'
- extend the mandatory Drawable.getOpacity() suppressions with OVERRIDE_DEPRECATION
- drop the always-true SDK_INT >= LOLLIPOP branch and the empty super.onCleared();
use the isEmpty() KTX extension
- suppress the deliberate SDK-guarded legacy-path platform deprecations in the
vibrator and getParcelableExtra tests, mirroring the production convention
The @deprecated PhysicalReachability -> Composer migration is left visible.
Add timeout-minutes to every job that lacked a cap (android-ci build, play-listing, play-reviews, and the release build/publish-play/harden/publish jobs) so a hung step fails fast instead of running to the 6h default.
The JNI 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 until heartbeat death forced a reconnect (contract Crypto forbids exactly this; horizon ~50 days at a sustained 1 kHz). Mirror the dish-mac / dish-linux design: - sendEncrypted draws from a 64-bit counter (send_counter.h) and goes SILENT past 2^32-1 instead of wrapping; getSendCounter() clamps at the wire max so the poll can never read back under the threshold. - The 1 Hz Kotlin alive-poll fires a single-shot onRekeyNeeded once the counter crosses 0xF0000000 (counterNeedsRepush); the manager's rekey re-PUTs the session for fresh token/salt/key, restarting the counter at 1 long before exhaustion. Reconcile cannot carry this: its matched-view early-exit adopts the epoch without rotating anything. Host gtest pins the guard across the exhaustion boundary (goes silent, no value ever repeats under one key, clamped view); JVM tests pin the threshold predicate, the single-shot latch and its re-arm, and the manager re-PUT installing fresh params exactly once. runMgrTest now tears sessions down in a finally so an assertion failure can no longer spin the virtual-time drain into OOM. Appended to fix/warning-sweep per the maintainer's one-branch preference for this wave.
A file-level @Suppress("DEPRECATION") would silently mask any NEW deprecation introduced anywhere in the class. Production scopes these to the exact legacy call, so the tests now do the same: the one-arg getParcelableExtra, Vibrator.vibrate(Long) and VIBRATOR_SERVICE sites the JVM stub's SDK_INT=0 forces onto the legacy paths.
The sweep claims every workflow job carries a runtime cap, but the five jobs in _security.yml had none. Warm runtimes are 5-10 s; a 10 min cap leaves cold-install and full-history-scan headroom while cutting a hung runner off 36x sooner than the 360 min default.
The sweep flipped this libsodium feature macro citing only its silenced build warning, but it is a behavior change to a security dependency: with sysconf available, sodium_mlock/guarded allocations round to the runtime page size instead of a compiled-in guess - what Android 15's 16 KB-page devices need. Say that where the flag is set. Not exercisable in the host gtest harness (the Android-tailored sodium build only exists in the NDK build); on-device coverage rides the emulator integration suite, which initialises sodium and runs every encrypted-session path.
emir-hasanbegovic
changed the title
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
2 participants
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 layers on one branch (maintainer's one-branch preference for the 2026-07-20 fix wave):
fa1c35e,9bd9cac,f897948,da24efe): clears the compiler/native/lint warnings surfaced by the last green CI build and a full uncached local gate run, and caps workflow jobs with runtime timeouts. No dependency-version changes.c802f8e,090574b,bbd5d4a,7bacf09): a serious UDP send-counter wrap fix (ChaCha20 nonce reuse), plus review follow-ups on the sweep itself. The branch is no longer behavior-neutral —c802f8echanges the crypto send path and session lifecycle.What changed
SERIOUS fix: send-counter wrap → nonce reuse (
c802f8e)satellite_jni.cppdrew the ChaCha20-Poly1305 nonce counter from auint32 fetch_addwith no exhaustion guard: at 2^32 packets in one unbroken session it wrapped and re-sealed under reused (key, nonce) pairs until heartbeat death (contract §Crypto forbids exactly this; horizon ~50 days at 1 kHz).send_counter.h) and goes silent past 2^32−1;getSendCounter()clamps at the wire max; the 1 Hz Kotlin alive-poll fires a single-shotonRekeyNeededonce the counter crosses0xF0000000(counterNeedsRepush); the manager re-PUTs the session for fresh token/salt/key, restarting the counter at 1 long before exhaustion.runMgrTestnow tears sessions down in afinally(an assertion failure used to skip teardown and spin the virtual-time drain into OOM).Sweep follow-ups (review findings on this PR)
090574b— the sweep's class-wide test@Suppress("DEPRECATION")did not match production (which scopes to the call site) and would have masked any new deprecation in those classes. Narrowed to the exact legacy call sites inRumbleRouterTest,BluetoothConnectionsTest,BluetoothDeviceScannerTest,BluetoothBondMonitorTest.bbd5d4a— the sweep claimed every job was capped, but the five_security.ymljobs had notimeout-minutes. Capped at 10 min each (warm runtimes 5–10 s). The sibling repos' copies of the shared file are still uncapped — cross-repo sync is a follow-up.7bacf09—HAVE_SYSCONF=1was justified only as warning-silencing; it is a behavior change to libsodium (runtime page size viasysconfforsodium_mlock/guarded allocations — what Android 15's 16 KB-page devices need). The comment now says so. Not exercisable in the host gtest harness; on-device coverage rides the emulator integration suite.Native / build (
fa1c35e)CMP0135policy set explicitly (NEW), removing theDOWNLOAD_EXTRACT_TIMESTAMPFetchContent dev warning.HAVE_SYSCONF=1for the vendored libsodium build (see7bacf09above for the real rationale). NDK debug build is 0 native warnings.Kotlin + Android Lint (
9bd9cac) — 12 production Kotlin compiler warnings cleared (these were hidden in CI by the Gradle build cache; an uncached--rerun-taskscompile surfaces them): unused expression results, override param-name mismatches, deprecated-override annotations on mandatoryDrawable.getOpacity()overrides, a K2 always-true condition, unnecessary safe-calls after smart-casts, and one Java platform-type nullability. Plus 3 Lint fixes:EmptySuperCall,ObsoleteSdkInt,UseKtx.Workflow hygiene (
f897948) — addedtimeout-minutesto the dish-android-owned jobs (build 30, play-listing 20, play-reviews 15, release jobs 10–60);bbd5d4acompletes the claim for_security.yml. All 13 action pin-maptag→SHAcomments verified against upstream (none stale). 40-char SHA pins kept.Comment trim (
da24efe) — sweep-added comments cut to terse why-only house style.Test evidence (branch head
7bacf09)All CI gates re-run locally at this head: clang-format (22.1.5 local vs 22.1.4 CI pin — no diffs), Play-metadata lint, ktlint + detekt, Android Lint, 1,542 JVM unit tests (0 failures), 166/166 native tests,
assembleDebug+assembleDebugAndroidTest— all green; native build and all touched Kotlin files warning-clean. Fail-before verified for the counter fix: neutering the guard fails 4 native tests; neutering the predicate fails 3 connection tests + the manager re-PUT test.Deliberately deferred (out of scope)
AndroidGradlePluginVersion+ 1GradleDependencyLint warnings stay until CodeQL supports it.PhysicalReachabilityobject usages, 2 USELESS_CAST, platform-nullability) — pre-date this branch; naive fixes trade one warning for another; recommend a focused pass.allWarningsAsErrors/ lintwarningsAsErrors) is not enabled yet — recommended once the above land and the tree is fully clean._security.ymltimeout caps for the sibling repos' copies of the shared workflow — sync convention follow-up.Unverified
connectedDebugAndroidTest) runs only on an emulator/CI — this PR's CI run is the arbiter. The re-key path itself is unit-tested at the JNI-boundary seam; the end-to-end rotation against a live satellite rides the existing session-PUT integration coverage.🤖 Generated with Claude Code