From 5cf85d523a205fb42d9ac1d34baf02c763bfacca Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Thu, 16 Jul 2026 15:13:26 -0500 Subject: [PATCH] fix: heap leaks on display wake and BLE re-enable; remove dead PacketCache (#11019) * fix(display): don't leak a TFT driver instance on every screen wake On variants whose Screen::handleSetOn() re-runs ui->init() on each display wake (Heltec Tracker V1.x/V2, VTFT_LEDA and ST7796 boards, MUZI/Cardputer via dispdev->init()), OLEDDisplay::init() unconditionally re-invokes connect(), and TFTDisplay::connect() allocated a fresh driver instance each time without freeing the old one. Each OFF->ON transition orphaned a full LGFX device (measured: exactly 1,260 bytes per wake on a Heltec Wireless Tracker V2). Since PowerFSM wakes the screen on every received text message, a device on a busy channel leaked tens of KB per day - matching field reports of heap climbing from 78% to 92% within a day. Null-guard the driver allocations (matching the existing linePixelBuffer/ repaintChunkBuffer guards below them) so re-entry re-runs tft->init() on the existing instance. Bench-validated on a Tracker V2: free heap now flat across 13 consecutive wake cycles, with the panel still re-initializing and rendering on every wake. * fix(ble): don't leak BluetoothPhoneAPI and callbacks on BLE re-enable NimbleBluetooth::setup()/setupService() re-run when Bluetooth is re-enabled after deinit(), but BLEDevice::deinit() only frees the GATT objects - the caller-owned allocations were re-created unguarded on every cycle: - bluetoothPhoneAPI: ~3.5KB PhoneAPI object per cycle, and the orphaned instance is an OSThread that stays registered with the scheduler, so a duplicate thread kept servicing the same static queues - toRadioCallbacks / fromRadioCallbacks / security + server callbacks: one small object each per cycle - the BLESecurity shim was heap-allocated and never freed even on first boot; it only forwards to static setters, so use a stack instance Reuse the existing objects on re-setup; the setCallbacks() calls still run every time since the characteristics themselves are new. * chore: remove unused PacketCache PacketCache landed in #8341 but no consumer was ever wired up - repo-wide, the only references to PacketCache/packetCache are in its own two files. As designed it also malloc()s per cached packet with no eviction or size cap, so it should be re-reviewed for bounds before any future use. Remove the dead code; git history preserves it if a bounded revival is wanted. * fix(ble): reset stale session state when reusing BluetoothPhoneAPI Review follow-up: reusing bluetoothPhoneAPI across BLE enable cycles could hand the next session the previous session's dirty state. The only full cleanup path (onDisconnect) is skipped when deinit()'s bounded disconnect wait expires before the event lands (its 2s cap matches the connection supervision timeout, so a phone that walked out of range makes this a coin flip): the reused object then enters the next session mid-config, with stale queue contents served to the new phone and a stale connection handle that defeats the checkConnectionTimeout self-heal. Factor onDisconnect's cleanup into resetBleSessionState() and run it from setupService() when reusing the instance, restoring the old fresh-object invariant. Also switch the four stateless callback objects to function- local statics (the resolved framework BLE wrapper stores plain pointers and never frees them, so static instances are safe and avoid the guarded heap allocations), correct the comment that claimed deinit() frees the GATT objects (it deletes only the BLEServer itself; the services and characteristics it created remain a small library-side leak per cycle), and fix the inverted connection check in getRssi() that made BLE RSSI always read 0 on ESP32-S3/C6. * refactor(display): construct-once guard around the whole driver ladder Review follow-up: one if (!tft) around the #if/#elif/#else construction block instead of a guard per branch, so a future display family can't reintroduce the per-wake leak by copying an unguarded branch. Make the HACKADAY bus pointer local to the construction (it was a write-only global), and point the comment at the Screen::handleSetOn gates instead of hand-listing boards that would go stale. * fix(ble): use-after-free notifying a freed characteristic after deinit() deinit() nulled bleServer and BatteryCharacteristic but left fromNumCharacteristic dangling after BLEDevice::deinit(true) freed the GATT graph, and it never detached the PhoneAPI fromNumChanged observer. When deinit()'s bounded 2s disconnect wait expires before onDisconnect runs (the same stale-bond / host-reset race resetBleSessionState was built for), close() is skipped, so the observer stays attached with state == STATE_SEND_PACKETS. After BLE is off, the next mesh packet drives MeshService fromNumChanged -> PhoneAPI::onNotify -> onNowHasData -> fromNumCharacteristic->notify() on freed memory (the framework BLE wrapper's notify() dereferences the freed server via getConnectedCount). Unlike sendLog(), onNowHasData() had no isConnected guard. deinit() now calls resetBleSessionState() to detach the observer and reset session state unconditionally (also forcing the conn handle to NONE so checkConnectionTimeout can't be fooled by the stale handle), nulls fromNumCharacteristic/logRadioCharacteristic like the other freed pointers, and onNowHasData() bails on a null characteristic. Reachable on all ESP32/S3/C6 NimBLE boards via admin disable-bluetooth or a sleep transition while a phone is connected. Found by a follow-up lifecycle audit of the re-enable changes in this PR. --- src/graphics/TFTDisplay.cpp | 19 +-- src/mesh/PacketCache.cpp | 253 --------------------------------- src/mesh/PacketCache.h | 75 ---------- src/nimble/NimbleBluetooth.cpp | 111 +++++++++------ 4 files changed, 79 insertions(+), 379 deletions(-) delete mode 100644 src/mesh/PacketCache.cpp delete mode 100644 src/mesh/PacketCache.h diff --git a/src/graphics/TFTDisplay.cpp b/src/graphics/TFTDisplay.cpp index c24633169..f67e35fce 100644 --- a/src/graphics/TFTDisplay.cpp +++ b/src/graphics/TFTDisplay.cpp @@ -121,7 +121,6 @@ static void rak14014_tpIntHandle(void) #elif defined(HACKADAY_COMMUNICATOR) #include -Arduino_DataBus *bus = nullptr; Arduino_GFX *tft = nullptr; #elif defined(ST72xx_DE) @@ -1601,17 +1600,21 @@ bool TFTDisplay::connect() { concurrency::LockGuard g(spiLock); LOG_INFO("Do TFT init"); + // connect() re-runs on every display wake on variants whose handleSetOn() re-inits + // the UI (see the gates in Screen::handleSetOn); construct the driver exactly once. + if (!tft) { #if defined(RAK14014) || defined(HELTEC_MESH_NODE_T096) || defined(HELTEC_MESH_NODE_T1) - tft = new TFT_eSPI; + tft = new TFT_eSPI; #elif defined(HACKADAY_COMMUNICATOR) - bus = new Arduino_ESP32SPI(TFT_DC, TFT_CS, 38 /* SCK */, 21 /* MOSI */, GFX_NOT_DEFINED /* MISO */, HSPI /* spi_num */); - tft = new Arduino_NV3007(bus, 40, 0 /* rotation */, false /* IPS */, 142 /* width */, 428 /* height */, 12 /* col offset 1 */, - 0 /* row offset 1 */, 14 /* col offset 2 */, 0 /* row offset 2 */, nv3007_279_init_operations, - sizeof(nv3007_279_init_operations)); - + Arduino_DataBus *bus = + new Arduino_ESP32SPI(TFT_DC, TFT_CS, 38 /* SCK */, 21 /* MOSI */, GFX_NOT_DEFINED /* MISO */, HSPI /* spi_num */); + tft = new Arduino_NV3007(bus, 40, 0 /* rotation */, false /* IPS */, 142 /* width */, 428 /* height */, + 12 /* col offset 1 */, 0 /* row offset 1 */, 14 /* col offset 2 */, 0 /* row offset 2 */, + nv3007_279_init_operations, sizeof(nv3007_279_init_operations)); #else - tft = new LGFX; + tft = new LGFX; #endif + } backlightEnable->set(true); LOG_INFO("Power to TFT Backlight"); diff --git a/src/mesh/PacketCache.cpp b/src/mesh/PacketCache.cpp deleted file mode 100644 index 0edf81741..000000000 --- a/src/mesh/PacketCache.cpp +++ /dev/null @@ -1,253 +0,0 @@ -#include "PacketCache.h" -#include "Router.h" - -PacketCache packetCache{}; - -/** - * Allocate a new cache entry and copy the packet header and payload into it - */ -PacketCacheEntry *PacketCache::cache(const meshtastic_MeshPacket *p, bool preserveMetadata) -{ - size_t payload_size = - (p->which_payload_variant == meshtastic_MeshPacket_encrypted_tag) ? p->encrypted.size : p->decoded.payload.size; - PacketCacheEntry *e = (PacketCacheEntry *)malloc(sizeof(PacketCacheEntry) + payload_size + - (preserveMetadata ? sizeof(PacketCacheMetadata) : 0)); - if (!e) { - LOG_ERROR("Unable to allocate memory for packet cache entry"); - return NULL; - } - - *e = {}; - e->header.from = p->from; - e->header.to = p->to; - e->header.id = p->id; - e->header.channel = p->channel; - e->header.next_hop = p->next_hop; - e->header.relay_node = p->relay_node; - e->header.flags = (p->hop_limit & PACKET_FLAGS_HOP_LIMIT_MASK) | (p->want_ack ? PACKET_FLAGS_WANT_ACK_MASK : 0) | - (p->via_mqtt ? PACKET_FLAGS_VIA_MQTT_MASK : 0) | - ((p->hop_start << PACKET_FLAGS_HOP_START_SHIFT) & PACKET_FLAGS_HOP_START_MASK); - - PacketCacheMetadata m{}; - if (preserveMetadata) { - e->has_metadata = true; - m.rx_rssi = (uint8_t)(p->rx_rssi + 200); - m.rx_snr = (uint8_t)((p->rx_snr + 30.0f) / 0.25f); - m.rx_time = p->rx_time; - m.transport_mechanism = p->transport_mechanism; - m.priority = p->priority; - } - if (p->which_payload_variant == meshtastic_MeshPacket_encrypted_tag) { - e->encrypted = true; - e->payload_len = p->encrypted.size; - memcpy(((unsigned char *)e) + sizeof(PacketCacheEntry), p->encrypted.bytes, p->encrypted.size); - } else if (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) { - e->encrypted = false; - if (preserveMetadata) { - m.portnum = p->decoded.portnum; - m.want_response = p->decoded.want_response; - m.emoji = p->decoded.emoji; - m.bitfield = p->decoded.bitfield; - if (p->decoded.reply_id) - m.reply_id = p->decoded.reply_id; - else if (p->decoded.request_id) - m.request_id = p->decoded.request_id; - } - e->payload_len = p->decoded.payload.size; - memcpy(((unsigned char *)e) + sizeof(PacketCacheEntry), p->decoded.payload.bytes, p->decoded.payload.size); - } else { - LOG_ERROR("Unable to cache packet with unknown payload type %d", p->which_payload_variant); - free(e); - return NULL; - } - if (preserveMetadata) - memcpy(((unsigned char *)e) + sizeof(PacketCacheEntry) + e->payload_len, &m, sizeof(m)); - - size += sizeof(PacketCacheEntry) + e->payload_len + (e->has_metadata ? sizeof(PacketCacheMetadata) : 0); - insert(e); - return e; -}; - -/** - * Dump a list of packets into the provided buffer - */ -void PacketCache::dump(void *dest, const PacketCacheEntry **entries, size_t num_entries) -{ - unsigned char *pos = (unsigned char *)dest; - for (size_t i = 0; i < num_entries; i++) { - size_t entry_len = - sizeof(PacketCacheEntry) + entries[i]->payload_len + (entries[i]->has_metadata ? sizeof(PacketCacheMetadata) : 0); - memcpy(pos, entries[i], entry_len); - pos += entry_len; - } -} - -/** - * Calculate the length of buffer needed to dump the specified entries - */ -size_t PacketCache::dumpSize(const PacketCacheEntry **entries, size_t num_entries) -{ - size_t total_size = 0; - for (size_t i = 0; i < num_entries; i++) { - total_size += sizeof(PacketCacheEntry) + entries[i]->payload_len; - if (entries[i]->has_metadata) - total_size += sizeof(PacketCacheMetadata); - } - return total_size; -} - -/** - * Find a packet in the cache - */ -PacketCacheEntry *PacketCache::find(NodeNum from, PacketId id) -{ - uint16_t h = PACKET_HASH(from, id); - PacketCacheEntry *e = buckets[PACKET_CACHE_BUCKET(h)]; - while (e) { - if (e->header.from == from && e->header.id == id) - return e; - e = e->next; - } - return NULL; -} - -/** - * Find a packet in the cache by its hash - */ -PacketCacheEntry *PacketCache::find(PacketHash h) -{ - PacketCacheEntry *e = buckets[PACKET_CACHE_BUCKET(h)]; - while (e) { - if (PACKET_HASH(e->header.from, e->header.id) == h) - return e; - e = e->next; - } - return NULL; -} - -/** - * Load a list of packets from the provided buffer - */ -bool PacketCache::load(void *src, PacketCacheEntry **entries, size_t num_entries) -{ - memset(entries, 0, sizeof(PacketCacheEntry *) * num_entries); - unsigned char *pos = (unsigned char *)src; - for (size_t i = 0; i < num_entries; i++) { - PacketCacheEntry e{}; - memcpy(&e, pos, sizeof(PacketCacheEntry)); - size_t entry_len = sizeof(PacketCacheEntry) + e.payload_len + (e.has_metadata ? sizeof(PacketCacheMetadata) : 0); - entries[i] = (PacketCacheEntry *)malloc(entry_len); - size += entry_len; - if (!entries[i]) { - LOG_ERROR("Unable to allocate memory for packet cache entry"); - for (size_t j = 0; j < i; j++) { - size -= sizeof(PacketCacheEntry) + entries[j]->payload_len + - (entries[j]->has_metadata ? sizeof(PacketCacheMetadata) : 0); - free(entries[j]); - entries[j] = NULL; - } - return false; - } - memcpy(entries[i], pos, entry_len); - pos += entry_len; - } - for (size_t i = 0; i < num_entries; i++) - insert(entries[i]); - return true; -} - -/** - * Copy the cached packet into the provided MeshPacket structure - */ -void PacketCache::rehydrate(const PacketCacheEntry *e, meshtastic_MeshPacket *p) -{ - if (!e || !p) - return; - - *p = {}; - p->from = e->header.from; - p->to = e->header.to; - p->id = e->header.id; - p->channel = e->header.channel; - p->next_hop = e->header.next_hop; - p->relay_node = e->header.relay_node; - p->hop_limit = e->header.flags & PACKET_FLAGS_HOP_LIMIT_MASK; - p->want_ack = !!(e->header.flags & PACKET_FLAGS_WANT_ACK_MASK); - p->via_mqtt = !!(e->header.flags & PACKET_FLAGS_VIA_MQTT_MASK); - p->hop_start = (e->header.flags & PACKET_FLAGS_HOP_START_MASK) >> PACKET_FLAGS_HOP_START_SHIFT; - p->which_payload_variant = e->encrypted ? meshtastic_MeshPacket_encrypted_tag : meshtastic_MeshPacket_decoded_tag; - - unsigned char *payload = ((unsigned char *)e) + sizeof(PacketCacheEntry); - PacketCacheMetadata m{}; - if (e->has_metadata) { - memcpy(&m, (payload + e->payload_len), sizeof(m)); - p->rx_rssi = ((int)m.rx_rssi) - 200; - p->rx_snr = ((float)m.rx_snr * 0.25f) - 30.0f; - p->rx_time = m.rx_time; - p->transport_mechanism = (meshtastic_MeshPacket_TransportMechanism)m.transport_mechanism; - p->priority = (meshtastic_MeshPacket_Priority)m.priority; - } - if (e->encrypted) { - memcpy(p->encrypted.bytes, payload, e->payload_len); - p->encrypted.size = e->payload_len; - } else { - memcpy(p->decoded.payload.bytes, payload, e->payload_len); - p->decoded.payload.size = e->payload_len; - if (e->has_metadata) { - // Decrypted-only metadata - p->decoded.portnum = (meshtastic_PortNum)m.portnum; - p->decoded.want_response = m.want_response; - p->decoded.emoji = m.emoji; - p->decoded.bitfield = m.bitfield; - if (m.reply_id) - p->decoded.reply_id = m.reply_id; - else if (m.request_id) - p->decoded.request_id = m.request_id; - } - } -} - -/** - * Release a cache entry - */ -void PacketCache::release(PacketCacheEntry *e) -{ - if (!e) - return; - remove(e); - size -= sizeof(PacketCacheEntry) + e->payload_len + (e->has_metadata ? sizeof(PacketCacheMetadata) : 0); - free(e); -} - -/** - * Insert a new entry into the hash table - */ -void PacketCache::insert(PacketCacheEntry *e) -{ - assert(e); - PacketHash h = PACKET_HASH(e->header.from, e->header.id); - PacketCacheEntry **target = &buckets[PACKET_CACHE_BUCKET(h)]; - e->next = *target; - *target = e; - num_entries++; -} - -/** - * Remove an entry from the hash table - */ -void PacketCache::remove(PacketCacheEntry *e) -{ - assert(e); - PacketHash h = PACKET_HASH(e->header.from, e->header.id); - PacketCacheEntry **target = &buckets[PACKET_CACHE_BUCKET(h)]; - while (*target) { - if (*target == e) { - *target = e->next; - e->next = NULL; - num_entries--; - break; - } else { - target = &(*target)->next; - } - } -} \ No newline at end of file diff --git a/src/mesh/PacketCache.h b/src/mesh/PacketCache.h deleted file mode 100644 index 85660922b..000000000 --- a/src/mesh/PacketCache.h +++ /dev/null @@ -1,75 +0,0 @@ -#pragma once -#include "RadioInterface.h" - -#define PACKET_HASH(a, b) ((((a ^ b) >> 16) ^ (a ^ b)) & 0xFFFF) // 16 bit fold of packet (from, id) tuple -typedef uint16_t PacketHash; - -#define PACKET_CACHE_BUCKETS 64 // Number of hash table buckets -#define PACKET_CACHE_BUCKET(h) (((h >> 12) ^ (h >> 6) ^ h) & 0x3F) // Fold hash down to 6-bit bucket index - -typedef struct PacketCacheEntry { - PacketCacheEntry *next; - PacketHeader header; - uint16_t payload_len = 0; - union { - uint16_t bitfield; - struct { - uint8_t encrypted : 1; // Payload is encrypted - uint8_t has_metadata : 1; // Payload includes PacketCacheMetadata - uint8_t : 6; // Reserved for future use - uint8_t : 8; // Reserved for future use - }; - }; -} PacketCacheEntry; - -typedef struct PacketCacheMetadata { - PacketCacheMetadata() : _bitfield(0), reply_id(0), _bitfield2(0) {} - union { - uint32_t _bitfield; - struct { - uint16_t portnum : 9; // meshtastic_MeshPacket::decoded::portnum - uint16_t want_response : 1; // meshtastic_MeshPacket::decoded::want_response - uint16_t emoji : 1; // meshtastic_MeshPacket::decoded::emoji - uint16_t bitfield : 5; // meshtastic_MeshPacket::decoded::bitfield (truncated) - uint8_t rx_rssi : 8; // meshtastic_MeshPacket::rx_rssi (map via actual RSSI + 200) - uint8_t rx_snr : 8; // meshtastic_MeshPacket::rx_snr (map via (p->rx_snr + 30.0f) / 0.25f) - }; - }; - union { - uint32_t reply_id; // meshtastic_MeshPacket::decoded.reply_id - uint32_t request_id; // meshtastic_MeshPacket::decoded.request_id - }; - uint32_t rx_time = 0; // meshtastic_MeshPacket::rx_time - uint8_t transport_mechanism = 0; // meshtastic_MeshPacket::transport_mechanism - struct { - uint8_t _bitfield2; - union { - uint8_t priority : 7; // meshtastic_MeshPacket::priority - uint8_t reserved : 1; // Reserved for future use - }; - }; -} PacketCacheMetadata; - -class PacketCache -{ - public: - PacketCacheEntry *cache(const meshtastic_MeshPacket *p, bool preserveMetadata); - static void dump(void *dest, const PacketCacheEntry **entries, size_t num_entries); - size_t dumpSize(const PacketCacheEntry **entries, size_t num_entries); - PacketCacheEntry *find(NodeNum from, PacketId id); - PacketCacheEntry *find(PacketHash h); - bool load(void *src, PacketCacheEntry **entries, size_t num_entries); - size_t getNumEntries() { return num_entries; } - size_t getSize() { return size; } - void rehydrate(const PacketCacheEntry *e, meshtastic_MeshPacket *p); - void release(PacketCacheEntry *e); - - private: - PacketCacheEntry *buckets[PACKET_CACHE_BUCKETS]{}; - size_t num_entries = 0; - size_t size = 0; - void insert(PacketCacheEntry *e); - void remove(PacketCacheEntry *e); -}; - -extern PacketCache packetCache; \ No newline at end of file diff --git a/src/nimble/NimbleBluetooth.cpp b/src/nimble/NimbleBluetooth.cpp index 4b52eae83..647c1bef8 100644 --- a/src/nimble/NimbleBluetooth.cpp +++ b/src/nimble/NimbleBluetooth.cpp @@ -409,6 +409,8 @@ class BluetoothPhoneAPI : public PhoneAPI, public concurrency::OSThread uint8_t val[4]; put_le32(val, fromRadioNum); + if (!fromNumCharacteristic) // BLE may have been torn down; never notify a freed characteristic + return; fromNumCharacteristic->setValue(val, sizeof(val)); fromNumCharacteristic->notify(); } @@ -694,6 +696,35 @@ class NimbleBluetoothSecurityCallback : public BLESecurityCallbacks } }; +// Reset per-session PhoneAPI and transport state. Runs from onDisconnect, and again from +// setupService() on BLE re-enable because deinit()'s bounded disconnect wait can expire +// before the disconnect event delivers this cleanup (leaving stale queues/state behind). +static void resetBleSessionState() +{ + if (bluetoothPhoneAPI) { + bluetoothPhoneAPI->close(); + + { // scope for fromPhoneMutex mutex + std::lock_guard guard(bluetoothPhoneAPI->fromPhoneMutex); + bluetoothPhoneAPI->fromPhoneQueueSize = 0; + } + + bluetoothPhoneAPI->onReadCallbackIsWaitingForData = false; + { // scope for toPhoneMutex mutex + std::lock_guard guard(bluetoothPhoneAPI->toPhoneMutex); + bluetoothPhoneAPI->toPhoneQueueSize = 0; + } + + bluetoothPhoneAPI->readCount = 0; + bluetoothPhoneAPI->notifyCount = 0; + bluetoothPhoneAPI->writeCount = 0; + } + + memset(lastToRadio, 0, sizeof(lastToRadio)); + + nimbleBluetoothConnHandle = BLE_HS_CONN_HANDLE_NONE; +} + class NimbleBluetoothServerCallback : public BLEServerCallbacks { public: @@ -736,28 +767,7 @@ class NimbleBluetoothServerCallback : public BLEServerCallbacks bluetoothStatus->updateStatus(&newStatus); clearPairingDisplay(); - if (bluetoothPhoneAPI) { - bluetoothPhoneAPI->close(); - - { // scope for fromPhoneMutex mutex - std::lock_guard guard(bluetoothPhoneAPI->fromPhoneMutex); - bluetoothPhoneAPI->fromPhoneQueueSize = 0; - } - - bluetoothPhoneAPI->onReadCallbackIsWaitingForData = false; - { // scope for toPhoneMutex mutex - std::lock_guard guard(bluetoothPhoneAPI->toPhoneMutex); - bluetoothPhoneAPI->toPhoneQueueSize = 0; - } - - bluetoothPhoneAPI->readCount = 0; - bluetoothPhoneAPI->notifyCount = 0; - bluetoothPhoneAPI->writeCount = 0; - } - - memset(lastToRadio, 0, sizeof(lastToRadio)); - - nimbleBluetoothConnHandle = BLE_HS_CONN_HANDLE_NONE; + resetBleSessionState(); // Defer the advertising restart to runOnce (see pendingStartAdvertising): calling // startAdvertising() here would crash if this disconnect was a host reset. @@ -769,9 +779,6 @@ class NimbleBluetoothServerCallback : public BLEServerCallbacks } }; -static NimbleBluetoothToRadioCallback *toRadioCallbacks; -static NimbleBluetoothFromRadioCallback *fromRadioCallbacks; - void NimbleBluetooth::startAdvertising() { BLEAdvertising *pAdvertising = BLEDevice::getAdvertising(); @@ -840,7 +847,14 @@ void NimbleBluetooth::deinit() BLEDevice::deinit(true); bleServer = nullptr; // deleted by deinit(); clear the dangling copy BatteryCharacteristic = nullptr; // freed by deinit; clear so updateBatteryLevel() won't touch it + fromNumCharacteristic = nullptr; // freed by deinit; a late onNowHasData() must not notify freed memory + logRadioCharacteristic = nullptr; lastBatteryLevel = -1; + + // The bounded disconnect wait above can expire before onDisconnect runs, leaving the PhoneAPI + // observer attached with a live state machine; a later mesh packet would then drive onNowHasData() + // into the now-freed characteristics. Detach the observer and reset session state unconditionally. + resetBleSessionState(); #endif } @@ -858,7 +872,7 @@ int NimbleBluetooth::getRssi() { #if defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32C6) uint16_t conn_handle = nimbleBluetoothConnHandle.load(); - if (conn_handle != BLE_HS_CONN_HANDLE_NONE) { + if (conn_handle == BLE_HS_CONN_HANDLE_NONE) { return 0; // No active BLE connection } @@ -903,33 +917,36 @@ void NimbleBluetooth::setup() LOG_WARN("Unable to request MTU %u, rc=%d", kPreferredBleMtu, mtuResult); } - BLESecurity *pSecurity = new BLESecurity(); - pSecurity->setInitEncryptionKey(ESP_BLE_ENC_KEY_MASK | ESP_BLE_ID_KEY_MASK); - pSecurity->setRespEncryptionKey(ESP_BLE_ENC_KEY_MASK | ESP_BLE_ID_KEY_MASK); + // BLESecurity only forwards to static NimBLEDevice setters; a stack instance suffices. + BLESecurity security; + security.setInitEncryptionKey(ESP_BLE_ENC_KEY_MASK | ESP_BLE_ID_KEY_MASK); + security.setRespEncryptionKey(ESP_BLE_ENC_KEY_MASK | ESP_BLE_ID_KEY_MASK); if (config.bluetooth.mode != meshtastic_Config_BluetoothConfig_PairingMode_NO_PIN) { // Set IO capability to DisplayOnly for MITM authentication - pSecurity->setCapability(ESP_IO_CAP_OUT); + security.setCapability(ESP_IO_CAP_OUT); // Set the passkey if (config.bluetooth.mode == meshtastic_Config_BluetoothConfig_PairingMode_RANDOM_PIN) { LOG_INFO("Use random passkey"); - pSecurity->setPassKey(false); // generate a random passkey + security.setPassKey(false); // generate a random passkey } else { LOG_INFO("Use fixed passkey"); - pSecurity->setPassKey(true, config.bluetooth.fixed_pin); + security.setPassKey(true, config.bluetooth.fixed_pin); } // Enable authorization requirements: // - bonding: true (for persistent storage of the keys) // - MITM: true (enables Man-In-The-Middle protection for password prompts) // - secure connection: true (enables secure connection for encryption) - pSecurity->setAuthenticationMode(true, true, true); + security.setAuthenticationMode(true, true, true); } else { // No IO capability for no PIN mode - pSecurity->setCapability(ESP_IO_CAP_NONE); + security.setCapability(ESP_IO_CAP_NONE); // No PIN mode: no MITM protection - pSecurity->setAuthenticationMode(true, false, false); + security.setAuthenticationMode(true, false, false); } - // Set the security callbacks - BLEDevice::setSecurityCallbacks(new NimbleBluetoothSecurityCallback()); + // Statics: setup() re-runs on BLE re-enable, and the library never frees these + // caller-owned callback objects, so register the same instances every cycle. + static NimbleBluetoothSecurityCallback securityCallbacks; + BLEDevice::setSecurityCallbacks(&securityCallbacks); bleServer = BLEDevice::createServer(); // BLEDevice::createServer calls ble_svc_gap_init, which resets the device @@ -939,7 +956,8 @@ void NimbleBluetooth::setup() LOG_ERROR("ble_svc_gap_device_name_set: rc=%d %s", nameRc, BLEUtils::returnCodeToString(nameRc)); } - bleServer->setCallbacks(new NimbleBluetoothServerCallback(this)); + static NimbleBluetoothServerCallback serverCallbacks(this); // safe: NimbleBluetooth is a never-deleted singleton + bleServer->setCallbacks(&serverCallbacks); setupService(); startAdvertising(); } @@ -972,13 +990,20 @@ void NimbleBluetooth::setupService() LOGRADIO_UUID, BLECharacteristic::PROPERTY_NOTIFY | BLECharacteristic::PROPERTY_READ | BLECharacteristic::PROPERTY_READ_AUTHEN | BLECharacteristic::PROPERTY_READ_ENC); } - bluetoothPhoneAPI = new BluetoothPhoneAPI(); + // setupService() re-runs on BLE re-enable; a fresh BluetoothPhoneAPI here would leak + // the old one as a still-scheduled zombie OSThread, so reuse it and reset its state + // (a skipped onDisconnect during deinit() can leave the previous session's behind). + if (!bluetoothPhoneAPI) + bluetoothPhoneAPI = new BluetoothPhoneAPI(); + else + resetBleSessionState(); - toRadioCallbacks = new NimbleBluetoothToRadioCallback(); - ToRadioCharacteristic->setCallbacks(toRadioCallbacks); + // The characteristics are new each cycle, so setCallbacks() must re-run every time. + static NimbleBluetoothToRadioCallback toRadioCallbacks; + ToRadioCharacteristic->setCallbacks(&toRadioCallbacks); - fromRadioCallbacks = new NimbleBluetoothFromRadioCallback(); - FromRadioCharacteristic->setCallbacks(fromRadioCallbacks); + static NimbleBluetoothFromRadioCallback fromRadioCallbacks; + FromRadioCharacteristic->setCallbacks(&fromRadioCallbacks); bleService->start();