Conversation
…terator * fix: whiteListRemove use-after-free - stop iterating after erase + shrink whiteListRemove erases the matched element (invalidating the iterator) and shrink-to-fits m_whiteList (freeing the buffer the iterator points into), then continues the loop on the dangling iterator. A layout-dependent spurious re-match then calls erase() with a foreign iterator, and the vector's internal element move writes across unrelated heap memory. Observed on ESP32-C3 as intermittent heap corruption under whitelist churn (multi_heap_free bad-head asserts with address bytes in the corrupted header; a clobbered NimBLE host mutex tripping xQueueSemaphoreTake). Verified fixed under CONFIG_HEAP_POISONING_COMPREHENSIVE plus periodic heap_caps_check_integrity_all() sweeps. whiteListAdd guards duplicates via onWhiteList(), so at most one element can match - breaking out after the removal preserves semantics. * fix: use m_whiteList.data() - operator[] on an empty vector is UB After erasing the last whitelist entry, &m_whiteList[0] indexes an empty vector before ble_gap_wl_set(..., 0). data() is well-defined for empty vectors; same change applied to the whiteListAdd call site for consistency.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR updates whitelist synchronization and removal handling. It adds lazy scan-response timer initialization, guarded cleanup, host-deinitialization handling, and failure reporting for ChangesScan and whitelist lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NimBLEDevice
participant NimBLEScan
participant ScanResponseCallout
participant NimBLEHost
NimBLEDevice->>NimBLEScan: onHostDeinit()
NimBLEScan->>NimBLEScan: clearWaitingList()
NimBLEScan->>ScanResponseCallout: deinit()
NimBLEDevice->>NimBLEHost: stop and deinitialize
NimBLEDevice->>NimBLEDevice: log and return false if nimble_port_stop() fails
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/NimBLEScan.cpp`:
- Around line 699-702: Move the NimBLE scan host-deinitialization call from its
current pre-stop location into the successful `rc == 0` shutdown path,
immediately after `nimble_port_stop()` and before `nimble_port_deinit()`. Update
the flow around `NimBLEDevice::m_pScan->onHostDeinit()` so
`ble_npl_callout_deinit()` in `onHostDeinit()` cannot run while NimBLE task
callbacks remain queued.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c44d8930-28ae-459e-b3bc-1913c24e4c79
📒 Files selected for processing (3)
src/NimBLEDevice.cppsrc/NimBLEScan.cppsrc/NimBLEScan.h
Fixes a crash when the stack is reinitialized without clearing all data on deinit then starting scanning.
Summary by CodeRabbit