Repair ESP32 3.3.11 BLE service cleanup in build-local SDK sources

.github/workflows/run-unit-tests.yml

@@ -1004,6 +1004,7 @@ jobs:
           python3 -B test/test_nimble_companion.py
           python3 -B test/test_esp32_ble_power.py -v
           python3 -B test/test_esp32_native_ble_stop.py -v
+          python3 -B test/test_esp32_ble_lifecycle_fix.py -v
           python3 -B test/test_bluetooth_pairing_ui.py
           python3 -B test/test_companion_pairing_display.py
           python3 -B test/test_companion_identity_startup.py

platformio.ini

@@ -96,6 +96,7 @@ extra_scripts =
   pre:scripts/esp32_ota_heap_context.py
   pre:scripts/esp32_usb_session_fix.py
   pre:scripts/esp32_file_buffer_fix.py
+  pre:scripts/esp32_ble_lifecycle_fix.py
   pre:scripts/esp32_wifi_scan_fix.py
   pre:scripts/esp32_async_web_fix.py
   merge-bin.py

scripts/esp32_ble_lifecycle_fix.py

@@ -0,0 +1,106 @@
+"""Restore pinned Arduino 3.3.11 BLE service ownership for reversible shutdown.
+
+BLEDevice owns the new wrapper's server, but its implicit destructor leaves
+raw service pointers behind. BLEService's destructor is private; delete it
+from BLEDevice's existing friend context. NUS characteristics/descriptors stay
+application-owned, as on the older wrappers. Never modify the shared SDK.
+"""
+from pathlib import Path
+import hashlib
+import re
+
+
+FRAMEWORKS_UNCHANGED = {(2, 0, 17), (3, 1, 3)}
+FRAMEWORK_PATCHED = (3, 3, 11)
+PINNED_SHA256 = "ec3735051f3263fc0101986fe33602a2593c294c6ccbd9f7b1b416c15529c3c9"
+PATCHED_SHA256 = "bf8fecfd6367eb0a2c23dd610336fefa17687d1f7bbc38e9d8f8642835763e87"
+SOURCE_DIRECTORY = ("libraries", "BLE", "src")
+ORIGINAL = '''  if (m_pServer != nullptr) {
+    delete m_pServer;
+    m_pServer = nullptr;
+  }
+'''
+REPAIRED = '''  if (m_pServer != nullptr) {
+    // MeshCore: the server map owns raw service pointers. BLEDevice is a
+    // friend of BLEServer/BLEService, whose destructor is private here.
+    // The application has already stopped host callback dispatch.
+    BLEService* service = m_pServer->m_serviceMap.getFirst();
+    while (service != nullptr) {
+      delete service;
+      service = m_pServer->m_serviceMap.getNext();
+    }
+    delete m_pServer;
+    m_pServer = nullptr;
+  }
+'''
+
+SERVICE_ORIGINAL = 'BLEService::~BLEService() {\n#if defined(CONFIG_NIMBLE_ENABLED)\n  if (m_pSvcDef != nullptr) {\n    if (m_pSvcDef->characteristics != nullptr) {\n      for (int i = 0; m_pSvcDef->characteristics[i].uuid != NULL; ++i) {\n        if (m_pSvcDef->characteristics[i].descriptors) {\n          delete (m_pSvcDef->characteristics[i].descriptors);\n        }\n      }\n      delete (m_pSvcDef->characteristics);\n    }\n\n    delete (m_pSvcDef);\n  }\n#endif\n}'
+# Each definition is allocated with new[] by BLEService::start().
+SERVICE_REPAIRED = SERVICE_ORIGINAL.replace("delete (", "delete[] (")
+SOURCE_PINS = {
+    "BLEDevice.cpp": (PINNED_SHA256, PATCHED_SHA256, ORIGINAL, REPAIRED),
+    "BLEService.cpp": (
+        "2b71699e1962cdc37a54ace429ebdd408b0e40b379d621d427add5bb5a93b252",
+        "8c4d8917ffb768a9eab468b1be06be209b4b8fc065b125c821682b6579a30267", SERVICE_ORIGINAL, SERVICE_REPAIRED),
+}
+
+
+def patched_ble_source(source, framework_version, name="BLEDevice.cpp"):
+    if framework_version in FRAMEWORKS_UNCHANGED:
+        return source
+    if framework_version != FRAMEWORK_PATCHED:
+        raise RuntimeError("ESP32 BLE lifecycle fix: unreviewed framework version")
+    source = source.replace("\r\n", "\n")
+    try:
+        original_digest, patched_digest, original, repaired = SOURCE_PINS[name]
+    except KeyError as error:
+        raise RuntimeError("ESP32 BLE lifecycle fix: unreviewed source") from error
+    digest = hashlib.sha256(source.encode("utf-8")).hexdigest()
+    if digest == patched_digest:
+        return source
+    if digest != original_digest or source.count(original) != 1:
+        raise RuntimeError("ESP32 BLE lifecycle fix: changed pinned source; review SDK update")
+    patched = source.replace(original, repaired, 1)
+    if hashlib.sha256(patched.encode("utf-8")).hexdigest() != patched_digest:
+        raise RuntimeError("ESP32 BLE lifecycle fix: unexpected transform result")
+    return patched
+
+
+def framework_version(source):
+    header = source.parents[3] / "cores" / "esp32" / "esp_arduino_version.h"
+    try:
+        text = header.read_text(encoding="utf-8")
+    except OSError as error:
+        raise RuntimeError("ESP32 BLE lifecycle fix: missing framework version header") from error
+    entries = re.findall(r"^#define ESP_ARDUINO_VERSION_(MAJOR|MINOR|PATCH)\s+(\d+)\s*$",
+                         text, re.MULTILINE)
+    values = dict(entries)
+    if len(entries) != 3 or set(values) != {"MAJOR", "MINOR", "PATCH"}:
+        raise RuntimeError("ESP32 BLE lifecycle fix: malformed framework version")
+    return tuple(int(values[key]) for key in ("MAJOR", "MINOR", "PATCH"))
+
+
+def replace_ble_source(build_env, node):
+    source = Path(node.srcnode().get_abspath())
+    if source.parts[-4:-1] != SOURCE_DIRECTORY or source.name not in SOURCE_PINS:
+        return node
+    version = framework_version(source)
+    original = source.read_text(encoding="utf-8")
+    patched = patched_ble_source(original, version, source.name)
+    if patched == original:
+        return node
+    destination = Path(build_env.subst("$BUILD_DIR")) / "patched-esp32-ble" / source.name
+    destination.parent.mkdir(parents=True, exist_ok=True)
+    if not destination.exists() or destination.read_text(encoding="utf-8") != patched:
+        destination.write_text(patched, encoding="utf-8")
+    build_env.AppendUnique(CPPPATH=[str(source.parent)])
+    return build_env.File(str(destination))
+
+
+def install(build_env):
+    build_env.AddBuildMiddleware(replace_ble_source, "*libraries*BLE*src*BLE*.cpp")
+
+
+if "Import" in globals():
+    Import("env")
+    install(env)

src/helpers/esp32/SerialBLEInterface.cpp

@@ -43,14 +43,18 @@ static bool stopBluetoothController() {
 }
 
 #if !defined(MESH_USE_NIMBLE_ARDUINO)
-// The pinned 2.0.17/3.1.3 wrappers leave their static server/advertiser alive
-// on deinit(false). The newer wrapper exposing getServer() deletes them.
-// Keep that ownership distinction independent of the processor model.
-template <typename Device>
-static auto bluetoothLibraryOwnsServer(int)
+// The newer wrapper owns its private service objects (the pinned 3.3.11
+// ownership repair is build-local in esp32_ble_lifecycle_fix.py). Select this
+// at compile time: a runtime if still compiles an inaccessible destructor.
+// Both wrappers leave our known NUS characteristics/descriptors app-owned.
+template <typename Device, typename Service>
+static auto cleanupBluetoothService(Service*, int)
     -> decltype(Device::getServer(), bool()) { return true; }
-template <typename Device>
-static bool bluetoothLibraryOwnsServer(...) { return false; }
+template <typename Device, typename Service>
+static bool cleanupBluetoothService(Service* service, ...) {
+  delete service;
+  return false;
+}
 #endif
 
 void SerialBLEInterface::forgetGattPointers() {
@@ -90,15 +94,11 @@ bool SerialBLEInterface::stopBluetoothStack() {
 #if defined(CONFIG_BLUEDROID_ENABLED)
   if (!stopBluetoothController()) return false;
 #endif
-  const bool owns_server = bluetoothLibraryOwnsServer<BLEDevice>(0);
   BLEDevice::deinit(false);
-  // Neither pinned Arduino wrapper owns our characteristics/service. The
-  // legacy server is retained until createServer() replaces its static
-  // pointer; deleting it sooner would leave a dangling callback target.
   delete pTxDescriptor;
   delete pRxCharacteristic;
   delete pTxCharacteristic;
-  delete pService;
+  const bool owns_server = cleanupBluetoothService<BLEDevice>(pService, 0);
   pTxDescriptor = NULL;
   pRxCharacteristic = pTxCharacteristic = NULL;
   pService = NULL;

test/fixtures/esp32_ble_power/mocks/BLEDevice.h

@@ -127,11 +127,22 @@ public:
 class BLEService {
 public:
   BLEService() { ++fake_bt::services; }
+#ifdef TEST_BLE_OWNED_SERVER
+private:
+  friend class BLEDevice;
   ~BLEService() { --fake_bt::services; }
+public:
+#else
+  ~BLEService() { --fake_bt::services; }
+#endif
+  std::vector<BLECharacteristic*> children;
   unsigned made=0;
   BLECharacteristic* createCharacteristic(const char*,uint32_t) {
     const char* phase = made++ ? "rx" : "tx";
-    return fake_bt::failure==phase ? nullptr : new BLECharacteristic;
+    if (fake_bt::failure==phase) return nullptr;
+    auto* child = new BLECharacteristic;
+    children.push_back(child);
+    return child;
   }
   void start() {}
 };
@@ -145,7 +156,10 @@ public:
     assert(fake_bt::host!=ESP_BLUEDROID_STATUS_ENABLED || fake_bt::server!=this);
     --fake_bt::servers;
   }
-  BLEService* createService(const char*) { return fake_bt::failure=="service" ? nullptr : new BLEService; }
+  BLEService* service = nullptr;
+  BLEService* createService(const char*) {
+    return service = fake_bt::failure=="service" ? nullptr : new BLEService;
+  }
   BLEAdvertising* getAdvertising() { if(!fake_advertising) fake_advertising=new BLEAdvertising; return fake_advertising; }
   void setCallbacks(BLEServerCallbacks* cb) { callbacks=cb; }
   unsigned getConnectedCount() const { return connected; }
@@ -175,6 +189,7 @@ public:
     if(!fake_bt::initialized) return;
 #ifdef TEST_BLE_OWNED_SERVER
     assert(fake_bt::host!=ESP_BLUEDROID_STATUS_ENABLED);
+    if (fake_bt::server) delete fake_bt::server->service;
     delete fake_bt::server; fake_bt::server=nullptr;
     delete fake_advertising; fake_advertising=nullptr;
 #endif

test/test_esp32_ble_lifecycle_fix.py

@@ -0,0 +1,213 @@
+#!/usr/bin/env python3
+"""Verify pinned SDK ownership, private destruction, and build-local patching."""
+from pathlib import Path
+import hashlib
+import importlib.util
+import os
+import subprocess
+import tempfile
+import unittest
+from unittest.mock import patch
+
+ROOT = Path(__file__).resolve().parents[1]
+SPEC = importlib.util.spec_from_file_location("esp32_ble_lifecycle_fix", ROOT / "scripts/esp32_ble_lifecycle_fix.py")
+FIX = importlib.util.module_from_spec(SPEC)
+SPEC.loader.exec_module(FIX)
+
+HARNESS = r'''
+#include <cassert>
+#include <cstdio>
+#include <vector>
+static bool host_running = false;
+static unsigned objects = 0;
+struct Lifetime {
+  Lifetime() { ++objects; }
+  ~Lifetime() { assert(!host_running); --objects; }
+};
+struct DescriptorDefinition { int permissions = 0; };
+struct CharacteristicDefinition { const void* uuid = nullptr; DescriptorDefinition* descriptors = nullptr; };
+struct ServiceDefinition { CharacteristicDefinition* characteristics = nullptr; };
+class BLEDevice;
+class BLEService : Lifetime {
+  friend class BLEDevice;
+  ServiceDefinition* m_pSvcDef = nullptr;
+  ~BLEService(); // Matches the actual SDK's private destructor.
+public:
+  BLEService() {
+#if defined(CONFIG_NIMBLE_ENABLED)
+    m_pSvcDef = new ServiceDefinition[2]{};
+    auto* chars = new CharacteristicDefinition[3]{};
+    for (int i = 0; i < 2; ++i) {
+      chars[i].uuid = this;
+      chars[i].descriptors = new DescriptorDefinition[2]{};
+    }
+    m_pSvcDef[0].characteristics = chars;
+#endif
+  }
+};
+@SERVICE_DESTRUCTOR@
+struct ServiceMap {
+  std::vector<BLEService*> pointers;
+  unsigned next = 0;
+  BLEService* getFirst() { next = 0; return getNext(); }
+  BLEService* getNext() { return next < pointers.size() ? pointers[next++] : nullptr; }
+};
+class BLEServer : Lifetime {
+  friend class BLEDevice;
+  ServiceMap m_serviceMap; // Raw-pointer map, as in the SDK.
+public:
+  void createService() { m_serviceMap.pointers.push_back(new BLEService); }
+};
+class BLEDevice {
+public:
+  inline static BLEServer* m_pServer = nullptr;
+  static void deinit() {
+    assert(!host_running);
+@SERVER_DELETE@
+  }
+};
+int main() {
+  BLEDevice::deinit(); // Init failed before any server existed.
+  for (unsigned cycle = 0; cycle < 100; ++cycle) {
+    BLEDevice::m_pServer = new BLEServer;
+    if (cycle % 3 != 0) {
+      BLEDevice::m_pServer->createService();
+      BLEDevice::m_pServer->createService();
+    }
+    // The SDK service destructor owns its private definition arrays, but
+    // NUS TX/RX/2902 objects remain app-owned and must not be deleted twice.
+    auto* tx = new Lifetime;
+    auto* rx = new Lifetime;
+    auto* cccd = new Lifetime;
+    host_running = true;
+    host_running = false;
+    BLEDevice::deinit();
+    assert(BLEDevice::m_pServer == nullptr && objects == 3);
+    delete tx; delete rx; delete cccd;
+    assert(objects == 0);
+  }
+  puts("private service ownership cycles passed");
+}
+'''
+
+
+class FakeNode:
+    def __init__(self, path):
+        self.path = path
+
+    def srcnode(self):
+        return self
+
+    def get_abspath(self):
+        return str(self.path)
+
+
+class FakeEnvironment:
+    def __init__(self, build):
+        self.build = build
+        self.paths = []
+
+    def subst(self, expression):
+        assert expression == "$BUILD_DIR"
+        return str(self.build)
+
+    def AppendUnique(self, **kwargs):
+        self.paths.extend(kwargs["CPPPATH"])
+
+    def File(self, path):
+        return Path(path)
+
+
+class BleLifecycleFixTests(unittest.TestCase):
+    def test_private_service_destruction_and_native_arrays_under_sanitizers(self):
+        for native in (False, True):
+            with self.subTest(native=native), tempfile.TemporaryDirectory(prefix="ble-private-service-") as directory:
+                root = Path(directory)
+                cpp, exe = root / "test.cpp", root / "test"
+                text = HARNESS.replace("@SERVER_DELETE@", FIX.REPAIRED).replace(
+                    "@SERVICE_DESTRUCTOR@", FIX.SERVICE_REPAIRED)
+                cpp.write_text(text)
+                command = [os.environ.get("CXX", "c++"), "-std=c++17", "-Wall", "-Wextra", "-Werror",
+                           "-fsanitize=address,undefined", *( ["-DCONFIG_NIMBLE_ENABLED=1"] if native else []),
+                           str(cpp), "-o", str(exe)]
+                built = subprocess.run(command, text=True, capture_output=True, timeout=30)
+                self.assertEqual(built.returncode, 0, built.stderr)
+                run = subprocess.run([str(exe)], text=True, capture_output=True, timeout=10)
+                self.assertEqual(run.returncode, 0, run.stderr)
+                if native:
+                    # The original SDK destructor used scalar delete for
+                    # arrays. Ensure ASan catches that precise mismatch.
+                    cpp.write_text(text.replace(FIX.SERVICE_REPAIRED, FIX.SERVICE_ORIGINAL))
+                    built = subprocess.run(command, text=True, capture_output=True, timeout=30)
+                    self.assertEqual(built.returncode, 0, built.stderr)
+                    run = subprocess.run([str(exe)], text=True, capture_output=True, timeout=10)
+                    self.assertNotEqual(run.returncode, 0)
+                    self.assertIn("alloc-dealloc-mismatch", run.stderr)
+
+    def test_hash_guards_idempotence_and_old_sdk_unchanged(self):
+        for name, (_, _, original, repaired) in FIX.SOURCE_PINS.items():
+            with self.subTest(name=name):
+                source = "// reviewed source fixture\n" + original
+                expected = source.replace(original, repaired)
+                pins = {name: (hashlib.sha256(source.encode()).hexdigest(),
+                               hashlib.sha256(expected.encode()).hexdigest(), original, repaired)}
+                with patch.object(FIX, "SOURCE_PINS", pins):
+                    self.assertEqual(FIX.patched_ble_source(source, (3, 3, 11), name), expected)
+                    self.assertEqual(FIX.patched_ble_source(expected, (3, 3, 11), name), expected)
+                    self.assertEqual(FIX.patched_ble_source(source.replace("\n", "\r\n"), (3, 3, 11), name), expected)
+                    for version in ((2, 0, 17), (3, 1, 3)):
+                        self.assertEqual(FIX.patched_ble_source(source, version, name), source)
+                    with self.assertRaisesRegex(RuntimeError, "changed pinned source"):
+                        FIX.patched_ble_source(source + "// changed\n", (3, 3, 11), name)
+                    with self.assertRaisesRegex(RuntimeError, "unreviewed framework"):
+                        FIX.patched_ble_source(source, (3, 3, 12), name)
+                    with self.assertRaisesRegex(RuntimeError, "unreviewed source"):
+                        FIX.patched_ble_source(source, (3, 3, 11), "BLEOther.cpp")
+
+    def test_middleware_copies_only_pinned_sources_without_touching_shared_sdk(self):
+        with tempfile.TemporaryDirectory(prefix="ble-sdk-middleware-") as directory:
+            root = Path(directory)
+            sdk = root / "framework"
+            source = sdk / "libraries/BLE/src/BLEDevice.cpp"
+            version = sdk / "cores/esp32/esp_arduino_version.h"
+            source.parent.mkdir(parents=True)
+            version.parent.mkdir(parents=True)
+            version.write_text("#define ESP_ARDUINO_VERSION_MAJOR 3\n#define ESP_ARDUINO_VERSION_MINOR 3\n#define ESP_ARDUINO_VERSION_PATCH 11\n")
+            original = "// fixture\n" + FIX.ORIGINAL
+            expected = original.replace(FIX.ORIGINAL, FIX.REPAIRED)
+            source.write_text(original)
+            pins = {"BLEDevice.cpp": (hashlib.sha256(original.encode()).hexdigest(),
+                    hashlib.sha256(expected.encode()).hexdigest(), FIX.ORIGINAL, FIX.REPAIRED)}
+            node, env = FakeNode(source), FakeEnvironment(root / "build")
+            with patch.object(FIX, "SOURCE_PINS", pins):
+                destination = FIX.replace_ble_source(env, node)
+                self.assertEqual(source.read_text(), original)
+                self.assertEqual(destination.read_text(), expected)
+                self.assertEqual(env.paths, [str(source.parent)])
+                timestamp = destination.stat().st_mtime_ns
+                self.assertEqual(FIX.replace_ble_source(env, node), destination)
+                self.assertEqual(destination.stat().st_mtime_ns, timestamp)
+                version.write_text(version.read_text().replace("PATCH 11", "PATCH 12"))
+                with self.assertRaisesRegex(RuntimeError, "unreviewed framework"):
+                    FIX.replace_ble_source(env, node)
+
+    def test_installed_exact_sdk_pins_when_available(self):
+        packages = Path.home() / ".platformio/packages"
+        candidates = list(packages.glob("framework-arduinoespressif32*"))
+        sdk = None
+        for candidate in candidates:
+            source = candidate / "libraries/BLE/src/BLEDevice.cpp"
+            if source.exists() and FIX.framework_version(source) == (3, 3, 11):
+                sdk = source.parent
+                break
+        if sdk is None:
+            self.skipTest("Pinned Arduino3.3.11 SDK is not installed")
+        for name in FIX.SOURCE_PINS:
+            source = (sdk / name).read_text()
+            patched = FIX.patched_ble_source(source, (3, 3, 11), name)
+            self.assertNotEqual(source, patched)
+            self.assertEqual(FIX.patched_ble_source(patched, (3, 3, 11), name), patched)
+
+
+if __name__ == "__main__":
+    unittest.main()

test/test_esp32_native_ble_stop.py

@@ -29,6 +29,7 @@ HARNESS = r'''
 #include <cassert>
 #include <cstdio>
 #include <initializer_list>
+#include <vector>
 using esp_err_t = int;
 constexpr int ESP_OK = 0;
 enum esp_bt_controller_status_t { ESP_BT_CONTROLLER_STATUS_IDLE,
@@ -71,6 +72,7 @@ using BLEService = GattObject;
 using BLECharacteristic = GattObject;
 using BLE2902 = GattObject;
 static BLEServer* library_server = nullptr;
+static std::vector<GattObject*> library_children;
 struct BLEDevice {
   static bool getInitialized() { return initialized; }
   static BLEServer* getServer() { return library_server; }
@@ -80,6 +82,8 @@ struct BLEDevice {
     ++wrapper_deinits;
     // The current native wrapper deletes the server before stopping its port.
     // The adapter must have stopped callback dispatch before entering here.
+    for (auto* child : library_children) delete child;
+    library_children.clear();
     delete library_server; library_server = nullptr;
     (void)nimble_port_stop();
     initialized = host_task_created = false;
@@ -110,6 +114,7 @@ public:
     pTxCharacteristic = new BLECharacteristic;
     pRxCharacteristic = new BLECharacteristic;
     pTxDescriptor = new BLE2902;
+    library_children = {pService};
   }
 };
 @STACK_STOP@
@@ -166,7 +171,7 @@ int main() {
 class NativeNimbleStopTests(unittest.TestCase):
     def test_production_stop_handles_absent_and_started_host_tasks(self):
         source = (ROOT / "src/helpers/esp32/SerialBLEInterface.cpp").read_text()
-        ownership_start = source.index("template <typename Device>")
+        ownership_start = source.index("template <typename Device,")
         ownership = source[ownership_start:source.index("\n#endif", ownership_start)]
         harness = HARNESS.replace("@OWNERSHIP@", ownership).replace(
             "@CONTROLLER_STOP@", function(source, "static bool stopBluetoothController()"))