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.
This commit is contained in:
Ben Meadors
GitHub
@@ -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<std::mutex> guard(bluetoothPhoneAPI->fromPhoneMutex);
|
||||
bluetoothPhoneAPI->fromPhoneQueueSize = 0;
|
||||
}
|
||||
|
||||
bluetoothPhoneAPI->onReadCallbackIsWaitingForData = false;
|
||||
{ // scope for toPhoneMutex mutex
|
||||
std::lock_guard<std::mutex> 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<std::mutex> guard(bluetoothPhoneAPI->fromPhoneMutex);
|
||||
bluetoothPhoneAPI->fromPhoneQueueSize = 0;
|
||||
}
|
||||
|
||||
bluetoothPhoneAPI->onReadCallbackIsWaitingForData = false;
|
||||
{ // scope for toPhoneMutex mutex
|
||||
std::lock_guard<std::mutex> 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();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user