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 |
![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
🤖 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
| if (m_srTimerInitialized) { | ||
| ble_npl_callout_deinit(&m_srTimer); | ||
| m_srTimerInitialized = false; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
: "${IDF_PATH:?Set IDF_PATH to a supported ESP-IDF checkout.}"
fd -a -t f 'npl_os_freertos\.c' "$IDF_PATH" |
while IFS= read -r source; do
rg -n -C 3 \
'ble_npl_eventq_put|npl_freertos_callout_stop|npl_freertos_callout_deinit|ble_npl_event_deinit' \
"$source"
doneRepository: h2zero/esp-nimble-cpp
Length of output: 229
🏁 Script executed:
#!/bin/sh
set -eu
echo "== tracked files matching NimBLEScan/nimble =="
git ls-files | rg '(^|/)(NimBLEScan\.cpp|NimBLEDevice\.cpp|.*nimble.*|.*NimBLE.*\.cpp$)' | sed -n '1,200p'
echo
echo "== relevant NimBLEScan references =="
file="$(git ls-files | rg '(^|/)NimBLEScan\.cpp$' | head -n 1)"
if [ -n "$file" ]; then
wc -l "$file"
rg -n -C 4 "onHostDeinit|ble_npl_callout_|srTimer|m_srTimerInitialized" "$file" || true
fi
echo
echo "== relevant NimBLEDevice references =="
file2="$(git ls-files | rg '(^|/)NimBLEDevice\.cpp$' | head -n 1)"
if [ -n "$file2" ]; then
wc -l "$file2"
rg -n -C 5 "nimble_port_stop|nimble_port_deinit|onHostDeinit|BLE_ROLE_OBSERVER" "$file2" || true
fi
echo
echo "== esp-idf npl sources in local checkout if present =="
if [ -n "${IDF_PATH:-}" ] && [ -d "$IDF_PATH" ]; then
fd -a -t f 'npl_os_freertos\.c$' "$IDF_PATH" | while IFS= read -r source; do
echo "--- $source ---"
rg -n -C 5 "ble_npl_eventq_put|npl_freertos_callout_stop|npl_freertos_callout_deinit|ble_npl_event_deinit|struct ble_npl_eventq|struct ble_npl_callout" "$source" || true
done
else
echo "IDF_PATH not set or empty"
fiRepository: h2zero/esp-nimble-cpp
Length of output: 8660
🌐 Web query:
ESP-IDF NimBLE NPL ble_npl_callout_stop pending event ble_npl_callout_deinit queue callout event storage
💡 Result:
In the NimBLE Network Porting Layer (NPL) within ESP-IDF, the management of callouts, events, and their associated storage relies on specific NPL functions to ensure system stability. Callout Management and Pending Events The function ble_npl_callout_stop is used to stop a scheduled callout from firing [1][2]. When called, it effectively removes the pending timer event, ensuring that the associated event is not posted to the event queue [3][2]. If a callout is active, it can be verified using ble_npl_callout_is_active [1][4]. Stopping an active callout is a necessary step before deinitialization to prevent race conditions or unexpected execution of the callback after the storage has been invalidated [5][6]. Deinitialization and Storage The ble_npl_callout_deinit function is intended to perform cleanup for a callout structure [7][5]. In practice, robust implementations (including those used in Rust wrappers for ESP-IDF) explicitly call ble_npl_callout_stop inside the deinit routine to ensure that any pending timer or event associated with the callout is fully cleared before the underlying storage is freed or repurposed [5][6]. Key considerations for handling callout event storage include: 1. Ownership: The caller is responsible for the lifecycle of the memory allocated for the struct ble_npl_callout [1]. 2. Synchronization: Always ensure that the callout is stopped using ble_npl_callout_stop before deinitializing the structure to avoid attempts to access invalidated event queues or callout metadata [5][8]. 3. Event Queue: Callouts are tied to a specific event queue during initialization via ble_npl_callout_init [1][2]. The event itself is posted to this queue upon timer expiration [2][4]. Removing the callout prevents this final queue operation [3][2]. In summary, to safely handle pending events and deinit callout storage: - Call ble_npl_callout_stop(co) to clear pending timers [1][2]. - Verify the callout is no longer active if necessary [1][4]. - Call ble_npl_callout_deinit(co) to finalize the cleanup [7][5].
Citations:
- 1: https://sourcevu.sysprogs.com/espressif/esp-idf/files/components/bt/host/nimble/nimble/nimble/include/nimble/nimble_npl.h
- 2: https://api.riot-os.org/nimble__npl__os_8h.html
- 3: https://github.com/apache/mynewt-nimble/blob/master/porting/npl/riot/include/nimble/nimble_npl_os.h
- 4: https://doc.riot-os.org/nimble__npl__os_8h.html
- 5: https://github.com/esp-rs/esp-hal/blob/667f8f02/esp-radio/src/ble/npl.rs
- 6: https://docs.espressif.com/projects/rust/esp-radio/0.16.0/esp32c3/src/esp_radio/ble/npl.rs.html
- 7: https://sourcevu.sysprogs.com/espressif/esp-idf/files/components/bt/host/nimble/nimble/porting/npl/freertos/include/nimble/nimble_npl_os.h
- 8: https://docs.espressif.com/projects/rust/esp-radio/1.0.0-beta.0/esp32h2/src/esp_radio/ble/npl.rs.html?search=
🌐 Web query:
sourcevu sysprogs ESP-IDF npl_os_freertos.c npl_freertos_callout_stop pending event ble_npl_eventq_put
💡 Result:
In the ESP-IDF implementation of the NimBLE Porting Layer (NPL), npl_freertos_callout_stop is a function defined in npl_os_freertos.c responsible for stopping a pending NimBLE callout timer [1][2]. When a NimBLE callout expires, the callback function os_callout_timer_cb is triggered [3]. If the callout is associated with an event queue (co->evq), it utilizes ble_npl_eventq_put to place the event onto the specified queue for processing [3]. If no event queue is associated, the callback executes the function directly [3]. The npl_freertos_callout_stop function ensures that the underlying FreeRTOS timer (or esp_timer, depending on the configuration) associated with the callout is stopped [2]. This prevents the timer from firing and potentially queueing a stale event via ble_npl_eventq_put if the callout has been explicitly stopped or cancelled by the application [3][2]. Issues involving the interaction between stopping these callouts and pending events have been addressed in ESP-IDF patches to ensure correct synchronization and state management [4].
Citations:
- 1: https://sourcevu.sysprogs.com/espressif/esp-idf/symbols/npl_freertos_callout_stop
- 2: https://sourcevu.sysprogs.com/espressif/esp-idf/files/components/bt/host/nimble/nimble/porting/npl/freertos/src/npl_os_freertos.c
- 3: https://github.com/apache/mynewt-nimble/blob/master/porting/npl/freertos/src/npl_os_freertos.c
- 4: espressif/esp-idf@bb4eeb8
🌐 Web query:
apache mynewt nimble porting npl freertos npl_os_freertos.c ble_npl_eventq_put ble_npl_callout_stop
💡 Result:
In Apache Mynewt NimBLE, the NimBLE Porting Layer (NPL) serves as an abstraction layer to allow the NimBLE stack to run on different operating systems, including FreeRTOS [1][2]. The file npl_os_freertos.c contains the FreeRTOS-specific implementation of these NPL primitives, bridging NimBLE's requirements to FreeRTOS kernel objects [3][4]. Key NPL functions for event queues and callouts are implemented as follows in the FreeRTOS port: ble_npl_eventq_put: This function is used to insert an event into an NPL event queue [3]. The implementation typically uses xQueueSendToBack to push the event onto a FreeRTOS queue [3][4]. If the function is called from an Interrupt Service Routine (ISR), it uses xQueueSendToBackFromISR and triggers a context switch via portYIELD_FROM_ISR if required by the FreeRTOS configuration [3][4]. ble_npl_callout_stop: This function is responsible for stopping an active timer callout [5][6]. In the FreeRTOS port, callouts are generally implemented using FreeRTOS software timers (xTimerHandle) [3][4]. When ble_npl_callout_stop is invoked, it typically calls xTimerStop (or xTimerStopFromISR if in an interrupt context) to halt the underlying FreeRTOS timer [4]. The NPL interface is defined by headers like nimble_npl_os.h [5][1]. While some simple NPL functions are implemented as static inline functions directly in the headers, more complex logic—such as event queue management and timer-based callouts—relies on the C implementation found in npl_os_freertos.c to manage global state and interact directly with FreeRTOS APIs [3][5].
Citations:
- 1: Porting NimBLE apache/mynewt-nimble#72
- 2: https://github.com/apache/mynewt-nimble/blob/master/README.md
- 3: https://github.com/apache/mynewt-nimble/blob/master/porting/npl/freertos/src/npl_os_freertos.c
- 4: https://github.com/lupyuen/bl_iot_sdk/blob/lorarecv/customer_app/sdk_app_lora/sdk_app_lora/npl_os_freertos.c
- 5: https://github.com/apache/mynewt-nimble/blob/master/porting/npl/freertos/include/nimble/nimble_npl_os.h
- 6: https://sourcevu.sysprogs.com/espressif/esp-idf/files/components/bt/host/nimble/nimble/porting/npl/freertos/include/nimble/nimble_npl_os.h
🌐 Web query:
git diff esp-idf bb4eeb8edc65308be85c3f26a9eb2fb2b93e4467 npl_os_freertos.c ble_npl_callout_stop ble_npl_eventq_remove
💡 Result:
The git commit bb4eeb8edc65308be85c3f26a9eb2fb2b93e4467 in the ESP-IDF repository is titled "fix(bt): fixed the npl callout stop issue" [1]. It was authored by ShenWeilong and merged on June 5, 2026 [1]. This commit addresses issues within the NimBLE Porting Layer (NPL) for FreeRTOS [1]. The changes specifically modify components/bt/porting/npl/freertos/src/npl_os_freertos.c (along with corresponding files in porting_btdm) to resolve an issue related to the ble_npl_callout_stop function [1]. While the commit title mentions ble_npl_callout_stop, the actual file modifications involve adding logic to handle callout operations more robustly within the ESP-IDF environment [1]. This fix ensures that callout stopping is handled correctly in the NPL layer, which interacts with ble_npl_eventq_remove and other NPL functions [1][2][3]. The NPL layer in ESP-IDF adapts the Apache NimBLE stack to work on top of FreeRTOS [1][2], and this commit is a specific maintenance patch for that integration [1].
Citations:
- 1: espressif/esp-idf@bb4eeb8
- 2: https://sourcevu.sysprogs.com/espressif/esp-idf/files/components/bt/host/nimble/nimble/porting/npl/freertos/include/nimble/nimble_npl_os.h
- 3: https://sourcevu.sysprogs.com/espressif/esp-idf/symbols/npl_freertos_callout_stop
🌐 Web query:
"ble_npl_eventq_remove" "ble_npl_callout_stop" npl_os_freertos.c
💡 Result:
The identifiers ble_npl_eventq_remove and ble_npl_callout_stop are functions defined as part of the NimBLE Porting Layer (NPL) [1][2]. The file npl_os_freertos.c acts as the implementation of this portability layer specifically for the FreeRTOS operating system [3][4]. ble_npl_eventq_remove This function is used to remove a specific event from a NimBLE event queue [5]. In the context of the FreeRTOS port (npl_os_freertos.c), implementation details note the technical difficulty of removing an arbitrary element from a standard FreeRTOS queue, often necessitating workarounds such as draining the queue and re-adding events that are not intended for removal [3]. ble_npl_callout_stop This function is used to stop an active NimBLE callout (timer) [2][5]. In the FreeRTOS implementation, this function typically maps to xTimerStop, which halts the associated FreeRTOS timer handle [1][3]. These functions are defined within the NimBLE NPL header structure (nimble_npl_os.h) to abstract OS-specific primitives away from the core Bluetooth stack [1][2]. You can find these implementations in the Apache Mynewt NimBLE repository under the directory porting/npl/freertos/ [3].
Citations:
- 1: https://github.com/apache/mynewt-nimble/blob/master/porting/npl/freertos/include/nimble/nimble_npl_os.h
- 2: https://sourcevu.sysprogs.com/espressif/esp-idf/files/components/bt/host/nimble/nimble/nimble/include/nimble/nimble_npl.h
- 3: https://github.com/apache/mynewt-nimble/blob/master/porting/npl/freertos/src/npl_os_freertos.c
- 4: https://github.com/lupyuen/bl_iot_sdk/blob/lorarecv/customer_app/sdk_app_lora/sdk_app_lora/npl_os_freertos.c
- 5: https://api.riot-os.org/nimble__npl__os_8h.html
🌐 Web query:
"esp-idf" "bb4eeb8edc65308be85c3f26a9eb2fb2b93e4467" "npl_os_freertos.c"
💡 Result:
The hash bb4eeb8edc65308be85c3f26a9eb2fb2b93e4467 corresponds to a specific commit in the Espressif ESP-IDF GitHub repository [1]. This commit, authored on June 5, 2026, is titled "fix(bt): fixed the npl callout stop issue" [1]. It modifies several files related to the NimBLE Bluetooth porting layer (NPL) for FreeRTOS [1]. The file npl_os_freertos.c is one of the files updated by this commit, specifically receiving modifications to address callout stop issues within the NPL implementation [1]. The file npl_os_freertos.c is a core component of the NimBLE Porting Layer (NPL) for FreeRTOS, which allows the NimBLE Bluetooth stack to run on FreeRTOS-based systems like the ESP-IDF [2][3]. It handles OS-specific abstractions such as event queues, mutexes, and callouts (timers) [2][4]. Developers often encounter this file in stack traces or debug logs when troubleshooting Bluetooth-related assertions or issues in ESP-IDF applications [5].
Citations:
- 1: espressif/esp-idf@bb4eeb8
- 2: https://sourcevu.sysprogs.com/espressif/esp-idf/files/components/bt/host/nimble/nimble/porting/npl/freertos/src/npl_os_freertos.c
- 3: https://github.com/apache/mynewt-nimble/blob/master/porting/npl/freertos/src/npl_os_freertos.c
- 4: https://sourcevu.sysprogs.com/espressif/esp-idf/symbols/npl_freertos_eventq_get
- 5: ESP IDF 5.3.2 assert ble_hs_locked_by_cur_task (IDFGH-14217) espressif/esp-idf#15014
🌐 Web query:
github apache mynewt-nimble npl_os_freertos.c ble_npl_eventq_remove
💡 Result:
In the Apache Mynewt NimBLE project, ble_npl_eventq_remove is a function in the NimBLE Porting Layer (NPL) used to remove a specific event from an event queue [1][2]. For FreeRTOS, the implementation is located in the file porting/npl/freertos/src/npl_os_freertos.c under the function name npl_freertos_eventq_remove [3][4]. The implementation functions by iterating through the underlying FreeRTOS queue [3][4]. Because FreeRTOS queues do not natively support removing an arbitrary element from the middle of the queue, the NPL implementation uses a workaround [3][4]: 1. It drains the queue by removing all elements and storing them temporarily [3][4]. 2. It then re-adds all elements back into the queue except for the specific event that is intended to be removed [3][4]. 3. This process is protected by critical sections (using vPortEnterCritical/vPortExitCritical or taskENTER_CRITICAL/taskEXIT_CRITICAL) to ensure thread safety when called from a task, or by using ISR-safe queue functions when called from an Interrupt Service Routine (ISR) [3][4]. The public API ble_npl_eventq_remove in nimble_npl_os.h typically acts as an inline wrapper that calls this underlying npl_freertos_eventq_remove function [1][5].
Citations:
- 1: https://github.com/apache/mynewt-nimble/blob/master/porting/npl/freertos/include/nimble/nimble_npl_os.h
- 2: https://doc.riot-os.org/nimble__npl__os_8h.html
- 3: https://github.com/apache/mynewt-nimble/blob/master/porting/npl/freertos/src/npl_os_freertos.c
- 4: https://github.com/lupyuen/bl_iot_sdk/blob/lorarecv/customer_app/sdk_app_lora/sdk_app_lora/npl_os_freertos.c
- 5: https://sourcevu.sysprogs.com/espressif/esp-idf/files/components/bt/host/nimble/nimble/porting/npl/freertos/include/nimble/nimble_npl_os.h
Do not deinitialize callouts before nimble_port_stop() completes.
onHostDeinit() is called before nimble_port_stop(), while the callout event runs from NimBLE’s task after deinit() starts. If srTimerCb has already queued its event, ble_npl_callout_deinit() can free callout storage before that queued callback runs.
Call NimBLEDevice::m_pScan->onHostDeinit() inside rc == 0 after nimble_port_stop() and before nimble_port_deinit().
🤖 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 `@src/NimBLEScan.cpp` around lines 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.
Fixes a crash when the stack is reinitialized without clearing all data on deinit then starting scanning.
2 participants
Summary by CodeRabbit