From 541862ce72d4203f015acb0956541767fa2470f1 Mon Sep 17 00:00:00 2001 From: Jakub Pawlowski Date: Wed, 7 Feb 2018 11:37:18 -0800 Subject: [PATCH] Call BTA_GATTS_AddService on correct thread Right now BTA_GATTS_AddService is incorrectly called on btif thread. This might lead to situations where it is executed while bta thread is already cleaning up, and deleted the control blocks, resulting in crashes. From now on, execute it on bta thread. This ensures that it is executed before control blocks are freed. Test: sl4a GattClientTest Bug: 73054849 Change-Id: Ie2bf80414bceba2590c21d3825c78fbb58449520 --- bta/gatt/bta_gatts_api.cc | 71 +++++++++++++++++++++++++++----------------- bta/include/bta_gatt_api.h | 9 ++++-- btif/src/btif_gatt_server.cc | 14 ++++++--- 3 files changed, 61 insertions(+), 33 deletions(-) diff --git a/bta/gatt/bta_gatts_api.cc b/bta/gatt/bta_gatts_api.cc index 612a4e008..9d7cda0d6 100644 --- a/bta/gatt/bta_gatts_api.cc +++ b/bta/gatt/bta_gatts_api.cc @@ -24,9 +24,11 @@ #include "bt_target.h" +#include #include #include "bt_common.h" +#include "bta_closure_api.h" #include "bta_gatt_api.h" #include "bta_gatts_int.h" #include "bta_sys.h" @@ -111,6 +113,43 @@ void BTA_GATTS_AppDeregister(tGATT_IF server_if) { bta_sys_sendmsg(p_buf); } +void bta_gatts_add_service_impl(tGATT_IF server_if, + std::vector service, + BTA_GATTS_AddServiceCb cb) { + uint8_t rcb_idx = + bta_gatts_find_app_rcb_idx_by_app_if(&bta_gatts_cb, server_if); + + LOG(INFO) << __func__ << ": rcb_idx=" << +rcb_idx; + + if (rcb_idx == BTA_GATTS_INVALID_APP) { + cb.Run(GATT_ERROR, server_if, std::move(service)); + return; + } + + uint8_t srvc_idx = bta_gatts_alloc_srvc_cb(&bta_gatts_cb, rcb_idx); + if (srvc_idx == BTA_GATTS_INVALID_APP) { + cb.Run(GATT_ERROR, server_if, std::move(service)); + return; + } + + uint16_t status = GATTS_AddService(server_if, service.data(), service.size()); + if (status != GATT_SERVICE_STARTED) { + memset(&bta_gatts_cb.srvc_cb[srvc_idx], 0, sizeof(tBTA_GATTS_SRVC_CB)); + LOG(ERROR) << __func__ << ": service creation failed."; + cb.Run(GATT_ERROR, server_if, std::move(service)); + return; + } + + bta_gatts_cb.srvc_cb[srvc_idx].service_uuid = service[0].uuid; + + // service_id is equal to service start handle + bta_gatts_cb.srvc_cb[srvc_idx].service_id = service[0].attribute_handle; + bta_gatts_cb.srvc_cb[srvc_idx].idx = srvc_idx; + + cb.Run(GATT_SUCCESS, server_if, std::move(service)); + return; +} + /******************************************************************************* * * Function BTA_GATTS_AddService @@ -126,33 +165,11 @@ void BTA_GATTS_AppDeregister(tGATT_IF server_if) { * service cannot be added. * ******************************************************************************/ -extern uint16_t BTA_GATTS_AddService( - tGATT_IF server_if, std::vector& service) { - uint8_t rcb_idx = - bta_gatts_find_app_rcb_idx_by_app_if(&bta_gatts_cb, server_if); - - LOG(INFO) << __func__ << ": rcb_idx=" << +rcb_idx; - - if (rcb_idx == BTA_GATTS_INVALID_APP) return GATT_ERROR; - - uint8_t srvc_idx = bta_gatts_alloc_srvc_cb(&bta_gatts_cb, rcb_idx); - if (srvc_idx == BTA_GATTS_INVALID_APP) return GATT_ERROR; - - uint16_t status = GATTS_AddService(server_if, service.data(), service.size()); - - if (status == GATT_SERVICE_STARTED) { - bta_gatts_cb.srvc_cb[srvc_idx].service_uuid = service[0].uuid; - - // service_id is equal to service start handle - bta_gatts_cb.srvc_cb[srvc_idx].service_id = service[0].attribute_handle; - bta_gatts_cb.srvc_cb[srvc_idx].idx = srvc_idx; - - return GATT_SUCCESS; - } else { - memset(&bta_gatts_cb.srvc_cb[srvc_idx], 0, sizeof(tBTA_GATTS_SRVC_CB)); - LOG(ERROR) << __func__ << ": service creation failed."; - return GATT_ERROR; - } +extern void BTA_GATTS_AddService(tGATT_IF server_if, + std::vector service, + BTA_GATTS_AddServiceCb cb) { + do_in_bta_thread(FROM_HERE, base::Bind(&bta_gatts_add_service_impl, server_if, + std::move(service), std::move(cb))); } /******************************************************************************* diff --git a/bta/include/bta_gatt_api.h b/bta/include/bta_gatt_api.h index 99eae97b8..4dcc76ab0 100644 --- a/bta/include/bta_gatt_api.h +++ b/bta/include/bta_gatt_api.h @@ -891,8 +891,13 @@ extern void BTA_GATTS_AppDeregister(tGATT_IF server_if); * service cannot be added. * ******************************************************************************/ -extern uint16_t BTA_GATTS_AddService(tGATT_IF server_if, - std::vector& service); +typedef base::Callback service)> + BTA_GATTS_AddServiceCb; + +extern void BTA_GATTS_AddService(tGATT_IF server_if, + std::vector service, + BTA_GATTS_AddServiceCb cb); /******************************************************************************* * diff --git a/btif/src/btif_gatt_server.cc b/btif/src/btif_gatt_server.cc index 35bb2fb06..b7fdc8c86 100644 --- a/btif/src/btif_gatt_server.cc +++ b/btif/src/btif_gatt_server.cc @@ -346,6 +346,12 @@ static bt_status_t btif_gatts_close(int server_if, const RawAddress& bd_addr, Bind(&btif_gatts_close_impl, server_if, bd_addr, conn_id)); } +static void on_service_added_cb(uint8_t status, int server_if, + vector service) { + HAL_CBACK(bt_gatt_callbacks, server->service_added_cb, status, server_if, + std::move(service)); +} + static void add_service_impl(int server_if, vector service) { // TODO(jpawlowski): btif should be a pass through layer, and no checks should @@ -359,16 +365,16 @@ static void add_service_impl(int server_if, return; } - int status = BTA_GATTS_AddService(server_if, service); - HAL_CBACK(bt_gatt_callbacks, server->service_added_cb, status, server_if, - std::move(service)); + BTA_GATTS_AddService( + server_if, service, + jni_thread_wrapper(FROM_HERE, base::Bind(&on_service_added_cb))); } static bt_status_t btif_gatts_add_service(int server_if, vector service) { CHECK_BTGATT_INIT(); return do_in_jni_thread( - Bind(&add_service_impl, server_if, std::move(service))); + FROM_HERE, Bind(&add_service_impl, server_if, std::move(service))); } static bt_status_t btif_gatts_stop_service(int server_if, int service_handle) { -- 2.11.0