From 0e701106403340fbdbcbffaa377580588db520cc Mon Sep 17 00:00:00 2001 From: mikecarper Date: Mon, 19 Oct 2026 00:06:03 -0700 Subject: [PATCH] Fix Bluefruit role dispatch after disconnect frees connection --- .github/workflows/run-unit-tests.yml | 1 + scripts/nrf52_ble_cccd_fix.py | 44 ++++ test/fixtures/nrf52_ble_disconnect/README.md | 12 ++ .../fixtures/nrf52_ble_disconnect/bluefruit.h | 138 ++++++++++++ .../bluefruit_dispatch.cpp | 197 ++++++++++++++++++ test/fixtures/nrf52_ble_disconnect/test.cpp | 151 ++++++++++++++ test/test_nrf52_ble_cccd_fix.py | 3 +- test/test_nrf52_ble_disconnect.py | 84 ++++++++ 8 files changed, 629 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/nrf52_ble_disconnect/README.md create mode 100644 test/fixtures/nrf52_ble_disconnect/bluefruit.h create mode 100644 test/fixtures/nrf52_ble_disconnect/bluefruit_dispatch.cpp create mode 100644 test/fixtures/nrf52_ble_disconnect/test.cpp create mode 100644 test/test_nrf52_ble_disconnect.py diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index a9da465e2..ae0f4bd55 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.github/workflows/run-unit-tests.yml @@ -779,6 +779,7 @@ jobs: pio pkg install --global --tool 'platformio/toolchain-gccarmnoneeabi@1.140201.0' python3 -B test/test_nrf52_svc_compat.py -v python3 -B test/test_nrf52_ble_cccd_fix.py -v + python3 -B test/test_nrf52_ble_disconnect.py -v pip install unicorn==2.1.4 python3 -B test/test_nrf52_ble_dfu_fix.py -v diff --git a/scripts/nrf52_ble_cccd_fix.py b/scripts/nrf52_ble_cccd_fix.py index c1c4cd7c2..f3c219c12 100644 --- a/scripts/nrf52_ble_cccd_fix.py +++ b/scripts/nrf52_ble_cccd_fix.py @@ -10,6 +10,8 @@ Patch a private build copy; never edit PlatformIO's shared framework cache. With the pinned GCC 14/LTO toolchain, the optimized Bluefruit.begin() fails on the RAK4631 repeater: S140 rejects a valid single-peripheral role request with NRF_ERROR_RESOURCES. The unoptimized method starts DFU successfully. +Cache the dispatch role before a disconnect deletes its BLEConnection. The +remaining peripheral/central routing must not dereference that freed object. """ from pathlib import Path @@ -49,6 +51,32 @@ FIXED_BEGIN = ( 'AdafruitBluefruit::begin(uint8_t prph_count, uint8_t central_count)' ) +DISCONNECT_PATCHES = ( + ( + " BLEConnection* conn = this->Connection(conn_hdl);\n\n" + ' LOG_LV2("BLE",', + " BLEConnection* conn = this->Connection(conn_hdl);\n" + " // Keep role routing valid after disconnect deletes the connection.\n" + " uint8_t conn_role = conn ? conn->getRole() : BLE_GAP_ROLE_INVALID;\n\n" + ' LOG_LV2("BLE",', + ), + ( + " conn = _connection[conn_hdl];\n\n" + " // Invoke connect callback\n", + " conn = _connection[conn_hdl];\n" + " conn_role = conn->getRole();\n\n" + " // Invoke connect callback\n", + ), + ( + " if ( (conn == NULL) || (conn->getRole() == BLE_GAP_ROLE_PERIPH) )\n", + " if ( (conn_role == BLE_GAP_ROLE_INVALID) || (conn_role == BLE_GAP_ROLE_PERIPH) )\n", + ), + ( + " if ( (conn == NULL) || (conn->getRole() == BLE_GAP_ROLE_CENTRAL) )\n", + " if ( (conn_role == BLE_GAP_ROLE_INVALID) || (conn_role == BLE_GAP_ROLE_CENTRAL) )\n", + ), +) + def patched_source(source): source = source.replace("\r\n", "\n") @@ -76,6 +104,7 @@ def patched_connection_source(source): def patched_bluefruit_source(source): source = source.replace("\r\n", "\n") + source = patched_disconnect_source(source) if FIXED_BEGIN in source and OLD_BEGIN not in source: return source if source.count(OLD_BEGIN) != 1: @@ -85,6 +114,21 @@ def patched_bluefruit_source(source): return source.replace(OLD_BEGIN, FIXED_BEGIN) +def patched_disconnect_source(source): + source = source.replace("\r\n", "\n") + counts = [(source.count(old), source.count(fixed)) + for old, fixed in DISCONNECT_PATCHES] + if all(old == 0 and fixed == 1 for old, fixed in counts): + return source + if not all(old == 1 and fixed == 0 for old, fixed in counts): + raise RuntimeError( + "nRF52 BLE disconnect fix: unrecognized framework dispatcher; review before building" + ) + for old, fixed in DISCONNECT_PATCHES: + source = source.replace(old, fixed) + return source + + def patched_trace_source(source): """Bracket real BLE dispatch in diagnostic builds, including stuck handlers.""" include = '#include "bluefruit.h"\n' diff --git a/test/fixtures/nrf52_ble_disconnect/README.md b/test/fixtures/nrf52_ble_disconnect/README.md new file mode 100644 index 000000000..5f7df5218 --- /dev/null +++ b/test/fixtures/nrf52_ble_disconnect/README.md @@ -0,0 +1,12 @@ +The dispatcher in `bluefruit_dispatch.cpp` is copied without changes from +`libraries/Bluefruit52Lib/src/bluefruit.cpp` at the framework revision pinned +by `platformio.ini`: + +https://github.com/meshcore-dev/Adafruit_nRF52_Arduino/blob/d541301665b40959682252911e57b11df3ee651a/libraries/Bluefruit52Lib/src/bluefruit.cpp + +Its original BSD license is retained. `bluefruit.h` supplies host-side test +doubles; `test.cpp` exercises the complete dispatcher after the production +middleware transforms it. The unpatched dispatcher must fail under ASan with +a heap-use-after-free on disconnect. The fixed dispatcher must retain role +filtering, callback deferral, handle reuse, deletion order, and generic-event +routing for peripheral and central connections. diff --git a/test/fixtures/nrf52_ble_disconnect/bluefruit.h b/test/fixtures/nrf52_ble_disconnect/bluefruit.h new file mode 100644 index 000000000..1e0261a03 --- /dev/null +++ b/test/fixtures/nrf52_ble_disconnect/bluefruit.h @@ -0,0 +1,138 @@ +#pragma once + +#include +#include +#include +#include +#include +#include + +#define LOG_LV1(...) do {} while (0) +#define LOG_LV2(...) do {} while (0) + +constexpr uint8_t BLE_GAP_ROLE_INVALID = 0; +constexpr uint8_t BLE_GAP_ROLE_PERIPH = 1; +constexpr uint8_t BLE_GAP_ROLE_CENTRAL = 2; +constexpr uint16_t BLE_CONN_HANDLE_INVALID = 0xffff; +enum { + BLE_GAP_EVT_CONNECTED = 1, + BLE_GAP_EVT_DISCONNECTED, + BLE_GAP_EVT_RSSI_CHANGED, + BLE_EVT_USER_MEM_REQUEST, + BLE_EVT_USER_MEM_RELEASE, + BLE_TEST_GENERIC_EVENT, +}; + +struct ble_gap_addr_t { + uint8_t addr[6] = {}, addr_type = 0, addr_id_peer = 0; +}; +struct ble_gap_conn_params_t { + uint16_t max_conn_interval = 24, slave_latency = 4, conn_sup_timeout = 200; +}; +struct ble_gap_evt_connected_t { + ble_gap_addr_t peer_addr; + ble_gap_conn_params_t conn_params; + uint8_t role = BLE_GAP_ROLE_INVALID; +}; +struct ble_gap_evt_disconnected_t { uint8_t reason = 0; }; +struct ble_gap_evt_rssi_changed_t { int8_t rssi = 0; }; +struct ble_evt_t { + struct { uint16_t evt_id = 0; } header; + struct { + // The real SDK exposes the same handle storage through both views. + union { + struct { uint16_t conn_handle; } common_evt; + struct { + uint16_t conn_handle; + struct { + ble_gap_evt_connected_t connected; + ble_gap_evt_disconnected_t disconnected; + ble_gap_evt_rssi_changed_t rssi_changed; + } params; + } gap_evt; + }; + } evt = {}; +}; + +inline std::vector steps; +inline std::vector> deferred; +inline unsigned deleted_connections = 0; +inline unsigned live_connections = 0; + +template +bool ada_callback(void*, size_t, Function function, Args... args) { + steps.push_back("defer"); + deferred.emplace_back([=] { function(args...); }); + return true; +} + +inline void sd_ble_user_mem_reply(uint16_t, void*) { + steps.push_back("memory_reply"); +} + +struct BLEConnection { + uint8_t _role; + bool _connected = true; + BLEConnection(uint16_t, const ble_gap_evt_connected_t* connected, + uint8_t, uint8_t) : _role(connected->role) { + ++live_connections; + steps.push_back("create"); + } + ~BLEConnection() { + --live_connections; + ++deleted_connections; + steps.push_back("delete"); + } + // Read object storage so ASan checks the original dispatcher's freed access. + __attribute__((noinline)) uint8_t getRole() { return _role; } + void _eventHandler(ble_evt_t* event) { + steps.push_back("connection"); + if (event->header.evt_id == BLE_GAP_EVT_DISCONNECTED) _connected = false; + } +}; + +using ConnectCallback = void (*)(uint16_t); +using DisconnectCallback = void (*)(uint16_t, uint8_t); +struct RoleHandler { + const char* name; + ConnectCallback _connect_cb = nullptr; + DisconnectCallback _disconnect_cb = nullptr; + void _eventHandler(ble_evt_t*) { steps.push_back(name); } +}; +struct Handler { + const char* name; + void _eventHandler(ble_evt_t*) { steps.push_back(name); } +}; +struct DiscoveryHandler : Handler { + bool enabled = true; + bool begun() const { return enabled; } +}; + +class AdafruitBluefruit { +public: + BLEConnection* _connection[4] = {}; + uint16_t _conn_hdl = BLE_CONN_HANDLE_INVALID; + struct { + struct { uint8_t hvn_qsize = 3, wrcmd_qsize = 1; } prph, central; + } _sd_cfg; + RoleHandler Periph{"peripheral"}, Central{"central"}; + Handler Security{"security"}, Advertising{"advertising"}, Scanner{"scanner"}, + Gatt{"gatt"}; + DiscoveryHandler Discovery{{"discovery"}}; + void (*_event_cb)(ble_evt_t*) = nullptr; + void (*_rssi_cb)(uint16_t, int8_t) = nullptr; + + BLEConnection* Connection(uint16_t handle) { + return handle < 4 ? _connection[handle] : nullptr; + } + unsigned connected() const { + unsigned count = 0; + for (auto* conn : _connection) count += conn && conn->_connected; + return count; + } + void _stopConnLed() { steps.push_back("led_stop"); } + void _setConnLed(bool enabled) { + steps.push_back(enabled ? "led_on" : "led_off"); + } + void _ble_handler(ble_evt_t*); +}; diff --git a/test/fixtures/nrf52_ble_disconnect/bluefruit_dispatch.cpp b/test/fixtures/nrf52_ble_disconnect/bluefruit_dispatch.cpp new file mode 100644 index 000000000..0e5e6c3a4 --- /dev/null +++ b/test/fixtures/nrf52_ble_disconnect/bluefruit_dispatch.cpp @@ -0,0 +1,197 @@ +/**************************************************************************/ +/*! + @file bluefruit.cpp + @author hathach (tinyusb.org) + + @section LICENSE + + Software License Agreement (BSD License) + + Copyright (c) 2018, Adafruit Industries (adafruit.com) + All rights reserved. + + Redistribution and use in source and binary forms, with or without + modification, are permitted provided that the following conditions are met: + 1. Redistributions of source code must retain the above copyright + notice, this list of conditions and the following disclaimer. + 2. Redistributions in binary form must reproduce the above copyright + notice, this list of conditions and the following disclaimer in the + documentation and/or other materials provided with the distribution. + 3. Neither the name of the copyright holders nor the + names of its contributors may be used to endorse or promote products + derived from this software without specific prior written permission. + + THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS ''AS IS'' AND ANY + EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER BE LIABLE FOR ANY + DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +*/ +/**************************************************************************/ + +#include "bluefruit.h" + +void AdafruitBluefruit::_ble_handler(ble_evt_t* evt) +{ + // conn handle has fixed offset for all events + uint16_t const conn_hdl = evt->evt.common_evt.conn_handle; + BLEConnection* conn = this->Connection(conn_hdl); + + LOG_LV2("BLE", "%s : Conn Handle = %d", dbg_ble_event_str(evt->header.evt_id), conn_hdl); + + // GAP handler + if ( conn ) + { + conn->_eventHandler(evt); + Security._eventHandler(evt); + } + + switch(evt->header.evt_id) + { + case BLE_GAP_EVT_CONNECTED: + { + // Turn on Conn LED + _stopConnLed(); + _setConnLed(true); + + ble_gap_evt_connected_t const * para = &evt->evt.gap_evt.params.connected; + ble_gap_addr_t const* peer_addr = ¶->peer_addr; + + (void) peer_addr; + + LOG_LV2("GAP", "MAC = %02X:%02X:%02X:%02X:%02X:%02X, Type = %d, Resolved = %d", + peer_addr->addr[5], peer_addr->addr[4], peer_addr->addr[3], peer_addr->addr[2], peer_addr->addr[1], peer_addr->addr[0], + peer_addr->addr_type, peer_addr->addr_id_peer); + LOG_LV2("GAP", "Conn Interval = %.2f ms, Latency = %d, Supervisor Timeout = %d ms", + para->conn_params.max_conn_interval*1.25f, para->conn_params.slave_latency, 10*para->conn_params.conn_sup_timeout); + + if ( _connection[conn_hdl] ) + { + LOG_LV1("GAP", "Connection is already in used, something wrong !!"); + delete _connection[conn_hdl]; + _connection[conn_hdl] = NULL; + } + + // Transmission buffer pool + uint8_t const hvn_qsize = (para->role == BLE_GAP_ROLE_PERIPH) ? _sd_cfg.prph.hvn_qsize : _sd_cfg.central.hvn_qsize; + uint8_t const wrcmd_qsize = (para->role == BLE_GAP_ROLE_PERIPH) ? _sd_cfg.prph.wrcmd_qsize : _sd_cfg.central.wrcmd_qsize; + + _connection[conn_hdl] = new BLEConnection(conn_hdl, para, hvn_qsize, wrcmd_qsize); + conn = _connection[conn_hdl]; + + // Invoke connect callback + if ( conn->getRole() == BLE_GAP_ROLE_PERIPH ) + { + if ( Periph._connect_cb ) ada_callback(NULL, 0, Periph._connect_cb, conn_hdl); + }else + { + if ( Central._connect_cb ) ada_callback(NULL, 0, Central._connect_cb, conn_hdl); + } + } + break; + + case BLE_GAP_EVT_DISCONNECTED: + { + ble_gap_evt_disconnected_t const* para = &evt->evt.gap_evt.params.disconnected; + + LOG_LV2("GAP", "Disconnect Reason: %s", dbg_hci_str(evt->evt.gap_evt.params.disconnected.reason)); + + // Turn off Conn LED If not connected at all + if ( !this->connected() ) _setConnLed(false); + + // Invoke disconnect callback + if ( conn->getRole() == BLE_GAP_ROLE_PERIPH ) + { + if ( Periph._disconnect_cb ) ada_callback(NULL, 0, Periph._disconnect_cb, conn_hdl, para->reason); + }else + { + if ( Central._disconnect_cb ) ada_callback(NULL, 0, Central._disconnect_cb, conn_hdl, para->reason); + } + + delete _connection[conn_hdl]; + _connection[conn_hdl] = NULL; + } + break; + + case BLE_GAP_EVT_RSSI_CHANGED: + { + ble_gap_evt_rssi_changed_t const * rssi_changed = &evt->evt.gap_evt.params.rssi_changed; + if ( _rssi_cb ) + { + ada_callback(NULL, 0, _rssi_cb, conn_hdl, rssi_changed->rssi); + } + } + break; + + case BLE_EVT_USER_MEM_REQUEST: + // We will handle Long Write sequence (RW Authorize PREP_WRITE_REQ) + sd_ble_user_mem_reply(conn_hdl, NULL); + break; + + case BLE_EVT_USER_MEM_RELEASE: + // nothing to do + break; + + default: break; + } + + Advertising._eventHandler(evt); + Scanner._eventHandler(evt); + + /*------------- BLE Peripheral Events -------------*/ + /* Only handle Peripheral events with matched connection handle + * or a few special one + * - Connected event + * - Advertising timeout (could be connected and advertising at the same time) + */ + if ( conn_hdl == _conn_hdl || + evt->header.evt_id == BLE_GAP_EVT_CONNECTED) + { + switch ( evt->header.evt_id ) + { + case BLE_GAP_EVT_CONNECTED: + { + ble_gap_evt_connected_t* para = &evt->evt.gap_evt.params.connected; + + if (para->role == BLE_GAP_ROLE_PERIPH) + { + _conn_hdl = evt->evt.gap_evt.conn_handle; + } + } + break; + + case BLE_GAP_EVT_DISCONNECTED: + _conn_hdl = BLE_CONN_HANDLE_INVALID; + break; + + default: break; + } + } + + // Periph event handler (also handle generic non-connection event) + if ( (conn == NULL) || (conn->getRole() == BLE_GAP_ROLE_PERIPH) ) + { + Periph._eventHandler(evt); + } + + // Central Event Handler (also handle generic non-connection event) + if ( (conn == NULL) || (conn->getRole() == BLE_GAP_ROLE_CENTRAL) ) + { + Central._eventHandler(evt); + } + + // Discovery Event Handler + if ( Discovery.begun() ) Discovery._eventHandler(evt); + + // GATTs characteristics event handler + Gatt._eventHandler(evt); + + // User callback if set + if (_event_cb) _event_cb(evt); +} + diff --git a/test/fixtures/nrf52_ble_disconnect/test.cpp b/test/fixtures/nrf52_ble_disconnect/test.cpp new file mode 100644 index 000000000..b4d82370e --- /dev/null +++ b/test/fixtures/nrf52_ble_disconnect/test.cpp @@ -0,0 +1,151 @@ +#include "bluefruit.h" + +#include + +static unsigned peripheral_connects = 0, central_connects = 0; +static unsigned peripheral_disconnects = 0, central_disconnects = 0; +static uint16_t callback_handle = BLE_CONN_HANDLE_INVALID; +static uint8_t callback_reason = 0; +static AdafruitBluefruit* active = nullptr; + +static void peripheralConnect(uint16_t handle) { + ++peripheral_connects; + callback_handle = handle; +} +static void centralConnect(uint16_t handle) { + ++central_connects; + callback_handle = handle; +} +static void peripheralDisconnect(uint16_t handle, uint8_t reason) { + assert(active->Connection(handle) == nullptr); + ++peripheral_disconnects; + callback_handle = handle; + callback_reason = reason; +} +static void centralDisconnect(uint16_t handle, uint8_t reason) { + assert(active->Connection(handle) == nullptr); + ++central_disconnects; + callback_handle = handle; + callback_reason = reason; +} +static void userEvent(ble_evt_t* event) { + if (event->header.evt_id == BLE_GAP_EVT_DISCONNECTED) { + assert(active->Connection(event->evt.common_evt.conn_handle) == nullptr); + } + steps.push_back("user"); +} +static ble_evt_t event(uint16_t id, uint16_t handle) { + ble_evt_t result = {}; + result.header.evt_id = id; + result.evt.common_evt.conn_handle = handle; + return result; +} +static void runDeferred() { + for (const auto& callback : deferred) callback(); + deferred.clear(); +} +static void checkSteps(std::initializer_list expected) { + assert(steps.size() == expected.size()); + assert(std::equal(steps.begin(), steps.end(), expected.begin())); + steps.clear(); +} + +static void connect(AdafruitBluefruit& bluefruit, uint8_t role, uint16_t handle) { + auto connected = event(BLE_GAP_EVT_CONNECTED, handle); + connected.evt.gap_evt.params.connected.role = role; + bluefruit._ble_handler(&connected); + assert(live_connections == 1); + assert(bluefruit.Connection(handle)->getRole() == role); + assert(bluefruit._conn_hdl == (role == BLE_GAP_ROLE_PERIPH + ? handle : BLE_CONN_HANDLE_INVALID)); + assert(deferred.size() == 1); + assert(steps[0] == "led_stop"); + assert(steps[1] == "led_on"); + assert(steps[2] == "create"); + assert(steps[3] == "defer"); + assert(steps[4] == "advertising"); + assert(steps[5] == "scanner"); + assert(steps[6] == (role == BLE_GAP_ROLE_PERIPH ? "peripheral" : "central")); + assert(steps[7] == "discovery" && steps[8] == "gatt" && steps[9] == "user"); + assert(steps.size() == 10); + steps.clear(); + runDeferred(); + assert(callback_handle == handle); +} + +static void disconnect(AdafruitBluefruit& bluefruit, uint8_t role, + uint16_t handle, uint8_t reason) { + const unsigned old_deleted = deleted_connections; + const unsigned old_peripheral = peripheral_disconnects; + const unsigned old_central = central_disconnects; + auto disconnected = event(BLE_GAP_EVT_DISCONNECTED, handle); + disconnected.evt.gap_evt.params.disconnected.reason = reason; + bluefruit._ble_handler(&disconnected); + assert(live_connections == 0 && deleted_connections == old_deleted + 1); + assert(bluefruit.Connection(handle) == nullptr); + assert(bluefruit._conn_hdl == BLE_CONN_HANDLE_INVALID); + assert(deferred.size() == 1); + assert(peripheral_disconnects == old_peripheral); + assert(central_disconnects == old_central); + if (role == BLE_GAP_ROLE_PERIPH) { + checkSteps({"connection", "security", "led_off", "defer", "delete", + "advertising", "scanner", "peripheral", "discovery", "gatt", "user"}); + } else { + checkSteps({"connection", "security", "led_off", "defer", "delete", + "advertising", "scanner", "central", "discovery", "gatt", "user"}); + } + runDeferred(); + assert(callback_handle == handle && callback_reason == reason); + assert(peripheral_disconnects == old_peripheral + (role == BLE_GAP_ROLE_PERIPH)); + assert(central_disconnects == old_central + (role == BLE_GAP_ROLE_CENTRAL)); +} + +int main() { + AdafruitBluefruit bluefruit; + active = &bluefruit; + bluefruit.Periph._connect_cb = peripheralConnect; + bluefruit.Central._connect_cb = centralConnect; + bluefruit.Periph._disconnect_cb = peripheralDisconnect; + bluefruit.Central._disconnect_cb = centralDisconnect; + bluefruit._event_cb = userEvent; + + // Exercise peripheral and central roles, handle reuse, and both local and + // supervision-timeout disconnect reasons without relying on real hardware. + for (unsigned i = 0; i < 20; ++i) { + const uint8_t role = (i & 1) ? BLE_GAP_ROLE_CENTRAL : BLE_GAP_ROLE_PERIPH; + const uint16_t handle = i % 4; + connect(bluefruit, role, handle); + auto ordinary = event(BLE_TEST_GENERIC_EVENT, handle); + bluefruit._ble_handler(&ordinary); + if (role == BLE_GAP_ROLE_PERIPH) { + checkSteps({"connection", "security", "advertising", "scanner", "peripheral", + "discovery", "gatt", "user"}); + } else { + checkSteps({"connection", "security", "advertising", "scanner", "central", + "discovery", "gatt", "user"}); + } + disconnect(bluefruit, role, handle, (i & 2) ? 0x08 : 0x16); + } + assert(peripheral_connects == 10 && central_connects == 10); + assert(peripheral_disconnects == 10 && central_disconnects == 10); + + // Unknown-handle generic events still reach both role handlers. + auto generic = event(BLE_EVT_USER_MEM_REQUEST, BLE_CONN_HANDLE_INVALID); + bluefruit._ble_handler(&generic); + checkSteps({"memory_reply", "advertising", "scanner", "peripheral", "central", + "discovery", "gatt", "user"}); + + // CONNECTED replaces an already occupied handle in the pinned framework. + // The routing snapshot must be refreshed for the newly allocated role. + connect(bluefruit, BLE_GAP_ROLE_PERIPH, 2); + const unsigned old_deleted = deleted_connections; + auto replacement = event(BLE_GAP_EVT_CONNECTED, 2); + replacement.evt.gap_evt.params.connected.role = BLE_GAP_ROLE_CENTRAL; + bluefruit._ble_handler(&replacement); + assert(deleted_connections == old_deleted + 1 && live_connections == 1); + checkSteps({"connection", "security", "led_stop", "led_on", "delete", "create", + "defer", "advertising", "scanner", "central", "discovery", "gatt", "user"}); + runDeferred(); + disconnect(bluefruit, BLE_GAP_ROLE_CENTRAL, 2, 0x13); + assert(deleted_connections == 22 && live_connections == 0); +} diff --git a/test/test_nrf52_ble_cccd_fix.py b/test/test_nrf52_ble_cccd_fix.py index 32fd4e0f6..e9e891a39 100644 --- a/test/test_nrf52_ble_cccd_fix.py +++ b/test/test_nrf52_ble_cccd_fix.py @@ -55,8 +55,9 @@ class Nrf52BleCccdFixTest(unittest.TestCase): self.assertNotIn("mesh_nrf52_service_cccd_save", app) def test_bluefruit_begin_uses_private_unoptimized_copy(self): + dispatcher = (ROOT / "test/fixtures/nrf52_ble_disconnect/bluefruit_dispatch.cpp").read_text() patched = module.patched_bluefruit_source( - "before\n" + module.OLD_BEGIN + "\nafter\n" + "before\n" + module.OLD_BEGIN + "\nafter\n" + dispatcher ) self.assertEqual(patched.count(module.FIXED_BEGIN), 1) self.assertEqual(module.patched_bluefruit_source(patched), patched) diff --git a/test/test_nrf52_ble_disconnect.py b/test/test_nrf52_ble_disconnect.py new file mode 100644 index 000000000..e0d63450f --- /dev/null +++ b/test/test_nrf52_ble_disconnect.py @@ -0,0 +1,84 @@ +"""Exercise the production Bluefruit dispatcher fix under ASan and UBSan.""" + +from importlib.util import module_from_spec, spec_from_file_location +import os +from pathlib import Path +import subprocess +import tempfile +import unittest + + +ROOT = Path(__file__).resolve().parents[1] +FIXTURE = ROOT / "test/fixtures/nrf52_ble_disconnect" +spec = spec_from_file_location("nrf52_ble_patcher", ROOT / "scripts/nrf52_ble_cccd_fix.py") +patcher = module_from_spec(spec) +spec.loader.exec_module(patcher) + + +class Nrf52BleDisconnectTest(unittest.TestCase): + def setUp(self): + self.source = (FIXTURE / "bluefruit_dispatch.cpp").read_text() + + def test_patch_is_idempotent_and_preserves_newlines(self): + fixed = patcher.patched_disconnect_source(self.source) + self.assertNotEqual(fixed, self.source) + self.assertEqual(patcher.patched_disconnect_source(fixed), fixed) + self.assertEqual(patcher.patched_disconnect_source( + self.source.replace("\n", "\r\n")), fixed) + + def test_changed_duplicate_or_partial_dispatchers_fail_closed(self): + for old, fixed in patcher.DISCONNECT_PATCHES: + for malformed in ( + self.source.replace(old, "changed framework source"), + self.source + old, + self.source.replace(old, fixed)): + with self.subTest(marker=old, source=malformed[:30]): + with self.assertRaises(RuntimeError): + patcher.patched_disconnect_source(malformed) + fixed_source = patcher.patched_disconnect_source(self.source) + for _, fixed in patcher.DISCONNECT_PATCHES: + with self.subTest(fixed_marker=fixed): + with self.assertRaises(RuntimeError): + patcher.patched_disconnect_source(fixed_source + fixed) + + def test_private_bluefruit_pipeline_applies_startup_and_disconnect_fixes(self): + source = patcher.OLD_BEGIN + " { return true; }\n" + self.source + fixed = patcher.patched_bluefruit_source(source) + self.assertIn(patcher.FIXED_BEGIN, fixed) + self.assertEqual(patcher.patched_bluefruit_source(fixed), fixed) + for old, new in patcher.DISCONNECT_PATCHES: + self.assertNotIn(old, fixed) + self.assertEqual(fixed.count(new), 1) + + def run_dispatcher(self, source): + with tempfile.TemporaryDirectory(prefix="nrf52-ble-disconnect-") as directory: + work = Path(directory) + dispatcher = work / "dispatch.cpp" + dispatcher.write_text(source) + binary = work / "dispatch" + subprocess.run([ + "c++", "-std=c++17", "-O1", "-g", "-Wall", "-Wextra", "-Werror", + "-fsanitize=address,undefined", "-fno-omit-frame-pointer", + "-fno-pie", "-no-pie", "-I" + str(FIXTURE), + str(dispatcher), str(FIXTURE / "test.cpp"), "-o", str(binary), + ], check=True, capture_output=True, text=True) + environment = os.environ.copy() + environment["ASAN_OPTIONS"] = "detect_leaks=1:halt_on_error=1" + environment["UBSAN_OPTIONS"] = "halt_on_error=1:print_stacktrace=1" + return subprocess.run([str(binary)], capture_output=True, + text=True, env=environment) + + def test_original_dispatcher_reproduces_heap_use_after_free(self): + result = self.run_dispatcher(self.source) + self.assertNotEqual(result.returncode, 0) + self.assertIn("AddressSanitizer: heap-use-after-free", result.stderr) + self.assertIn("BLEConnection::getRole()", result.stderr) + + def test_fixed_dispatcher_preserves_both_roles_and_reconnects(self): + fixed = patcher.patched_disconnect_source(self.source) + result = self.run_dispatcher(fixed) + self.assertEqual(result.returncode, 0, result.stderr) + + +if __name__ == "__main__": + unittest.main()