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

diff --git a/bta/gatt/bta_gatts_api.cc b/bta/gatt/bta_gatts_api.cc
index 612a4e0..9d7cda0 100644 (file)
--- a/bta/gatt/bta_gatts_api.cc
+++ b/bta/gatt/bta_gatts_api.cc
@@ -24,9 +24,11 @@
 
 #include "bt_target.h"
 
+#include <base/bind.h>
 #include <string.h>
 
 #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<btgatt_db_element_t> 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<btgatt_db_element_t>& 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<btgatt_db_element_t> 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 99eae97..4dcc76a 100644 (file)
--- 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<btgatt_db_element_t>& service);
+typedef base::Callback<void(uint8_t status, int server_if,
+                            std::vector<btgatt_db_element_t> service)>
+    BTA_GATTS_AddServiceCb;
+
+extern void BTA_GATTS_AddService(tGATT_IF server_if,
+                                 std::vector<btgatt_db_element_t> service,
+                                 BTA_GATTS_AddServiceCb cb);
 
 /*******************************************************************************
  *

diff --git a/btif/src/btif_gatt_server.cc b/btif/src/btif_gatt_server.cc
index 35bb2fb..b7fdc8c 100644 (file)
--- 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<btgatt_db_element_t> 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<btgatt_db_element_t> 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<btgatt_db_element_t> 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) {