From 168f669e613ee4eb8467db7f335181b1cf5b4903 Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Thu, 9 Jul 2026 09:42:35 -0500 Subject: [PATCH] esp32: fix NimBLE use-after-free crash when disabling bluetooth while connected (#10950) * esp32: fix NimBLE use-after-free crash when disabling bluetooth while connected Saving certain configs over BLE (e.g. MQTT module config) calls disableBluetooth() -> NimbleBluetooth::deinit() while the phone is still connected. deinit() called BLEDevice::deinit(true) straight away, which deletes the BLEServer before nimble_port_stop(). Stopping the NimBLE port with a live connection makes the host synthesize unsubscribe events via ble_gatts_connection_broken() and dispatch them into the freed BLEServer -> LoadProhibited panic in BLEServer::handleGATTServerEvent (iterating server->m_notifyChrVec on freed memory). Backtrace tail: BLEServer::handleGATTServerEvent (SUBSCRIBE) <- ble_gatts_subscribe_event <- ble_gatts_connection_broken <- ble_gap_conn_broken <- ble_gap_rx_disconn_complete It was reliably preceded by our onRead callback pinning the NimBLE host task in its up-to-20s polling loop ('BLE onRead: timeout after 19920 ms, 4000 tries'), so the host task couldn't process the teardown until long after the objects under it were freed. Fix: drain before demolition in deinit(). Clear onReadCallbackIsWaitingForData so an in-flight onRead returns immediately; disconnect the peer and wait (bounded, ~2s) for the host task to run onDisconnect (which clears nimbleBluetoothConnHandle) so the unsubscribe/GATT teardown lands on the still-live server; only then BLEDevice::deinit(true). isDeInit is raised after the drain (onDisconnect early-returns when set and would otherwise never clear the handle), the deferred advertising restart is dropped (the stack is going away), and the now-dangling bleServer pointer is nulled. deinit() runs on the main task, so waiting on the host task's callbacks is safe; the waits are bounded regardless. Upstream ordering bug (delete BLEServer before nimble_port_stop) to be reported to espressif/arduino-esp32 separately. * esp32: gate onRead during BLE teardown and use Throttle for drain wait Addresses review feedback on the NimBLE deinit UAF fix: - Set a bleDraining flag before disconnecting so onRead bails immediately instead of arming the up-to-20s wait. Without this, a read that re-arms after deinit() clears onReadCallbackIsWaitingForData could pin the NimBLE host task, block onDisconnect from clearing the conn handle, and make the 2s drain expire. onRead now skips the wait when draining, and the wait loop also observes the flag so an in-flight read is released. - Use Throttle::isWithinTimespanMs() for the bounded drain wait instead of a raw millis() subtraction, per coding guidelines. * esp32: reset BLE teardown guards on re-init deinit() latches bleDraining (new) and isDeInit (pre-existing) as teardown guards, but setup() never cleared them. AdminModule's disable-bluetooth admin command calls deinit() directly without a reboot, and PowerFSM can re-enable via setBluetoothEnable(true) -> setup() on the same boot (isActive() is false because deinit() nulls bleServer). Without resetting the flags, the re-initialized stack has onRead permanently bailing on the drain path (empty reads) and onDisconnect early-returning without clearing the connection handle. Clear both flags at the top of setup() so a re-init recovers cleanly. --- src/nimble/NimbleBluetooth.cpp | 41 ++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/src/nimble/NimbleBluetooth.cpp b/src/nimble/NimbleBluetooth.cpp index 3ea0aab31..4b52eae83 100644 --- a/src/nimble/NimbleBluetooth.cpp +++ b/src/nimble/NimbleBluetooth.cpp @@ -8,6 +8,7 @@ #include "concurrency/OSThread.h" #include "main.h" #include "mesh/PhoneAPI.h" +#include "mesh/Throttle.h" #include "mesh/mesh-pb-constants.h" #include "sleep.h" #include @@ -107,6 +108,10 @@ static std::atomic nimbleBluetoothConnHandle{BLE_HS_CONN_HANDLE_NONE}; // callback while the host is mid-reset crashes (LoadProhibited), so the main task does it instead. static std::atomic pendingStartAdvertising{false}; +// Set by deinit() before it disconnects. Makes onRead bail immediately instead of arming the +// up-to-20s wait, so a read arriving mid-teardown can't pin the NimBLE task and stall the disconnect. +static std::atomic bleDraining{false}; + static void clearPairingDisplay() { if (!passkeyShowing) { @@ -541,14 +546,18 @@ class NimbleBluetoothFromRadioCallback : public BLECharacteristicCallbacks #ifdef DEBUG_NIMBLE_ON_READ_TIMING LOG_DEBUG("BLE onRead(%d): packet already waiting, no need to set onReadCallbackIsWaitingForData", currentReadCount); #endif - } else { + } else if (!bleDraining) { + // (If deinit() is tearing the stack down, skip the wait entirely and just return a 0-size + // response below - arming the wait here could pin this NimBLE task for ~20s and stall teardown.) + // Tell the main task that we'd like a packet. bluetoothPhoneAPI->onReadCallbackIsWaitingForData = true; // Wait for the main task to produce a packet for us, up to about 20 seconds. // It normally takes just a few milliseconds, but at initial startup, etc, the main task can get blocked for longer // doing various setup tasks. - while (bluetoothPhoneAPI->onReadCallbackIsWaitingForData && tries < 4000) { + // bleDraining lets deinit() release an in-flight wait immediately. + while (bluetoothPhoneAPI->onReadCallbackIsWaitingForData && !bleDraining && tries < 4000) { // Schedule the main task runOnce to run ASAP. bluetoothPhoneAPI->setIntervalFromNow(0); concurrency::mainDelay.interrupt(); // wake up main loop if sleeping @@ -801,13 +810,35 @@ void NimbleBluetooth::deinit() { #ifdef ARCH_ESP32 LOG_INFO("Disable bluetooth until reboot"); + + // BLEDevice::deinit() deletes the BLEServer before nimble_port_stop(); doing that with a live + // connection dispatches synthesized unsubscribe events into the freed server (LoadProhibited), + // so disconnect cleanly first. deinit() runs on the main task; the waits below are bounded. + // bleDraining must be set before we clear the flag / disconnect, so a read arriving now bails + // instead of re-arming the ~20s wait and re-pinning the NimBLE task through the whole teardown. + bleDraining = true; + if (bluetoothPhoneAPI) + bluetoothPhoneAPI->onReadCallbackIsWaitingForData = false; // release any in-flight onRead + + // isDeInit must stay false here, else onDisconnect early-returns without clearing the handle. + uint16_t connHandle = nimbleBluetoothConnHandle.load(); + if (connHandle != BLE_HS_CONN_HANDLE_NONE && bleServer) { + bleServer->disconnect(connHandle); + uint32_t start = millis(); + while (nimbleBluetoothConnHandle.load() != BLE_HS_CONN_HANDLE_NONE && Throttle::isWithinTimespanMs(start, 2000)) + delay(10); + delay(50); + } + isDeInit = true; + pendingStartAdvertising = false; // stack is going away; don't let runOnce retry the adv restart #ifdef BLE_LED digitalWrite(BLE_LED, LED_STATE_OFF); #endif BLEDevice::deinit(true); + bleServer = nullptr; // deleted by deinit(); clear the dangling copy BatteryCharacteristic = nullptr; // freed by deinit; clear so updateBatteryLevel() won't touch it lastBatteryLevel = -1; #endif @@ -850,6 +881,12 @@ void NimbleBluetooth::setup() LOG_INFO("Init the NimBLE bluetooth module"); + // deinit() latches these teardown guards; clear them so a re-init on the same boot (e.g. an + // admin disable-bluetooth followed by re-enable) doesn't leave onRead stuck draining or + // onDisconnect early-returning without clearing the connection handle. + bleDraining = false; + isDeInit = false; + #ifdef ARCH_ESP32 // Runs before BLEDevice::init() reads the bond store, but logs after the "Init" line above so // any bond-cleanup output doesn't appear to precede the module init.