services/ans: Fix category 0 skipped by immediate-notify-all command#2267
Open
94xhn wants to merge 1 commit into
Open
services/ans: Fix category 0 skipped by immediate-notify-all command#226794xhn wants to merge 1 commit into
94xhn wants to merge 1 commit into
Conversation
The Alert Notification Service control point handler for
BLE_SVC_ANS_CMD_NOT_NEW_ALERT_IMMEDIATE and
BLE_SVC_ANS_CMD_NOT_UNR_ALERT_IMMEDIATE loops over all supported
categories when the peer writes cat_id == 0xff ("all categories"):
for (i = BLE_SVC_ANS_CAT_NUM - 1; i > 0; --i) {
if ((ble_svc_ans_new_alert_cat >> i) & 0x01) {
ble_svc_ans_new_alert_notify(i, NULL);
}
}
The loop condition `i > 0` stops before `i` reaches 0, so category ID
0 (BLE_SVC_ANS_CAT_ID_SIMPLE_ALERT, "Simple Alert") is never notified
by this path, even when it is enabled and has a pending alert. If
Simple Alert is the only category the peer has enabled, a 0xff
immediate-notify request results in zero notifications being sent.
Change the bound to `i >= 0` so category 0 is included, matching the
same fix in both the new-alert and unread-alert-status branches.
Signed-off-by: 94xhn <87560781+94xhn@users.noreply.github.com>
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
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.
Summary
ble_svc_ans_access()innimble/host/services/ans/src/ble_svc_ans.chas anoff-by-one loop boundary that causes category 0 (
BLE_SVC_ANS_CAT_ID_SIMPLE_ALERT,the most basic ANS category) to be silently skipped whenever a client sends the
Notify New Alert ImmediatelyorNotify Unread Alert Immediatelycontrol-pointcommand.
iis signed (int i;, declared at line 160),BLE_SVC_ANS_CAT_NUMis8(
nimble/host/services/ans/include/services/ans/ble_svc_ans.hline 63), andBLE_SVC_ANS_CAT_ID_SIMPLE_ALERTis0(same header, line 51). The loopcondition
i > 0means the body never executes withi == 0, so even when aclient has enabled category 0 via
BLE_SVC_ANS_CMD_EN_NEW_ALERT_CAT(cmd id 0,cat_bit_mask = 1 << 0), animmediate-notify-all command will never emit a notification for it.
This is a plain logic bug, not a hypothetical: category 0 is a normal,
reachable category per the ANS spec, there is no special-casing that excludes
it elsewhere in the file, and the array it indexes into
(
ble_svc_ans_new_alert_cnt[BLE_SVC_ANS_CAT_NUM], size 8) has a valid slot 0.The identical pattern is duplicated for the Unread Alert Status characteristic:
Fix
Change both loop bounds from
i > 0toi >= 0so the iteration includescategory 0. This is a minimal, single-character (
>→>=) change in twoplaces; no other logic is touched.
History check
The repo is a shallow clone (
--depth 1), so I usedgh api repos/apache/mynewt-nimble/commits?path=nimble/host/services/ans/src/ble_svc_ans.cto pull the full history of this file (16 commits). The file has contained
this loop shape since it was moved from
profilestoservicesin 2016.One later commit,
2b5468af4("nimble/ans: Fix out of bounds access",2017), touched the same function but fixed a different problem — handling of
an invalid
cat_id(neither< BLE_SVC_ANS_CAT_NUMnor0xff) — and didnot touch this loop boundary. So this off-by-one has never been fixed.
Verification
Because
ble_svc_ans_new_alert_notify()/ble_svc_ans_unr_alert_notify()ultimately call
ble_gatts_notify(), which needs a full BLE host runtime torun, I wrote a standalone reproduction that extracts the real switch/loop
logic from
ble_svc_ans_access()verbatim (same variable types, same masks,same loop bounds) and replaces only the notify call with a recording stub
function, compiled with/without a
-DBUGGYmacro to toggle the old vs. newloop bound:
-DBUGGY,i > 0):is silently dropped.
8) — category 0 is the one missing.
i >= 0):Both binaries were actually compiled (gcc, MinGW) and run on this machine;
output matches the analysis above exactly.
Disclosure
This investigation, the fix, and this PR description were prepared with the
assistance of Claude (Anthropic AI). I (the human submitter) reviewed the
diff, the reasoning above, and the reproduction results before submitting.
Happy to answer any follow-up questions about the change.