[Bugfix] NimBLEDevice::createServer can crash if stack not initialized.#1117
Merged
[Bugfix] NimBLEDevice::createServer can crash if stack not initialized.#1117
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughArr: server creation no longer runs GAP/GATT init; server/service start/reset now return booleans and use an internal service start path; public Changes
Sequence Diagram(s)sequenceDiagram
participant App as App / Setup
participant Device as NimBLEDevice
participant Server as NimBLEServer
participant Service as NimBLEService
participant Stack as BLE GATT Stack
App->>Device: createServer()
Device->>Server: instantiate singleton (skip GAP/GATT init)
App->>Server: start()
Server->>Stack: resetGATT()
alt resetGATT success
Server->>Server: set flags / prepare services
loop each service
Server->>Service: start_internal()
Service->>Stack: ble_gatts_count_cfg / ble_gatts_add_svcs
Stack-->>Service: registration result
end
Server-->>App: start() returns true
else resetGATT failure
Server-->>App: start() returns false
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NimBLEService.h`:
- Around line 49-51: Fix the typo in the deprecation message for
NimBLEService::start(): update the __attribute__((deprecated(...))) string to
read "NimBLEService::start() has no effect. Services are started when the server
is started." so the message uses "has no effect" (remove the extra "is").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d9eb032-f808-40ca-b2d3-29586b60d676
📒 Files selected for processing (4)
src/NimBLEDevice.cppsrc/NimBLEServer.cppsrc/NimBLEService.cppsrc/NimBLEService.h
💤 Files with no reviewable changes (1)
- src/NimBLEDevice.cpp
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.
h2zero
force-pushed
the
bugfix/create-server-crash
branch
from
eed5542 to
4da6670
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NimBLEService.cpp (1)
153-165:⚠️ Potential issue | 🟡 MinorArrr, reset “started” state on registration failure.
After Line 153 sets service type, failure at Line 157 or Line 163 returns
falsebut leavesisStarted()lookin’ true.🩹 Proposed fix
int rc = ble_gatts_count_cfg(m_pSvcDef); if (rc != 0) { + m_pSvcDef->type = 0; NIMBLE_LOGE(LOG_TAG, "ble_gatts_count_cfg failed, rc= %d, %s", rc, NimBLEUtils::returnCodeToString(rc)); return false; } rc = ble_gatts_add_svcs(m_pSvcDef); if (rc != 0) { + m_pSvcDef->type = 0; NIMBLE_LOGE(LOG_TAG, "ble_gatts_add_svcs, rc= %d, %s", rc, NimBLEUtils::returnCodeToString(rc)); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEService.cpp` around lines 153 - 165, The code sets m_pSvcDef->type then returns false on failures from ble_gatts_count_cfg or ble_gatts_add_svcs but leaves isStarted() true; ensure you reset the service started state before returning by clearing the started flag (e.g. call the service's setter or mutate the m_started state via setStarted(false) or equivalent) in both failure branches after the failing call, so that isStarted() accurately reflects the aborted registration for m_pSvcDef.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NimBLEServer.cpp`:
- Around line 910-912: The call to pSvc->start_internal() in the service loop
currently ignores failures; update the logic in NimBLEServer (where pSvc is
iterated and getRemoved() is checked) to check the return value of
pSvc->start_internal(), and on failure handle it deterministically (e.g., log
the error with context using the server logger, mark the service as
removed/invalid via pSvc->setRemoved() or similar, and abort/return an error
from the overall startup routine so startup does not continue in a partial
state). Ensure you reference pSvc->getRemoved(), pSvc->start_internal(), and the
surrounding startup/initialization function so a failed start_internal() causes
proper cleanup and an early failure return.
---
Outside diff comments:
In `@src/NimBLEService.cpp`:
- Around line 153-165: The code sets m_pSvcDef->type then returns false on
failures from ble_gatts_count_cfg or ble_gatts_add_svcs but leaves isStarted()
true; ensure you reset the service started state before returning by clearing
the started flag (e.g. call the service's setter or mutate the m_started state
via setStarted(false) or equivalent) in both failure branches after the failing
call, so that isStarted() accurately reflects the aborted registration for
m_pSvcDef.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3b137a8-2583-482a-8fa5-afafd7a16dd4
📒 Files selected for processing (4)
src/NimBLEDevice.cppsrc/NimBLEServer.cppsrc/NimBLEService.cppsrc/NimBLEService.h
💤 Files with no reviewable changes (1)
- src/NimBLEDevice.cpp
h2zero
force-pushed
the
bugfix/create-server-crash
branch
from
771180c to
40db200
Compare
h2zero
force-pushed
the
bugfix/create-server-crash
branch
from
40db200 to
402be9a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NimBLEService.cpp (1)
156-167: Ahoy! Consider usin'clearServiceDefinitions()on these error paths, cap'n.When
ble_gatts_count_cfgorble_gatts_add_svcsfails, ye be settin' onlym_pSvcDef->type = 0, but the characteristics and descriptors arrays allocated earlier (lines 106, 124) be left danglin' until the next voyage or when the ship be scuttled (destructor).While this ain't causin' a true leak (the destructor or next
start_internal()call will swab the deck), callin'clearServiceDefinitions()directly would free the treasure immediately and keep the code consistent.🏴☠️ Proposed fix for immediate cleanup
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)); - m_pSvcDef->type = 0; // Clear the type to indicate the service is not started/registered. + clearServiceDefinitions(); // Clears type and frees allocated characteristics/descriptors. 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)); - m_pSvcDef->type = 0; // Clear the type to indicate the service is not started/registered. + clearServiceDefinitions(); // Clears type and frees allocated characteristics/descriptors. return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEService.cpp` around lines 156 - 167, On error paths after ble_gatts_count_cfg and ble_gatts_add_svcs failures, call clearServiceDefinitions() instead of only setting m_pSvcDef->type = 0 so the characteristic and descriptor arrays allocated earlier are freed immediately; update the two error blocks that currently set m_pSvcDef->type = 0 (the ones handling rc != 0 for ble_gatts_count_cfg and ble_gatts_add_svcs) to invoke clearServiceDefinitions() and then return false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NimBLEService.cpp`:
- Around line 156-167: On error paths after ble_gatts_count_cfg and
ble_gatts_add_svcs failures, call clearServiceDefinitions() instead of only
setting m_pSvcDef->type = 0 so the characteristic and descriptor arrays
allocated earlier are freed immediately; update the two error blocks that
currently set m_pSvcDef->type = 0 (the ones handling rc != 0 for
ble_gatts_count_cfg and ble_gatts_add_svcs) to invoke clearServiceDefinitions()
and then return false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b965e38c-7eaa-4ba7-aa2b-f50823b3c263
📒 Files selected for processing (8)
examples/Bluetooth_5/NimBLE_extended_server/NimBLE_extended_server.inoexamples/Bluetooth_5/NimBLE_multi_advertiser/NimBLE_multi_advertiser.inoexamples/L2CAP/L2CAP_Server/L2CAP_Server.inoexamples/NimBLE_Secure_Server/NimBLE_Secure_Server.inoexamples/NimBLE_Server/NimBLE_Server.inoexamples/NimBLE_Server_Whitelist/NimBLE_Server_Whitelist.inosrc/NimBLEService.cppsrc/NimBLEStream.cpp
💤 Files with no reviewable changes (6)
- examples/NimBLE_Server_Whitelist/NimBLE_Server_Whitelist.ino
- examples/L2CAP/L2CAP_Server/L2CAP_Server.ino
- examples/NimBLE_Secure_Server/NimBLE_Secure_Server.ino
- examples/Bluetooth_5/NimBLE_multi_advertiser/NimBLE_multi_advertiser.ino
- examples/NimBLE_Server/NimBLE_Server.ino
- examples/Bluetooth_5/NimBLE_extended_server/NimBLE_extended_server.ino
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NimBLEStream.cpp
h2zero
force-pushed
the
bugfix/create-server-crash
branch
from
402be9a to
6034b1c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Fixes #1088
Summary by CodeRabbit
Refactor
Deprecation
Bug Fixes
Examples / Behavior