diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index fe7a4c77f..c995f948f 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.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 diff --git a/platformio.ini b/platformio.ini index 80f7d7448..f8a111195 100644 --- a/platformio.ini +++ b/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 diff --git a/scripts/esp32_ble_lifecycle_fix.py b/scripts/esp32_ble_lifecycle_fix.py new file mode 100644 index 000000000..85337ac10 --- /dev/null +++ b/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) diff --git a/src/helpers/esp32/SerialBLEInterface.cpp b/src/helpers/esp32/SerialBLEInterface.cpp index 3e3061bda..33affb175 100644 --- a/src/helpers/esp32/SerialBLEInterface.cpp +++ b/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 -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 +static auto cleanupBluetoothService(Service*, int) -> decltype(Device::getServer(), bool()) { return true; } -template -static bool bluetoothLibraryOwnsServer(...) { return false; } +template +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(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(pService, 0); pTxDescriptor = NULL; pRxCharacteristic = pTxCharacteristic = NULL; pService = NULL; diff --git a/test/fixtures/esp32_ble_power/mocks/BLEDevice.h b/test/fixtures/esp32_ble_power/mocks/BLEDevice.h index ef2d78090..b420b0583 100644 --- a/test/fixtures/esp32_ble_power/mocks/BLEDevice.h +++ b/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 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 diff --git a/test/test_esp32_ble_lifecycle_fix.py b/test/test_esp32_ble_lifecycle_fix.py new file mode 100644 index 000000000..a7e9f8577 --- /dev/null +++ b/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 +#include +#include +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 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() diff --git a/test/test_esp32_native_ble_stop.py b/test/test_esp32_native_ble_stop.py index d78aff136..2dd4ee84b 100644 --- a/test/test_esp32_native_ble_stop.py +++ b/test/test_esp32_native_ble_stop.py @@ -29,6 +29,7 @@ HARNESS = r''' #include #include #include +#include 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 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 ") + ownership_start = source.index("template