From 2d6d584a9fffbb05789077b0824f6511b715a63c Mon Sep 17 00:00:00 2001 From: Simon Schubert <2@0x2c.org> Date: Tue, 1 Sep 2020 20:19:52 +0200 Subject: [PATCH 1/3] nimble/hs: remove recursive call On esp32, tail call optimization does not exist, which can lead to a stack overflow. --- nimble/host/src/ble_hs_conn.c | 106 +++++++++++++++++----------------- 1 file changed, 53 insertions(+), 53 deletions(-) diff --git a/nimble/host/src/ble_hs_conn.c b/nimble/host/src/ble_hs_conn.c index eb65e3288..b45128e23 100644 --- a/nimble/host/src/ble_hs_conn.c +++ b/nimble/host/src/ble_hs_conn.c @@ -470,29 +470,52 @@ ble_hs_conn_timer(void) int32_t time_diff; uint16_t conn_handle; - conn_handle = BLE_HS_CONN_HANDLE_NONE; - next_exp_in = BLE_HS_FOREVER; - now = ble_npl_time_get(); + for (;;) { + conn_handle = BLE_HS_CONN_HANDLE_NONE; + next_exp_in = BLE_HS_FOREVER; + now = ble_npl_time_get(); - ble_hs_lock(); + ble_hs_lock(); - /* This loop performs one of two tasks: - * 1. Determine if any connections need to be terminated due to timeout. - * If so, break out of the loop and terminate the connection. This - * function will need to be executed again. - * 2. Otherwise, determine when the next timeout will occur. - */ - SLIST_FOREACH(conn, &ble_hs_conns, bhc_next) { - if (!(conn->bhc_flags & BLE_HS_CONN_F_TERMINATING)) { + /* This loop performs one of two tasks: + * 1. Determine if any connections need to be terminated due to timeout. + * If so, break out of the loop and terminate the connection. This + * function will need to be executed again. + * 2. Otherwise, determine when the next timeout will occur. + */ + SLIST_FOREACH(conn, &ble_hs_conns, bhc_next) { + if (!(conn->bhc_flags & BLE_HS_CONN_F_TERMINATING)) { #if MYNEWT_VAL(BLE_L2CAP_RX_FRAG_TIMEOUT) != 0 - /* Check each connection's rx fragment timer. If too much time - * passes after a partial packet is received, the connection is - * terminated. - */ - if (conn->bhc_rx_chan != NULL) { - time_diff = conn->bhc_rx_timeout - now; + /* Check each connection's rx fragment timer. If too much time + * passes after a partial packet is received, the connection is + * terminated. + */ + if (conn->bhc_rx_chan != NULL) { + time_diff = conn->bhc_rx_timeout - now; + if (time_diff <= 0) { + /* ACL reassembly has timed out. Remember the connection + * handle so it can be terminated after the mutex is + * unlocked. + */ + conn_handle = conn->bhc_handle; + break; + } + + /* Determine if this connection is the soonest to time out. */ + if (time_diff < next_exp_in) { + next_exp_in = time_diff; + } + } +#endif + +#if BLE_HS_ATT_SVR_QUEUED_WRITE_TMO + /* Check each connection's rx queued write timer. If too much + * time passes after a prep write is received, the queue is + * cleared. + */ + time_diff = ble_att_svr_ticks_until_tmo(&conn->bhc_att_svr, now); if (time_diff <= 0) { /* ACL reassembly has timed out. Remember the connection * handle so it can be terminated after the mutex is @@ -506,45 +529,22 @@ ble_hs_conn_timer(void) if (time_diff < next_exp_in) { next_exp_in = time_diff; } - } #endif - -#if BLE_HS_ATT_SVR_QUEUED_WRITE_TMO - /* Check each connection's rx queued write timer. If too much - * time passes after a prep write is received, the queue is - * cleared. - */ - time_diff = ble_att_svr_ticks_until_tmo(&conn->bhc_att_svr, now); - if (time_diff <= 0) { - /* ACL reassembly has timed out. Remember the connection - * handle so it can be terminated after the mutex is - * unlocked. - */ - conn_handle = conn->bhc_handle; - break; } - - /* Determine if this connection is the soonest to time out. */ - if (time_diff < next_exp_in) { - next_exp_in = time_diff; - } -#endif } + + ble_hs_unlock(); + + /* If a connection has timed out, terminate it. We need to repeatedly + * call this function again to determine when the next timeout is. + */ + if (conn_handle != BLE_HS_CONN_HANDLE_NONE) { + ble_gap_terminate(conn_handle, BLE_ERR_REM_USER_CONN_TERM); + continue; + } + + return next_exp_in; } - - ble_hs_unlock(); - - /* If a connection has timed out, terminate it. We need to recursively - * call this function again to determine when the next timeout is. This - * is a tail-recursive call, so it should be optimized to execute in the - * same stack frame. - */ - if (conn_handle != BLE_HS_CONN_HANDLE_NONE) { - ble_gap_terminate(conn_handle, BLE_ERR_REM_USER_CONN_TERM); - return ble_hs_conn_timer(); - } - - return next_exp_in; } int From 6823f9cf365735bb7be626ec2ba0fbe0de289b85 Mon Sep 17 00:00:00 2001 From: Prasad Alatkar Date: Wed, 9 Sep 2020 12:54:31 +0530 Subject: [PATCH 2/3] nimble host: Fix minor bug in ble_eddystone_set_adv_data_gen --- nimble/host/src/ble_eddystone.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nimble/host/src/ble_eddystone.c b/nimble/host/src/ble_eddystone.c index 7d80d134d..eccb3e988 100644 --- a/nimble/host/src/ble_eddystone.c +++ b/nimble/host/src/ble_eddystone.c @@ -76,7 +76,7 @@ ble_eddystone_set_adv_data_gen(struct ble_hs_adv_fields *adv_fields, if (adv_fields->num_uuids16 > BLE_EDDYSTONE_MAX_UUIDS16) { return BLE_HS_EINVAL; } - if (svc_data_len > BLE_EDDYSTONE_MAX_SVC_DATA_LEN) { + if (svc_data_len > (BLE_EDDYSTONE_MAX_SVC_DATA_LEN - BLE_EDDYSTONE_SVC_DATA_BASE_SZ)) { return BLE_HS_EINVAL; } if (adv_fields->num_uuids16 > 0 && !adv_fields->uuids16_is_complete) { From 82a7c9de3e6a4825209f7227eabc92dde37d1547 Mon Sep 17 00:00:00 2001 From: Prasad Alatkar Date: Thu, 1 Oct 2020 11:42:40 +0530 Subject: [PATCH 3/3] NimBLE NVS: Additional check to account for NVS operation failure --- nimble/host/store/config/src/ble_store_nvs.c | 21 ++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/nimble/host/store/config/src/ble_store_nvs.c b/nimble/host/store/config/src/ble_store_nvs.c index 6a72e667a..6ee027b43 100644 --- a/nimble/host/store/config/src/ble_store_nvs.c +++ b/nimble/host/store/config/src/ble_store_nvs.c @@ -490,6 +490,11 @@ int ble_store_config_persist_cccds(void) union ble_store_value val; nvs_count = get_nvs_db_attribute(BLE_STORE_OBJ_TYPE_CCCD, 0, NULL, 0); + if (nvs_count == -1) { + ESP_LOGE(TAG, "NVS operation failed while persisting CCCD"); + return BLE_HS_ESTORE_FAIL; + } + if (nvs_count < ble_store_config_num_cccds) { /* NVS db count less than RAM count, write operation */ @@ -516,6 +521,11 @@ int ble_store_config_persist_peer_secs(void) union ble_store_value val; nvs_count = get_nvs_db_attribute(BLE_STORE_OBJ_TYPE_PEER_SEC, 0, NULL, 0); + if (nvs_count == -1) { + ESP_LOGE(TAG, "NVS operation failed while persisting peer sec"); + return BLE_HS_ESTORE_FAIL; + } + if (nvs_count < ble_store_config_num_peer_secs) { /* NVS db count less than RAM count, write operation */ @@ -542,6 +552,11 @@ int ble_store_config_persist_our_secs(void) union ble_store_value val; nvs_count = get_nvs_db_attribute(BLE_STORE_OBJ_TYPE_OUR_SEC, 0, NULL, 0); + if (nvs_count == -1) { + ESP_LOGE(TAG, "NVS operation failed while persisting our sec"); + return BLE_HS_ESTORE_FAIL; + } + if (nvs_count < ble_store_config_num_our_secs) { /* NVS db count less than RAM count, write operation */ @@ -571,7 +586,13 @@ int ble_store_persist_peer_records(void) struct ble_hs_dev_records *peer_dev_rec = ble_rpa_get_peer_dev_records(); nvs_count = get_nvs_db_attribute(BLE_STORE_OBJ_TYPE_PEER_DEV_REC, 0, NULL, 0); + if (nvs_count == -1) { + ESP_LOGE(TAG, "NVS operation failed while persisting peer_dev_rec"); + return BLE_HS_ESTORE_FAIL; + } + if (nvs_count < ble_store_num_peer_dev_rec) { + /* NVS db count less than RAM count, write operation */ ESP_LOGD(TAG, "Persisting peer dev record to NVS..."); peer_rec = peer_dev_rec[ble_store_num_peer_dev_rec - 1];