MYNEWT-702 BLE Host - duplicate update entries If the application calls ble_gap_update_params() while an update connection procedure for that connection is already in progress, the existing entry gets re-inserted in the ble_gap_update_entries list. This yields a cycle in the list, causing the host task to loop endlessly during iteration.
More details: 1. Host initiates a connection update procedure; creates an entry and inserts it into the list (ble_gap_update_entries). 2. Host attempts to initiate a second connection update procedure for the same connection. Since an existing update procedure is ongoing, this attempt fails with a status code of BLE_HS_EALREADY. 3. On detecting the error, the ble_gap_update_params() function tries to clean up (goto done). Part of this cleanup involves freeing the update entry that got allocated earlier in the function but never got inserted into the list. In this case, no entry was allocated, but it looks like one was, because the entry pointer was used to detect a duplicate entry. Consequently, the entry is freed but never removed from the list! 4. The host initiates a third connection update procedure for the same connection. This time, no duplicate is detected because the entry in the list got corrupted when it was freed, making its connection handle value indeterminate. The host allocates the same entry from the pool, populates it, and inserts it into the list. Now the same entry is in the list twice, creating a cycle. When the host iterates this list, it loops forever. Project: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/commit/091fbe12 Tree: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/tree/091fbe12 Diff: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/diff/091fbe12 Branch: refs/heads/master Commit: 091fbe124365ee8b109368928526ed86301b5af8 Parents: 8a6b33b Author: Christopher Collins <ccoll...@apache.org> Authored: Fri Mar 31 15:20:06 2017 -0700 Committer: Christopher Collins <ccoll...@apache.org> Committed: Fri Mar 31 15:21:42 2017 -0700 ---------------------------------------------------------------------- net/nimble/host/src/ble_gap.c | 10 ++++++++-- net/nimble/host/test/src/ble_gap_test.c | 9 +++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/091fbe12/net/nimble/host/src/ble_gap.c ---------------------------------------------------------------------- diff --git a/net/nimble/host/src/ble_gap.c b/net/nimble/host/src/ble_gap.c index ed71374..b494613 100644 --- a/net/nimble/host/src/ble_gap.c +++ b/net/nimble/host/src/ble_gap.c @@ -2869,6 +2869,7 @@ ble_gap_update_params(uint16_t conn_handle, struct ble_l2cap_sig_update_params l2cap_params; struct ble_gap_update_entry *entry; + struct ble_gap_update_entry *dup; struct ble_hs_conn *conn; int rc; @@ -2889,8 +2890,9 @@ ble_gap_update_params(uint16_t conn_handle, goto done; } - entry = ble_gap_update_entry_find(conn_handle, NULL); - if (entry != NULL) { + /* Don't allow two concurrent updates to the same connection. */ + dup = ble_gap_update_entry_find(conn_handle, NULL); + if (dup != NULL) { rc = BLE_HS_EALREADY; goto done; } @@ -2928,6 +2930,10 @@ done: } else { ble_gap_update_entry_free(entry); + /* If the l2cap_params struct is populated, the only error is that the + * controller doesn't support the connection parameters request + * procedure. In this case, fallback to the L2CAP update procedure. + */ if (l2cap_params.itvl_min != 0) { rc = ble_l2cap_sig_update(conn_handle, &l2cap_params, http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/091fbe12/net/nimble/host/test/src/ble_gap_test.c ---------------------------------------------------------------------- diff --git a/net/nimble/host/test/src/ble_gap_test.c b/net/nimble/host/test/src/ble_gap_test.c index ac02343..8471d5b 100644 --- a/net/nimble/host/test/src/ble_gap_test.c +++ b/net/nimble/host/test/src/ble_gap_test.c @@ -1771,6 +1771,15 @@ ble_gap_test_util_update_no_l2cap(struct ble_gap_upd_params *params, if (rc == 0) { TEST_ASSERT(ble_gap_dbg_update_active(2)); + /* Attempt two duplicate updates; ensure BLE_HS_EALREADY gets returned + * both times. Make sure initial update still completes successfully + * (MYNEWT-702). + */ + rc = ble_hs_test_util_conn_update(2, params, 0); + TEST_ASSERT(rc == BLE_HS_EALREADY); + rc = ble_hs_test_util_conn_update(2, params, 0); + TEST_ASSERT(rc == BLE_HS_EALREADY); + /* Receive connection update complete event. */ ble_gap_test_util_rx_update_complete(event_status, params);