From e9b014e5350daef048683e6face55b52e065542f Mon Sep 17 00:00:00 2001 From: Andrzej Kaczmarek Date: Tue, 7 Oct 2025 10:55:12 +0200 Subject: [PATCH] nimble/host: Fix ATT Read By Type Response handling ATT_READ_BY_TYPE_RSP handler will extract proc from the proc list and then call the callback for each handle-value pair in the PDU. However, this means each time proc is extracted it will be then added to the tail of the proc list. If there are more than 1 matching procesures on the list, subsequent extracts when handling *the same PDU* may extract invalid proc from the list and break the procedure. This fixes the problem by adding proc back to list at the head instead of the tail of proc list. This way subsequent iterations of handle-value pairs handling will extract proper proc as it will be the first matching on the list. --- nimble/host/src/ble_att_clt.c | 9 +++- nimble/host/src/ble_gatt_priv.h | 4 +- nimble/host/src/ble_gattc.c | 87 +++++++++++++++++++-------------- 3 files changed, 59 insertions(+), 41 deletions(-) diff --git a/nimble/host/src/ble_att_clt.c b/nimble/host/src/ble_att_clt.c index 2b60f8b53..ddd7fc8a9 100644 --- a/nimble/host/src/ble_att_clt.c +++ b/nimble/host/src/ble_att_clt.c @@ -431,13 +431,20 @@ ble_att_clt_rx_read_type(uint16_t conn_handle, uint16_t cid, struct os_mbuf **rx adata.value_len = data_len - sizeof(*data); adata.value = data->value; - ble_gattc_rx_read_type_adata(conn_handle, cid, &adata); + rc = ble_gattc_rx_read_type_adata(conn_handle, cid, &adata); + if (rc != 0) { + /* We should not call complete callback if this returned an error + * since proc is not added to the proc list. + */ + return 0; + } os_mbuf_adj(*rxom, data_len); } done: /* Notify GATT that the response is done being parsed. */ ble_gattc_rx_read_type_complete(conn_handle, cid, rc); + return rc; } diff --git a/nimble/host/src/ble_gatt_priv.h b/nimble/host/src/ble_gatt_priv.h index 50e9a7d82..9a622788b 100644 --- a/nimble/host/src/ble_gatt_priv.h +++ b/nimble/host/src/ble_gatt_priv.h @@ -119,8 +119,8 @@ void ble_gatts_indicate_fail_notconn(uint16_t conn_handle); void ble_gattc_rx_err(uint16_t conn_handle, uint16_t cid, uint16_t handle, uint16_t status); void ble_gattc_rx_mtu(uint16_t conn_handle, uint16_t cid, int status, uint16_t chan_mtu); -void ble_gattc_rx_read_type_adata(uint16_t conn_handle, uint16_t cid, - struct ble_att_read_type_adata *adata); +int ble_gattc_rx_read_type_adata(uint16_t conn_handle, uint16_t cid, + struct ble_att_read_type_adata *adata); void ble_gattc_rx_read_type_complete(uint16_t conn_handle, uint16_t cid, int status); void ble_gattc_rx_read_rsp(uint16_t conn_handle, uint16_t cid, int status, struct os_mbuf **rxom); diff --git a/nimble/host/src/ble_gattc.c b/nimble/host/src/ble_gattc.c index 0ff4ec28e..a19ed8325 100644 --- a/nimble/host/src/ble_gattc.c +++ b/nimble/host/src/ble_gattc.c @@ -754,12 +754,16 @@ ble_gattc_proc_free(struct ble_gattc_proc *proc) } static void -ble_gattc_proc_insert(struct ble_gattc_proc *proc) +ble_gattc_proc_insert(struct ble_gattc_proc *proc, bool insert_head) { ble_gattc_dbg_assert_proc_not_inserted(proc); ble_hs_lock(); - STAILQ_INSERT_TAIL(&ble_gattc_procs, proc, next); + if (insert_head) { + STAILQ_INSERT_HEAD(&ble_gattc_procs, proc, next); + } else { + STAILQ_INSERT_TAIL(&ble_gattc_procs, proc, next); + } ble_hs_unlock(); } @@ -790,7 +794,7 @@ ble_gattc_proc_set_resume_timer(struct ble_gattc_proc *proc) } static void -ble_gattc_process_status(struct ble_gattc_proc *proc, int status) +ble_gattc_process_status(struct ble_gattc_proc *proc, int status, bool insert_head) { switch (status) { case 0: @@ -798,7 +802,7 @@ ble_gattc_process_status(struct ble_gattc_proc *proc, int status) ble_gattc_proc_set_exp_timer(proc); } - ble_gattc_proc_insert(proc); + ble_gattc_proc_insert(proc, insert_head); ble_hs_timer_resched(); break; @@ -1187,7 +1191,7 @@ ble_gattc_resume_procs(void) proc->flags &= ~BLE_GATTC_PROC_F_STALLED; rc = resume_cb(proc); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); } } @@ -1398,7 +1402,7 @@ done: STATS_INC(ble_gattc_stats, mtu_fail); } - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -1623,7 +1627,7 @@ done: STATS_INC(ble_gattc_stats, disc_all_svcs_fail); } - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -1837,7 +1841,7 @@ done: STATS_INC(ble_gattc_stats, disc_svc_uuid_fail); } - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -2153,7 +2157,7 @@ done: STATS_INC(ble_gattc_stats, find_inc_svcs_fail); } - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -2386,7 +2390,7 @@ done: STATS_INC(ble_gattc_stats, disc_all_chrs_fail); } - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -2626,7 +2630,7 @@ done: STATS_INC(ble_gattc_stats, disc_chrs_uuid_fail); } - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -2836,7 +2840,7 @@ done: STATS_INC(ble_gattc_stats, disc_all_dscs_fail); } - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -2971,7 +2975,7 @@ done: STATS_INC(ble_gattc_stats, read_fail); } - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -3135,7 +3139,7 @@ done: STATS_INC(ble_gattc_stats, read_uuid_fail); } - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -3331,7 +3335,7 @@ done: STATS_INC(ble_gattc_stats, read_long_fail); } - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -3555,7 +3559,7 @@ done: STATS_INC(ble_gattc_stats, read_mult_fail); } - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -3730,7 +3734,7 @@ done: /* Free the mbuf in case the send failed. */ os_mbuf_free_chain(txom); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -4048,7 +4052,7 @@ done: /* Free the mbuf in case of failure. */ os_mbuf_free_chain(txom); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -4337,7 +4341,7 @@ done: attrs[i].om = NULL; } - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); return rc; } @@ -4709,7 +4713,7 @@ done: /* Tell the application that an indication transmission was attempted. */ ble_gap_notify_tx_event(rc, conn_handle, chr_val_handle, 1); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); os_mbuf_free_chain(txom); return rc; } @@ -4777,7 +4781,7 @@ ble_gattc_rx_mtu(uint16_t conn_handle, uint16_t cid, int status, uint16_t chan_m proc = ble_gattc_extract_first_by_conn_cid_op(conn_handle, BLE_L2CAP_CID_ATT, BLE_GATT_OP_MTU); if (proc != NULL) { ble_gattc_mtu_cb(proc, status, 0, chan_mtu); - ble_gattc_process_status(proc, BLE_HS_EDONE); + ble_gattc_process_status(proc, BLE_HS_EDONE, false); } } @@ -4800,7 +4804,7 @@ ble_gattc_rx_find_info_idata(uint16_t conn_handle, uint16_t cid, BLE_GATT_OP_DISC_ALL_DSCS); if (proc != NULL) { rc = ble_gattc_disc_all_dscs_rx_idata(proc, idata); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); } } @@ -4822,7 +4826,7 @@ ble_gattc_rx_find_info_complete(uint16_t conn_handle, uint16_t cid, int status) BLE_GATT_OP_DISC_ALL_DSCS); if (proc != NULL) { rc = ble_gattc_disc_all_dscs_rx_complete(proc, status); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); } } @@ -4845,7 +4849,7 @@ ble_gattc_rx_find_type_value_hinfo(uint16_t conn_handle, uint16_t cid, BLE_GATT_OP_DISC_SVC_UUID); if (proc != NULL) { rc = ble_gattc_disc_svc_uuid_rx_hinfo(proc, hinfo); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); } } @@ -4867,7 +4871,7 @@ ble_gattc_rx_find_type_value_complete(uint16_t conn_handle, uint16_t cid, int st BLE_GATT_OP_DISC_SVC_UUID); if (proc != NULL) { rc = ble_gattc_disc_svc_uuid_rx_complete(proc, status); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); } } @@ -4875,12 +4879,12 @@ ble_gattc_rx_find_type_value_complete(uint16_t conn_handle, uint16_t cid, int st * Dispatches an incoming "attribute data" entry from a read-by-type-response * to the appropriate active GATT procedure. */ -void +int ble_gattc_rx_read_type_adata(uint16_t conn_handle, uint16_t cid, struct ble_att_read_type_adata *adata) { #if !NIMBLE_BLE_ATT_CLT_READ_TYPE - return; + return BLE_HS_ENOTSUP; #endif const struct ble_gattc_rx_adata_entry *rx_entry; @@ -4892,8 +4896,15 @@ ble_gattc_rx_read_type_adata(uint16_t conn_handle, uint16_t cid, &rx_entry); if (proc != NULL) { rc = rx_entry->cb(proc, adata); - ble_gattc_process_status(proc, rc); + /* We need to put procedure on head of the queue since caller expects + * to process the same proc again. + */ + ble_gattc_process_status(proc, rc, true); + } else { + rc = -1; } + + return rc; } /** @@ -4916,7 +4927,7 @@ ble_gattc_rx_read_type_complete(uint16_t conn_handle, uint16_t cid, int status) &rx_entry); if (proc != NULL) { rc = rx_entry->cb(proc, status); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); } } @@ -4939,7 +4950,7 @@ ble_gattc_rx_read_group_type_adata(uint16_t conn_handle, uint16_t cid, BLE_GATT_OP_DISC_ALL_SVCS); if (proc != NULL) { rc = ble_gattc_disc_all_svcs_rx_adata(proc, adata); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); } } @@ -4961,7 +4972,7 @@ ble_gattc_rx_read_group_type_complete(uint16_t conn_handle, uint16_t cid, int st BLE_GATT_OP_DISC_ALL_SVCS); if (proc != NULL) { rc = ble_gattc_disc_all_svcs_rx_complete(proc, status); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); } } @@ -4985,7 +4996,7 @@ ble_gattc_rx_read_rsp(uint16_t conn_handle, uint16_t cid, int status, struct os_ &rx_entry); if (proc != NULL) { rc = rx_entry->cb(proc, status, om); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); } } @@ -5008,7 +5019,7 @@ ble_gattc_rx_read_blob_rsp(uint16_t conn_handle, uint16_t cid, int status, BLE_GATT_OP_READ_LONG); if (proc != NULL) { rc = ble_gattc_read_long_rx_read_rsp(proc, status, om); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); } } @@ -5032,7 +5043,7 @@ ble_gattc_rx_read_mult_rsp(uint16_t conn_handle, uint16_t cid, int status, proc = ble_gattc_extract_first_by_conn_cid_op(conn_handle, cid, op); if (proc != NULL) { ble_gattc_read_mult_cb(proc, status, 0, om); - ble_gattc_process_status(proc, BLE_HS_EDONE); + ble_gattc_process_status(proc, BLE_HS_EDONE, false); } } @@ -5053,7 +5064,7 @@ ble_gattc_rx_write_rsp(uint16_t conn_handle, uint16_t cid) BLE_GATT_OP_WRITE); if (proc != NULL) { ble_gattc_write_cb(proc, 0, 0); - ble_gattc_process_status(proc, BLE_HS_EDONE); + ble_gattc_process_status(proc, BLE_HS_EDONE, false); } } @@ -5079,7 +5090,7 @@ ble_gattc_rx_prep_write_rsp(uint16_t conn_handle, uint16_t cid, int status, &rx_entry); if (proc != NULL) { rc = rx_entry->cb(proc, status, handle, offset, om); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); } } @@ -5102,7 +5113,7 @@ ble_gattc_rx_exec_write_rsp(uint16_t conn_handle, uint16_t cid, int status) ble_gattc_rx_exec_entries, &rx_entry); if (proc != NULL) { rc = rx_entry->cb(proc, status); - ble_gattc_process_status(proc, rc); + ble_gattc_process_status(proc, rc, false); } } @@ -5123,7 +5134,7 @@ ble_gatts_rx_indicate_rsp(uint16_t conn_handle, uint16_t cid) BLE_GATT_OP_INDICATE); if (proc != NULL) { ble_gatts_indicate_rx_rsp(proc); - ble_gattc_process_status(proc, BLE_HS_EDONE); + ble_gattc_process_status(proc, BLE_HS_EDONE, false); } }