From 9d6e48caa46c3f85fdd94333397cd4c21e6fd2a4 Mon Sep 17 00:00:00 2001 From: Ryan Powell Date: Tue, 17 Mar 2026 13:47:57 -0600 Subject: [PATCH] [Bugfix] NimBLEDevice::createServer can crash if stack not initialized. * [Bugfix] NimBLEDevice::createServer can crash if stack not initialized. This removes the gatts/gap reset calls from NimBLEDevice::createServer so that it can be safely used before initializing the stack. This also deprecates NimBLEService::start the the services will now be added/created only when the server is started. --- .../NimBLE_extended_server/main/main.cpp | 3 -- .../NimBLE_multi_advertiser/main/main.cpp | 3 -- examples/NimBLE_Server/main/main.cpp | 4 --- src/NimBLEAdvertising.cpp | 5 +-- src/NimBLEDevice.cpp | 3 -- src/NimBLEServer.cpp | 33 +++++++++++++------ src/NimBLEServer.h | 6 ++-- src/NimBLEService.cpp | 18 ++-------- src/NimBLEService.h | 19 ++++++++--- src/NimBLEStream.cpp | 9 ++--- 10 files changed, 48 insertions(+), 55 deletions(-) diff --git a/examples/Bluetooth_5/NimBLE_extended_server/main/main.cpp b/examples/Bluetooth_5/NimBLE_extended_server/main/main.cpp index a19b49e..719b44d 100644 --- a/examples/Bluetooth_5/NimBLE_extended_server/main/main.cpp +++ b/examples/Bluetooth_5/NimBLE_extended_server/main/main.cpp @@ -80,9 +80,6 @@ extern "C" void app_main(void) { pCharacteristic->setValue("Hello World"); - /** Start the service */ - pService->start(); - /** * Create an extended advertisement with the instance ID 0 and set the PHY's. * Multiple instances can be added as long as the instance ID is incremented. diff --git a/examples/Bluetooth_5/NimBLE_multi_advertiser/main/main.cpp b/examples/Bluetooth_5/NimBLE_multi_advertiser/main/main.cpp index 3322e54..3752570 100644 --- a/examples/Bluetooth_5/NimBLE_multi_advertiser/main/main.cpp +++ b/examples/Bluetooth_5/NimBLE_multi_advertiser/main/main.cpp @@ -98,9 +98,6 @@ extern "C" void app_main(void) { pCharacteristic->setValue("Hello World"); - /** Start the service */ - pService->start(); - /** Create our multi advertising instances */ /** extended scannable instance advertising on coded and 1m PHY's. */ diff --git a/examples/NimBLE_Server/main/main.cpp b/examples/NimBLE_Server/main/main.cpp index 7f8d265..d2b7653 100644 --- a/examples/NimBLE_Server/main/main.cpp +++ b/examples/NimBLE_Server/main/main.cpp @@ -184,10 +184,6 @@ extern "C" void app_main(void) { pC01Ddsc->setValue("Send it back!"); pC01Ddsc->setCallbacks(&dscCallbacks); - /** Start the services when finished creating all Characteristics and Descriptors */ - pDeadService->start(); - pBaadService->start(); - /** Create an advertising instance and add the services to the advertised data */ NimBLEAdvertising* pAdvertising = NimBLEDevice::getAdvertising(); pAdvertising->setName("NimBLE-Server"); diff --git a/src/NimBLEAdvertising.cpp b/src/NimBLEAdvertising.cpp index a93e9ee..53ec223 100644 --- a/src/NimBLEAdvertising.cpp +++ b/src/NimBLEAdvertising.cpp @@ -197,8 +197,9 @@ bool NimBLEAdvertising::start(uint32_t duration, const NimBLEAddress* dirAddr) { # if MYNEWT_VAL(BLE_ROLE_PERIPHERAL) NimBLEServer* pServer = NimBLEDevice::getServer(); - if (pServer != nullptr) { - pServer->start(); // make sure the GATT server is ready before advertising + if (pServer != nullptr && !pServer->start()) { // make sure the GATT server is ready before advertising + NIMBLE_LOGE(LOG_TAG, "Failed to start GATT server"); + return false; } # endif diff --git a/src/NimBLEDevice.cpp b/src/NimBLEDevice.cpp index cca5290..bbd10f0 100644 --- a/src/NimBLEDevice.cpp +++ b/src/NimBLEDevice.cpp @@ -122,9 +122,6 @@ extern "C" int ble_vhci_disc_duplicate_mode_enable(int mode); NimBLEServer* NimBLEDevice::createServer() { if (NimBLEDevice::m_pServer == nullptr) { NimBLEDevice::m_pServer = new NimBLEServer(); - ble_gatts_reset(); - ble_svc_gap_init(); - ble_svc_gatt_init(); } return m_pServer; diff --git a/src/NimBLEServer.cpp b/src/NimBLEServer.cpp index 60f07d3..bcbec9a 100644 --- a/src/NimBLEServer.cpp +++ b/src/NimBLEServer.cpp @@ -269,14 +269,18 @@ void NimBLEServer::gattRegisterCallback(ble_gatt_register_ctxt* ctxt, void* arg) * @details Required to be called after setup of all services and characteristics / descriptors * for the NimBLE host to register them. */ -void NimBLEServer::start() { +bool NimBLEServer::start() { if (m_svcChanged && !getConnectedCount()) { NIMBLE_LOGD(LOG_TAG, "Services have changed since last start, resetting GATT server"); - resetGATT(); + m_gattsStarted = false; } if (m_gattsStarted) { - return; // already started + return true; // already started + } + + if (!resetGATT()) { + return false; } ble_hs_cfg.gatts_register_cb = NimBLEServer::gattRegisterCallback; @@ -286,7 +290,7 @@ void NimBLEServer::start() { int rc = ble_gatts_start(); if (rc != 0) { NIMBLE_LOGE(LOG_TAG, "ble_gatts_start; rc=%d, %s", rc, NimBLEUtils::returnCodeToString(rc)); - return; + return false; } # if MYNEWT_VAL(NIMBLE_CPP_LOG_LEVEL) >= 4 @@ -313,6 +317,7 @@ void NimBLEServer::start() { } m_gattsStarted = true; + return true; } // start /** @@ -335,7 +340,7 @@ bool NimBLEServer::disconnect(uint16_t connHandle, uint8_t reason) const { * @brief Disconnect the specified client with optional reason. * @param [in] connInfo Connection of the client to disconnect. * @param [in] reason code for disconnecting. - * @return NimBLE host return code. + * @return True if successful. */ bool NimBLEServer::disconnect(const NimBLEConnInfo& connInfo, uint8_t reason) const { return disconnect(connInfo.getConnHandle(), reason); @@ -505,7 +510,7 @@ int NimBLEServer::handleGapEvent(ble_gap_event* event, void* arg) { peerInfo.m_desc = event->disconnect.conn; pServer->m_pServerCallbacks->onDisconnect(pServer, peerInfo, event->disconnect.reason); -# if !MYNEWT_VAL(BLE_EXT_ADV) +# if !MYNEWT_VAL(BLE_EXT_ADV) && MYNEWT_VAL(BLE_ROLE_BROADCASTER) if (pServer->m_advertiseOnDisconnect) { pServer->startAdvertising(); } @@ -863,8 +868,13 @@ void NimBLEServer::addService(NimBLEService* service) { /** * @brief Resets the GATT server, used when services are added/removed after initialization. + * @return True if successful. + * @details This will reset the GATT server and re-register all services, characteristics, and + * descriptors that have not been removed. Services, characteristics, and descriptors that have been + * removed but not deleted will be skipped and have their handles cleared, and those that have been + * deleted will be removed from the server's service vector. */ -void NimBLEServer::resetGATT() { +bool NimBLEServer::resetGATT() { # if MYNEWT_VAL(BLE_ROLE_BROADCASTER) NimBLEDevice::stopAdvertising(); # endif @@ -873,8 +883,6 @@ void NimBLEServer::resetGATT() { ble_svc_gap_init(); ble_svc_gatt_init(); - m_gattsStarted = false; - for (auto svcIt = m_svcVec.begin(); svcIt != m_svcVec.end();) { auto* pSvc = *svcIt; if (pSvc->getRemoved() == NIMBLE_ATT_REMOVE_DELETE) { @@ -908,12 +916,17 @@ void NimBLEServer::resetGATT() { } if (pSvc->getRemoved() == 0) { - pSvc->start(); + if (!pSvc->start_internal()) { + NIMBLE_LOGE(LOG_TAG, "Failed to start service: %s", pSvc->getUUID().toString().c_str()); + return false; + } } pSvc->m_handle = 0; ++svcIt; } + + return true; } // resetGATT /** diff --git a/src/NimBLEServer.h b/src/NimBLEServer.h index c299406..7c998cb 100644 --- a/src/NimBLEServer.h +++ b/src/NimBLEServer.h @@ -61,7 +61,7 @@ class NimBLEClient; */ class NimBLEServer { public: - void start(); + bool start(); uint8_t getConnectedCount() const; bool disconnect(uint16_t connHandle, uint8_t reason = BLE_ERR_REM_USER_CONN_TERM) const; bool disconnect(const NimBLEConnInfo& connInfo, uint8_t reason = BLE_ERR_REM_USER_CONN_TERM) const; @@ -123,12 +123,12 @@ class NimBLEServer { static int handleGattEvent(uint16_t connHandle, uint16_t attrHandle, ble_gatt_access_ctxt* ctxt, void* arg); static void gattRegisterCallback(struct ble_gatt_register_ctxt* ctxt, void* arg); void serviceChanged(); - void resetGATT(); + bool resetGATT(); bool m_gattsStarted : 1; bool m_svcChanged : 1; bool m_deleteCallbacks : 1; -# if !MYNEWT_VAL(BLE_EXT_ADV) +# if !MYNEWT_VAL(BLE_EXT_ADV) && MYNEWT_VAL(BLE_ROLE_BROADCASTER) bool m_advertiseOnDisconnect : 1; # endif NimBLEServerCallbacks* m_pServerCallbacks; diff --git a/src/NimBLEService.cpp b/src/NimBLEService.cpp index 47a6fb1..aebe59f 100644 --- a/src/NimBLEService.cpp +++ b/src/NimBLEService.cpp @@ -83,22 +83,8 @@ void NimBLEService::dump() const { * and registers it with the NimBLE stack. * @return bool success/failure . */ -bool NimBLEService::start() { +bool NimBLEService::start_internal() { NIMBLE_LOGD(LOG_TAG, ">> start(): Starting service: UUID: %s", getUUID().toString().c_str()); - // If the server has started before then we need to reset the GATT server - // to update the service/characteristic/descriptor definitions. If characteristics or descriptors - // have been added/removed since the last server start then this service will be started on gatt reset. - if (getServer()->m_svcChanged && getServer()->m_gattsStarted) { - NIMBLE_LOGW(LOG_TAG, "<< start(): GATT change pending, cannot start service"); - return false; - } - - // If started previously and no characteristics have been added or removed, - // then we can skip the service registration process. - if (m_pSvcDef->characteristics && !getServer()->m_svcChanged) { - return true; - } - // Make sure the definitions are cleared first clearServiceDefinitions(); @@ -169,12 +155,14 @@ bool NimBLEService::start() { int rc = ble_gatts_count_cfg(m_pSvcDef); if (rc != 0) { NIMBLE_LOGE(LOG_TAG, "ble_gatts_count_cfg failed, rc= %d, %s", rc, NimBLEUtils::returnCodeToString(rc)); + clearServiceDefinitions(); // Clear the definitions to free memory and reset the service for re-registration. return false; } rc = ble_gatts_add_svcs(m_pSvcDef); if (rc != 0) { NIMBLE_LOGE(LOG_TAG, "ble_gatts_add_svcs, rc= %d, %s", rc, NimBLEUtils::returnCodeToString(rc)); + clearServiceDefinitions(); // Clear the definitions to free memory and reset the service for re-registration. return false; } diff --git a/src/NimBLEService.h b/src/NimBLEService.h index 58e91c7..dcbe1f9 100644 --- a/src/NimBLEService.h +++ b/src/NimBLEService.h @@ -37,11 +37,19 @@ class NimBLEService : public NimBLELocalAttribute { NimBLEService(const NimBLEUUID& uuid); ~NimBLEService(); - NimBLEServer* getServer() const; - std::string toString() const; - void dump() const; - bool isStarted() const; - bool start(); + NimBLEServer* getServer() const; + std::string toString() const; + void dump() const; + bool isStarted() const; + + /** + * @brief Dummy function to start the service. Services are started when the server is started. + * This will be removed in a future release. Use `NimBLEServer::start()` to start the server and all associated services. + */ + __attribute__((deprecated("NimBLEService::start() has no effect. " + "Services are started when the server is started."))) + bool start() { return true; } + NimBLECharacteristic* createCharacteristic(const char* uuid, uint32_t properties = NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::WRITE, uint16_t max_len = BLE_ATT_ATTR_MAX_LEN); @@ -61,6 +69,7 @@ class NimBLEService : public NimBLELocalAttribute { private: friend class NimBLEServer; + bool start_internal(); void clearServiceDefinitions(); std::vector m_vChars{}; diff --git a/src/NimBLEStream.cpp b/src/NimBLEStream.cpp index 3e276a2..3b72ace 100644 --- a/src/NimBLEStream.cpp +++ b/src/NimBLEStream.cpp @@ -608,6 +608,8 @@ bool NimBLEStreamServer::begin( return false; } + m_deleteSvcOnEnd = true; // mark service for deletion on end since we created it here + // Create characteristic with notify + write properties for bidirectional stream uint32_t props = 0; if (txBufSize > 0) { @@ -630,13 +632,6 @@ bool NimBLEStreamServer::begin( goto error; } - m_deleteSvcOnEnd = true; // mark service for deletion on end since we created it here - - if (!pSvc->start()) { - NIMBLE_LOGE(LOG_TAG, "Failed to start service"); - goto error; - } - if (!begin(pChr, txBufSize, rxBufSize)) { NIMBLE_LOGE(LOG_TAG, "Failed to initialize stream with characteristic"); goto error;