Cleanup compiler warnings when using HID device or bonds disabled.#406
Cleanup compiler warnings when using HID device or bonds disabled.#406
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughArrr — This PR gates bond/security logic behind compile-time flags (returnin' safe defaults or logging when disabled) and deprecates NimBLEHIDDevice::startServices(), removin' explicit service starts so the server now handles service startup. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NimBLEDevice.cpp (1)
684-693:⚠️ Potential issue | 🔴 CriticalCritical: off-by-one bound check can read past the bonded array.
Arrr, on Line 688 the condition uses
index > num_peers; whenindex == num_peers, Line 692 indexes out o’ bounds. Useindex >= num_peers.⚙️ Patch fer the bound check
- if (rc != 0 || index > num_peers || index < 0) { + if (rc != 0 || index >= num_peers || index < 0) { return NimBLEAddress{}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEDevice.cpp` around lines 684 - 693, The bound check in the block that calls ble_store_util_bonded_peers can read past peer_id_addrs because it uses `index > num_peers`; change the condition in that function (the block that returns NimBLEAddress(peer_id_addrs[index])) to check `index >= num_peers` (and still keep index < 0 and rc != 0 checks) so that accessing peer_id_addrs[index] cannot go out-of-bounds; look for the code around ble_store_util_bonded_peers, peer_id_addrs, and the NimBLEAddress return to apply the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/NimBLEDevice.cpp`:
- Around line 684-693: The bound check in the block that calls
ble_store_util_bonded_peers can read past peer_id_addrs because it uses `index >
num_peers`; change the condition in that function (the block that returns
NimBLEAddress(peer_id_addrs[index])) to check `index >= num_peers` (and still
keep index < 0 and rc != 0 checks) so that accessing peer_id_addrs[index] cannot
go out-of-bounds; look for the code around ble_store_util_bonded_peers,
peer_id_addrs, and the NimBLEAddress return to apply the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 659b0053-785e-4f87-bb84-c0a47e44b4ee
📒 Files selected for processing (3)
src/NimBLEDevice.cppsrc/NimBLEHIDDevice.cppsrc/NimBLEHIDDevice.h
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/NimBLEDevice.cpp (1)
659-676:⚠️ Potential issue | 🟡 MinorShiver me timbers! The
addressparameter be unused when bonds be disabled, cap'n!When
BLE_STORE_MAX_BONDSbe zero, this whole function becomes justreturn false;— leavin' theaddressparameter adrift with no use. This could summon another compiler warnin' about unused parameters, which be the very sea monster ye be tryin' to vanquish!Ye might want to add a
(void)address;cast like ye did fer theinjectPassKeyandinjectConfirmPasskeyfunctions down below.🏴☠️ Proposed fix to silence the unused parameter warnin'
bool NimBLEDevice::isBonded(const NimBLEAddress& address) { # if MYNEWT_VAL(BLE_STORE_MAX_BONDS) ble_addr_t peer_id_addrs[MYNEWT_VAL(BLE_STORE_MAX_BONDS)]; int num_peers, rc; rc = ble_store_util_bonded_peers(&peer_id_addrs[0], &num_peers, MYNEWT_VAL(BLE_STORE_MAX_BONDS)); if (rc != 0) { return false; } for (int i = 0; i < num_peers; i++) { NimBLEAddress storedAddr(peer_id_addrs[i]); if (storedAddr == address) { return true; } } -# endif +# else + (void)address; +# endif return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEDevice.cpp` around lines 659 - 676, The function leaves the parameter 'address' unused when MYNEWT_VAL(BLE_STORE_MAX_BONDS) is zero, causing compiler warnings; to fix, add a cast-to-void for the unused parameter (e.g., (void)address;) in the branch where the bond-check code is disabled so 'address' is referenced; locate the function using the 'address' parameter in this diff and mirror the pattern used in 'injectPassKey' and 'injectConfirmPasskey' to silence the warning, then return false as before.
🤖 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/NimBLEDevice.cpp`:
- Around line 684-696: When BLE_STORE_MAX_BONDS is zero the function's index
parameter is unused and can trigger compiler warnings; update the `#else` block so
the unused parameter is explicitly marked (e.g. cast to void: (void)index;)
before returning NimBLEAddress{} to silence the warning. Locate the code around
the conditional compilation that returns NimBLEAddress{} (references:
BLE_STORE_MAX_BONDS, NimBLEAddress, and the index parameter) and add the
unused-parameter cast in that `#else` path.
---
Outside diff comments:
In `@src/NimBLEDevice.cpp`:
- Around line 659-676: The function leaves the parameter 'address' unused when
MYNEWT_VAL(BLE_STORE_MAX_BONDS) is zero, causing compiler warnings; to fix, add
a cast-to-void for the unused parameter (e.g., (void)address;) in the branch
where the bond-check code is disabled so 'address' is referenced; locate the
function using the 'address' parameter in this diff and mirror the pattern used
in 'injectPassKey' and 'injectConfirmPasskey' to silence the warning, then
return false as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0201025d-fd8a-4e72-8254-ec382b8f397d
📒 Files selected for processing (1)
src/NimBLEDevice.cpp
h2zero
force-pushed
the
fix-compile-warnings
branch
from
8977376 to
5a301f1
Compare
1 participant
Fixes #397
Summary by CodeRabbit
Refactor
Security
Deprecations