fix: harden against adversarial mesh-fuzz findings (crash, GC-thrash, unbounded growth, OOM)#6093
Merged
Merged
Conversation
Adversarial mesh-fuzz testing (a hostile replay session sending corrupted NodeInfo/User protos) found that a single malformed packet buffered before the node DB / local node number are ready could crash the entire app. flushEarlyReceivedPackets() replays buffered packets in a bare scope.launch, unlike the live receive path (processFromRadio) which is already guarded by safeCatching. A packet that fails deep inside handleReceivedData -> handleNodeInfo's proto decode (ProtocolException / EOFException) threw out of that coroutine uncaught, crashing the process (observed live: FATAL EXCEPTION at MeshMessageProcessorImpl.kt:214, "Fdroid Debug keeps stopping"). Wrap each buffered packet in the same safeCatching pattern already used in processFromRadio, so one bad packet is logged and dropped instead of taking down the rest of the batch (and the app). Added a regression test that fails without the fix and passes with it.
…et flood DebugViewModel.meshLog re-decoded up to DEFAULT_MAX_LOGS (5000) rows -- including proto toString() and a full node-DB snapshot per row -- on every single mesh_log table insert, via Room's auto-invalidating Flow + mapLatest. Under a packet flood this can restart faster than it completes, generating a firehose of short-lived garbage that starves the GC (matches a live incident where a 70-minute adversarial soak pinned the Dalvik heap at 98.7% with the main thread stuck in WaitForGcToComplete loops). - debounce the decode pipeline (200ms) so a flood can't retrigger a full re-decode faster than the previous one can complete. - hoist the per-row nodeDBbyNum.values.toList() snapshot out of the loop in toUiState/annotatePacketLog -- it was rebuilt once per row instead of once per decode pass, an O(rows * nodes) hotspot that compounded under both a packet flood and a large mesh (see push_fake_nodedb's 2000-node fixture). Verified live post-fix: re-ran the same seeded chaos-fuzz campaign that previously showed heap climbing with the Debug Panel open for 200+ seconds with stable memory and no crash.
…bounded RadioConfigRepositoryImpl.addFileInfo() did an unconditional _fileManifestFlow.value += info. clearFileManifest() only runs at the start of the *next* handshake, so nothing else bounded this accumulator in between -- a rogue/adversarial peer emitting FileInfo packets in a loop for the life of a session grows it without limit. A real device's manifest is bounded by its own flash storage (realistically a handful to a few hundred entries), so this only bites under a malformed/ hostile stream, but it's a real unbounded-memory-growth vector for a long-running session. Cap it at 4096 entries, same defensive pattern as the early-packet buffer in MeshMessageProcessorImpl, and add tests covering the cap and the handshake-clear behavior.
…ions Reproduced a genuine OutOfMemoryError live: a 2000-node NodeDB (push_fake_nodedb's own largest fixture -- large public-event meshes like MeshCon/DEF CON/Burning Man are a supported scenario, not just an adversarial edge case) combined with sustained chaos-fuzz traffic and repeated reconnects in one long-lived process exhausted the default ~256MB Dalvik heap. The crash surfaced on an unrelated DataStore settings read (androidx.datastore.core.DataStoreImpl.readAndInitOrPropagateAndThrowFailure) -- any allocation would have thrown at that point, the heap was simply full. android:largeHeap raises the ceiling (device-dependent, commonly 2-4x) rather than reducing footprint; the debounce/cap fixes in this branch are the real long-term mitigation. Verified live: re-ran the identical crash-inducing sequence (2000-node chaos fuzz + 3 rapid reconnect cycles) against the same device post-fix (confirmed dalvik.vm.heapsize=512m active) and it survived cleanly where it previously OOM'd.
Adds continuous, automatic leak detection (heap dump + leak trace +
notification) for future soak/fuzz runs instead of manually polling
dumpsys meminfo. Debug-only (0 method count in release, per LeakCanary's
own design). Pinned to the latest stable release (2.14), not the 3.0-alpha
line. Confirmed it initializes correctly on-device ("LeakCanary is running
and ready to detect memory leaks") and stayed clean across an extended
adversarial soak.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds debug-only LeakCanary and a largeHeap manifest flag, hardens buffered mesh packet replay, caps file-manifest growth, and debounces debug log decoding while reusing precomputed node data. ChangesDiagnostics, resilience and debug log throttling
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
androidApp/src/main/AndroidManifest.xml (1)
121-136: 🩺 Stability & Availability | 🔵 Trivial
largeHeapraises GC pause risk — confirm this is truly a stopgap.The comment is thorough and self-aware about the tradeoff, but note that enabling
largeHeapincreases the working set the GC scans each cycle, which can increase pause times/jank across the whole app, not just the fuzz/reconnect path — this could be a net regression for typical (non-fuzz) sessions on low-memory devices. Since the comment says the "real fix" (bounding/streaming caches) is still pending, consider tracking removal of this flag as a follow-up once that work lands, and validate on low-end/low-memory devices, not just the flagship test device.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/main/AndroidManifest.xml` around lines 121 - 136, The AndroidManifest’s largeHeap setting should be treated explicitly as a temporary stopgap rather than a permanent fix. Update the manifest comment around android:largeHeap in AndroidManifest to state that removal is a follow-up once the cache bounding/streaming work in DebugViewModel and MeshMessageProcessorImpl lands, and make sure the change is validated on low-memory devices as well as the current soak target. If there is a tracking task or TODO mechanism in this codebase, link this flag to that removal plan so the tradeoff is visible and actionable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/RadioConfigRepositoryImpl.kt`:
- Around line 131-143: The addFileInfo update path is racy because it reads and
writes _fileManifestFlow.value without serialization, so concurrent calls from
scope.handledLaunch can overwrite each other and bypass the
MAX_FILE_MANIFEST_ENTRIES cap. Fix RadioConfigRepositoryImpl.addFileInfo by
making the manifest update atomic, either with MutableStateFlow.update or by
guarding the read/append/cap logic with a Mutex, while keeping the existing cap
and log behavior intact.
---
Nitpick comments:
In `@androidApp/src/main/AndroidManifest.xml`:
- Around line 121-136: The AndroidManifest’s largeHeap setting should be treated
explicitly as a temporary stopgap rather than a permanent fix. Update the
manifest comment around android:largeHeap in AndroidManifest to state that
removal is a follow-up once the cache bounding/streaming work in DebugViewModel
and MeshMessageProcessorImpl lands, and make sure the change is validated on
low-memory devices as well as the current soak target. If there is a tracking
task or TODO mechanism in this codebase, link this flag to that removal plan so
the tradeoff is visible and actionable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 36d53ca1-7c75-43f8-8992-a9372fba562b
📒 Files selected for processing (8)
androidApp/build.gradle.ktsandroidApp/src/main/AndroidManifest.xmlcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshMessageProcessorImpl.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/RadioConfigRepositoryImpl.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshMessageProcessorImplTest.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/repository/RadioConfigRepositoryImplTest.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/debugging/DebugViewModel.ktgradle/libs.versions.toml
Review feedback on #6093: - addFileInfo did a non-atomic read-then-write on _fileManifestFlow. Since handleFileInfo dispatches each packet on its own scope.handledLaunch, concurrent calls could race -- lost updates, or two callers both seeing a sub-cap size and bypassing MAX_FILE_MANIFEST_ENTRIES. Switched to MutableStateFlow.update{} (atomic CAS loop). Added a concurrency regression test (fire 2000 concurrent adds, assert none are lost); verified it fails against the old read-then-write and passes with update{}. - largeHeap: expanded the manifest comment to spell out that it's a temporary, whole-app stopgap with a global cost (larger GC working set -> more pause/jank for ALL sessions, including small-mesh use on low-RAM devices that never hit the OOM it guards), and that removal is a tracked follow-up once the cache-bounding work lands -- to be validated on a low-RAM device, not just the flagship soak target.
Collaborator
Author
|
Both addressed in 23f066b: Race in
|
The largeHeap comment expansion in the previous commit introduced a literal
"--" inside the XML comment ("for ALL sessions -- including..."), which is
illegal in XML and broke manifest parsing -- failing every androidApp-dependent
CI check (android-check, lint-check, shard-app) while core modules passed.
Reworded to avoid the double-dash. Verified locally with
:androidApp:processFdroidDebugMainManifest and :androidApp:testFdroidDebugUnitTest.
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.
Found and fixed 4 real bugs by adversarially testing the app against a live replay-fuzz harness (via
meshtastic-mcp's replay engine) on a physical Pixel 6a — corrupted/malformed packets, connection churn, and large-mesh scale, escalating throughlight→parser→adversary→chaosfuzz presets. All four are reproduced live and each has a fix + regression test (or a live before/after re-run where a unit test wasn't practical).🐛 Bug Fixes
flushEarlyReceivedPackets()replayed buffered packets in a barescope.launch, unlike the live receive path which is guarded bysafeCatching. A single corrupted NodeInfo/User packet arriving before the node DB / local node number resolve threw an uncaughtProtocolException/EOFExceptionand crashed the whole process. Reproduced twice with a seededchaosfuzz run. Fixed by wrapping each buffered packet the same way the live path already is.DebugViewModel.meshLogre-decoded up to 5000 log rows — including prototoString()and a full node-DB snapshot per row — on every single packet insert, via Room's auto-invalidatingFlow+mapLatest. Under a packet flood this can restart faster than it completes, generating a firehose of garbage (matches a prior live incident where a 70-minute soak pinned the Dalvik heap at 98.7%). Fixed with a 200ms debounce and by hoisting the per-row node-list snapshot out of the loop.RadioConfigRepositoryImpl.addFileInfo()did an unconditional+= infowith no cap;clearFileManifest()only runs on the next handshake, so a rogue peer emitting FileInfo packets in a loop grows it without limit for the life of a session. Capped at 4096 entries, same defensive pattern as the existing early-packet buffer.push_fake_nodedbfixture — large public-event meshes like MeshCon/DEF CON/Burning Man are a supported scenario) + sustainedchaosfuzz + rapid reconnects in one process exhausted the default ~256MB heap. Addedandroid:largeHeap="true"; re-ran the identical crash-inducing sequence post-fix and it survived cleanly at the new 512MB ceiling.🧹 Chores
Testing Performed
MeshMessageProcessorImplTest(regression test verified to fail without the fix, pass with it),RadioConfigRepositoryImplTest(new file, cap + handshake-clear behavior)../gradlew spotlessCheck detektclean across the whole repo../gradlew :core:data:jvmTest :feature:settings:jvmTestpass.Summary by CodeRabbit
New Features
Bug Fixes
Tests