Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 9cb13d7b authored by Kyunglyul Hyun's avatar Kyunglyul Hyun
Browse files

Prevent NPE in bt_gatt_callback 

A thread setting bt_gatt_callbacks and
a thread using it may be different,
potentially causing an NPE if
bt_gatt_callbacks is accessed after being cleaned up.

Use a local variable to ensure safe access in threaded environments.

Bug: 353974484
Flag: EXEMPT, strict null check.
Test: atest BluetoothInstrumentaionTests
Change-Id: I2bd8dd62d0fab0e58db1c1b690406341fc251bf8
parent 0209fb30

system/btif/src/btif_gatt.cc

+3 −1
Original line number Diff line number Diff line
@@ -28,12 +28,14 @@ 


#include "btif_gatt.h"



#include <com_android_bluetooth_flags.h>

#include <hardware/bluetooth.h>

#include <hardware/bt_gatt.h>

#include <stdlib.h>

#include <string.h>



#include "bta_gatt_api.h"

#include "bta/include/bta_gatt_api.h"

#include "btif/include/btif_common.h"

#include "main/shim/distance_measurement_manager.h"

#include "main/shim/le_advertising_manager.h"

system/btif/src/btif_gatt_client.cc

+37 −29
Original line number Diff line number Diff line
@@ -30,6 +30,7 @@ 
#include <base/functional/bind.h>

#include <base/threading/thread.h>

#include <bluetooth/log.h>

#include <com_android_bluetooth_flags.h>

#include <hardware/bluetooth.h>

#include <hardware/bt_gatt.h>

#include <hardware/bt_gatt_types.h>
@@ -89,7 +90,8 @@ static btif_test_cb_t test_cb; 
 ******************************************************************************/

#define CLI_CBACK_WRAP_IN_JNI(P_CBACK, P_CBACK_WRAP)               \

  do {                                                             \

    if (bt_gatt_callbacks && bt_gatt_callbacks->client->P_CBACK) { \

    auto callbacks = bt_gatt_callbacks;                            \

    if (callbacks && callbacks->client->P_CBACK) {                 \

      log::verbose("HAL bt_gatt_callbacks->client->{}", #P_CBACK); \

      do_in_jni_thread(P_CBACK_WRAP);                              \

    } else {                                                       \
@@ -99,9 +101,10 @@ static btif_test_cb_t test_cb; 


#define CLI_CBACK_IN_JNI(P_CBACK, ...)                                 \

  do {                                                                 \

    if (bt_gatt_callbacks && bt_gatt_callbacks->client->P_CBACK) {             \

    auto callbacks = bt_gatt_callbacks;                                \

    if (callbacks && callbacks->client->P_CBACK) {                     \

      log::verbose("HAL bt_gatt_callbacks->client->{}", #P_CBACK);     \

      do_in_jni_thread(Bind(bt_gatt_callbacks->client->P_CBACK, __VA_ARGS__)); \

      do_in_jni_thread(Bind(callbacks->client->P_CBACK, __VA_ARGS__)); \

    } else {                                                           \

      ASSERTC(0, "Callback is NULL", 0);                               \

    }                                                                  \
@@ -139,16 +142,17 @@ uint8_t rssi_request_client_if; 
static void btif_gattc_upstreams_evt(uint16_t event, char* p_param) {

  log::debug("Event {} [{}]", gatt_client_event_text(static_cast<tBTA_GATTC_EVT>(event)), event);



  auto callbacks = bt_gatt_callbacks;

  tBTA_GATTC* p_data = (tBTA_GATTC*)p_param;

  switch (event) {

    case BTA_GATTC_EXEC_EVT: {

      HAL_CBACK(bt_gatt_callbacks, client->execute_write_cb, p_data->exec_cmpl.conn_id,

      HAL_CBACK(callbacks, client->execute_write_cb, p_data->exec_cmpl.conn_id,

                p_data->exec_cmpl.status);

      break;

    }



    case BTA_GATTC_SEARCH_CMPL_EVT: {

      HAL_CBACK(bt_gatt_callbacks, client->search_complete_cb, p_data->search_cmpl.conn_id,

      HAL_CBACK(callbacks, client->search_complete_cb, p_data->search_cmpl.conn_id,

                p_data->search_cmpl.status);

      break;

    }
@@ -163,7 +167,7 @@ static void btif_gattc_upstreams_evt(uint16_t event, char* p_param) { 
      data.is_notify = p_data->notify.is_notify;

      data.len = p_data->notify.len;



      HAL_CBACK(bt_gatt_callbacks, client->notify_cb, p_data->notify.conn_id, data);

      HAL_CBACK(callbacks, client->notify_cb, p_data->notify.conn_id, data);



      if (!p_data->notify.is_notify) {

        BTA_GATTC_SendIndConfirm(p_data->notify.conn_id, p_data->notify.cid);
@@ -174,12 +178,12 @@ static void btif_gattc_upstreams_evt(uint16_t event, char* p_param) { 


    case BTA_GATTC_OPEN_EVT: {

      log::debug("BTA_GATTC_OPEN_EVT {}", p_data->open.remote_bda);

      HAL_CBACK(bt_gatt_callbacks, client->open_cb, p_data->open.conn_id, p_data->open.status,

      HAL_CBACK(callbacks, client->open_cb, p_data->open.conn_id, p_data->open.status,

                p_data->open.client_if, p_data->open.remote_bda);



      if (GATT_DEF_BLE_MTU_SIZE != p_data->open.mtu && p_data->open.mtu) {

        HAL_CBACK(bt_gatt_callbacks, client->configure_mtu_cb, p_data->open.conn_id,

                  p_data->open.status, p_data->open.mtu);

        HAL_CBACK(callbacks, client->configure_mtu_cb, p_data->open.conn_id, p_data->open.status,

                  p_data->open.mtu);

      }



      if (p_data->open.status == GATT_SUCCESS) {
@@ -190,7 +194,7 @@ static void btif_gattc_upstreams_evt(uint16_t event, char* p_param) { 


    case BTA_GATTC_CLOSE_EVT: {

      log::debug("BTA_GATTC_CLOSE_EVT {}", p_data->close.remote_bda);

      HAL_CBACK(bt_gatt_callbacks, client->close_cb, p_data->close.conn_id, p_data->close.status,

      HAL_CBACK(callbacks, client->close_cb, p_data->close.conn_id, p_data->close.status,

                p_data->close.client_if, p_data->close.remote_bda);

      break;

    }
@@ -203,33 +207,33 @@ static void btif_gattc_upstreams_evt(uint16_t event, char* p_param) { 
      break;



    case BTA_GATTC_CFG_MTU_EVT: {

      HAL_CBACK(bt_gatt_callbacks, client->configure_mtu_cb, p_data->cfg_mtu.conn_id,

      HAL_CBACK(callbacks, client->configure_mtu_cb, p_data->cfg_mtu.conn_id,

                p_data->cfg_mtu.status, p_data->cfg_mtu.mtu);

      break;

    }



    case BTA_GATTC_CONGEST_EVT:

      HAL_CBACK(bt_gatt_callbacks, client->congestion_cb, p_data->congest.conn_id,

      HAL_CBACK(callbacks, client->congestion_cb, p_data->congest.conn_id,

                p_data->congest.congested);

      break;



    case BTA_GATTC_PHY_UPDATE_EVT:

      HAL_CBACK(bt_gatt_callbacks, client->phy_updated_cb, p_data->phy_update.conn_id,

      HAL_CBACK(callbacks, client->phy_updated_cb, p_data->phy_update.conn_id,

                p_data->phy_update.tx_phy, p_data->phy_update.rx_phy, p_data->phy_update.status);

      break;



    case BTA_GATTC_CONN_UPDATE_EVT:

      HAL_CBACK(bt_gatt_callbacks, client->conn_updated_cb, p_data->conn_update.conn_id,

      HAL_CBACK(callbacks, client->conn_updated_cb, p_data->conn_update.conn_id,

                p_data->conn_update.interval, p_data->conn_update.latency,

                p_data->conn_update.timeout, p_data->conn_update.status);

      break;



    case BTA_GATTC_SRVC_CHG_EVT:

      HAL_CBACK(bt_gatt_callbacks, client->service_changed_cb, p_data->service_changed.conn_id);

      HAL_CBACK(callbacks, client->service_changed_cb, p_data->service_changed.conn_id);

      break;



    case BTA_GATTC_SUBRATE_CHG_EVT:

      HAL_CBACK(bt_gatt_callbacks, client->subrate_chg_cb, p_data->subrate_chg.conn_id,

      HAL_CBACK(callbacks, client->subrate_chg_cb, p_data->subrate_chg.conn_id,

                p_data->subrate_chg.subrate_factor, p_data->subrate_chg.latency,

                p_data->subrate_chg.cont_num, p_data->subrate_chg.timeout,

                p_data->subrate_chg.status);
@@ -274,8 +278,9 @@ static bt_status_t btif_gattc_register_app(const Uuid& uuid, bool eatt_support) 
                            [](const Uuid& uuid, uint8_t client_id, uint8_t status) {

                              do_in_jni_thread(Bind(

                                      [](const Uuid& uuid, uint8_t client_id, uint8_t status) {

                                        HAL_CBACK(bt_gatt_callbacks, client->register_client_cb,

                                                  status, client_id, uuid);

                                        auto callbacks = bt_gatt_callbacks;

                                        HAL_CBACK(callbacks, client->register_client_cb, status,

                                                  client_id, uuid);

                                      },

                                      uuid, client_id, status));

                            },
@@ -319,7 +324,8 @@ void btif_gattc_open_impl(int client_if, RawAddress address, tBLE_ADDR_TYPE addr 
      tBTM_BLE_VSC_CB vnd_capabilities;

      BTM_BleGetVendorCapabilities(&vnd_capabilities);

      if (!vnd_capabilities.rpa_offloading) {

        HAL_CBACK(bt_gatt_callbacks, client->open_cb, 0, BT_STATUS_UNSUPPORTED, client_if, address);

        auto callbacks = bt_gatt_callbacks;

        HAL_CBACK(callbacks, client->open_cb, 0, BT_STATUS_UNSUPPORTED, client_if, address);

        return;

      }

    }
@@ -404,7 +410,8 @@ void btif_gattc_get_gatt_db_impl(int conn_id) { 
  int count = 0;

  BTA_GATTC_GetGattDb(conn_id, 0x0000, 0xFFFF, &db, &count);



  HAL_CBACK(bt_gatt_callbacks, client->get_gatt_db_cb, conn_id, db, count);

  auto callbacks = bt_gatt_callbacks;

  HAL_CBACK(callbacks, client->get_gatt_db_cb, conn_id, db, count);

  osi_free(db);

}
@@ -469,7 +476,6 @@ void read_desc_cb(uint16_t conn_id, tGATT_STATUS status, uint16_t handle, uint16 
  if (len > 0) {

    memcpy(params.value.value, value, len);

  }



  CLI_CBACK_IN_JNI(read_descriptor_cb, conn_id, status, params);

}
@@ -546,7 +552,8 @@ static void btif_gattc_reg_for_notification_impl(tGATT_IF client_if, const RawAd 
  tGATT_STATUS status = BTA_GATTC_RegisterForNotifications(client_if, bda, handle);



  // TODO(jpawlowski): conn_id is currently unused

  HAL_CBACK(bt_gatt_callbacks, client->register_for_notification_cb,

  auto callbacks = bt_gatt_callbacks;

  HAL_CBACK(callbacks, client->register_for_notification_cb,

            /* conn_id */ 0, 1, status, handle);

}
@@ -563,7 +570,8 @@ static void btif_gattc_dereg_for_notification_impl(tGATT_IF client_if, const Raw 
  tGATT_STATUS status = BTA_GATTC_DeregisterForNotifications(client_if, bda, handle);



  // TODO(jpawlowski): conn_id is currently unused

  HAL_CBACK(bt_gatt_callbacks, client->register_for_notification_cb,

  auto callbacks = bt_gatt_callbacks;

  HAL_CBACK(callbacks, client->register_for_notification_cb,

            /* conn_id */ 0, 0, status, handle);

}

system/btif/src/btif_gatt_server.cc

+27 −24
Original line number Diff line number Diff line
@@ -147,10 +147,11 @@ static void btapp_gatts_free_req_data(uint16_t event, tBTA_GATTS* p_data) { 
static void btapp_gatts_handle_cback(uint16_t event, char* p_param) {

  log::verbose("Event {}", event);



  auto callbacks = bt_gatt_callbacks;

  tBTA_GATTS* p_data = (tBTA_GATTS*)p_param;

  switch (event) {

    case BTA_GATTS_REG_EVT: {

      HAL_CBACK(bt_gatt_callbacks, server->register_server_cb, p_data->reg_oper.status,

      HAL_CBACK(callbacks, server->register_server_cb, p_data->reg_oper.status,

                p_data->reg_oper.server_if, p_data->reg_oper.uuid);

      break;

    }
@@ -161,29 +162,29 @@ static void btapp_gatts_handle_cback(uint16_t event, char* p_param) { 
    case BTA_GATTS_CONNECT_EVT: {

      btif_gatt_check_encrypted_link(p_data->conn.remote_bda, p_data->conn.transport);



      HAL_CBACK(bt_gatt_callbacks, server->connection_cb, p_data->conn.conn_id,

                p_data->conn.server_if, true, p_data->conn.remote_bda);

      HAL_CBACK(callbacks, server->connection_cb, p_data->conn.conn_id, p_data->conn.server_if,

                true, p_data->conn.remote_bda);

      break;

    }



    case BTA_GATTS_DISCONNECT_EVT: {

      HAL_CBACK(bt_gatt_callbacks, server->connection_cb, p_data->conn.conn_id,

                p_data->conn.server_if, false, p_data->conn.remote_bda);

      HAL_CBACK(callbacks, server->connection_cb, p_data->conn.conn_id, p_data->conn.server_if,

                false, p_data->conn.remote_bda);

      break;

    }



    case BTA_GATTS_STOP_EVT:

      HAL_CBACK(bt_gatt_callbacks, server->service_stopped_cb, p_data->srvc_oper.status,

      HAL_CBACK(callbacks, server->service_stopped_cb, p_data->srvc_oper.status,

                p_data->srvc_oper.server_if, p_data->srvc_oper.service_id);

      break;



    case BTA_GATTS_DELETE_EVT:

      HAL_CBACK(bt_gatt_callbacks, server->service_deleted_cb, p_data->srvc_oper.status,

      HAL_CBACK(callbacks, server->service_deleted_cb, p_data->srvc_oper.status,

                p_data->srvc_oper.server_if, p_data->srvc_oper.service_id);

      break;



    case BTA_GATTS_READ_CHARACTERISTIC_EVT: {

      HAL_CBACK(bt_gatt_callbacks, server->request_read_characteristic_cb, p_data->req_data.conn_id,

      HAL_CBACK(callbacks, server->request_read_characteristic_cb, p_data->req_data.conn_id,

                p_data->req_data.trans_id, p_data->req_data.remote_bda,

                p_data->req_data.p_data->read_req.handle, p_data->req_data.p_data->read_req.offset,

                p_data->req_data.p_data->read_req.is_long);
@@ -191,7 +192,7 @@ static void btapp_gatts_handle_cback(uint16_t event, char* p_param) { 
    }



    case BTA_GATTS_READ_DESCRIPTOR_EVT: {

      HAL_CBACK(bt_gatt_callbacks, server->request_read_descriptor_cb, p_data->req_data.conn_id,

      HAL_CBACK(callbacks, server->request_read_descriptor_cb, p_data->req_data.conn_id,

                p_data->req_data.trans_id, p_data->req_data.remote_bda,

                p_data->req_data.p_data->read_req.handle, p_data->req_data.p_data->read_req.offset,

                p_data->req_data.p_data->read_req.is_long);
@@ -200,39 +201,39 @@ static void btapp_gatts_handle_cback(uint16_t event, char* p_param) { 


    case BTA_GATTS_WRITE_CHARACTERISTIC_EVT: {

      const auto& req = p_data->req_data.p_data->write_req;

      HAL_CBACK(bt_gatt_callbacks, server->request_write_characteristic_cb,

                p_data->req_data.conn_id, p_data->req_data.trans_id, p_data->req_data.remote_bda,

                req.handle, req.offset, req.need_rsp, req.is_prep, req.value, req.len);

      HAL_CBACK(callbacks, server->request_write_characteristic_cb, p_data->req_data.conn_id,

                p_data->req_data.trans_id, p_data->req_data.remote_bda, req.handle, req.offset,

                req.need_rsp, req.is_prep, req.value, req.len);

      break;

    }



    case BTA_GATTS_WRITE_DESCRIPTOR_EVT: {

      const auto& req = p_data->req_data.p_data->write_req;

      HAL_CBACK(bt_gatt_callbacks, server->request_write_descriptor_cb, p_data->req_data.conn_id,

      HAL_CBACK(callbacks, server->request_write_descriptor_cb, p_data->req_data.conn_id,

                p_data->req_data.trans_id, p_data->req_data.remote_bda, req.handle, req.offset,

                req.need_rsp, req.is_prep, req.value, req.len);

      break;

    }



    case BTA_GATTS_EXEC_WRITE_EVT: {

      HAL_CBACK(bt_gatt_callbacks, server->request_exec_write_cb, p_data->req_data.conn_id,

      HAL_CBACK(callbacks, server->request_exec_write_cb, p_data->req_data.conn_id,

                p_data->req_data.trans_id, p_data->req_data.remote_bda,

                p_data->req_data.p_data->exec_write);

      break;

    }



    case BTA_GATTS_CONF_EVT:

      HAL_CBACK(bt_gatt_callbacks, server->indication_sent_cb, p_data->req_data.conn_id,

      HAL_CBACK(callbacks, server->indication_sent_cb, p_data->req_data.conn_id,

                p_data->req_data.status);

      break;



    case BTA_GATTS_CONGEST_EVT:

      HAL_CBACK(bt_gatt_callbacks, server->congestion_cb, p_data->congest.conn_id,

      HAL_CBACK(callbacks, server->congestion_cb, p_data->congest.conn_id,

                p_data->congest.congested);

      break;



    case BTA_GATTS_MTU_EVT:

      HAL_CBACK(bt_gatt_callbacks, server->mtu_changed_cb, p_data->req_data.conn_id,

      HAL_CBACK(callbacks, server->mtu_changed_cb, p_data->req_data.conn_id,

                p_data->req_data.p_data->mtu);

      break;
@@ -243,18 +244,18 @@ static void btapp_gatts_handle_cback(uint16_t event, char* p_param) { 
      break;



    case BTA_GATTS_PHY_UPDATE_EVT:

      HAL_CBACK(bt_gatt_callbacks, server->phy_updated_cb, p_data->phy_update.conn_id,

      HAL_CBACK(callbacks, server->phy_updated_cb, p_data->phy_update.conn_id,

                p_data->phy_update.tx_phy, p_data->phy_update.rx_phy, p_data->phy_update.status);

      break;



    case BTA_GATTS_CONN_UPDATE_EVT:

      HAL_CBACK(bt_gatt_callbacks, server->conn_updated_cb, p_data->conn_update.conn_id,

      HAL_CBACK(callbacks, server->conn_updated_cb, p_data->conn_update.conn_id,

                p_data->conn_update.interval, p_data->conn_update.latency,

                p_data->conn_update.timeout, p_data->conn_update.status);

      break;



    case BTA_GATTS_SUBRATE_CHG_EVT:

      HAL_CBACK(bt_gatt_callbacks, server->subrate_chg_cb, p_data->subrate_chg.conn_id,

      HAL_CBACK(callbacks, server->subrate_chg_cb, p_data->subrate_chg.conn_id,

                p_data->subrate_chg.subrate_factor, p_data->subrate_chg.latency,

                p_data->subrate_chg.cont_num, p_data->subrate_chg.timeout,

                p_data->subrate_chg.status);
@@ -390,8 +391,8 @@ static bt_status_t btif_gatts_close(int server_if, const RawAddress& bd_addr, in 


static void on_service_added_cb(tGATT_STATUS status, int server_if,

                                vector<btgatt_db_element_t> service) {

  HAL_CBACK(bt_gatt_callbacks, server->service_added_cb, status, server_if, service.data(),

            service.size());

  auto callbacks = bt_gatt_callbacks;

  HAL_CBACK(callbacks, server->service_added_cb, status, server_if, service.data(), service.size());

}



static void add_service_impl(int server_if, vector<btgatt_db_element_t> service) {
@@ -401,7 +402,8 @@ static void add_service_impl(int server_if, vector<btgatt_db_element_t> service) 
  if (service[0].uuid == Uuid::From16Bit(UUID_SERVCLASS_GATT_SERVER) ||

      service[0].uuid == Uuid::From16Bit(UUID_SERVCLASS_GAP_SERVER)) {

    log::error("Attept to register restricted service");

    HAL_CBACK(bt_gatt_callbacks, server->service_added_cb, BT_STATUS_AUTH_REJECTED, server_if,

    auto callbacks = bt_gatt_callbacks;

    HAL_CBACK(callbacks, server->service_added_cb, BT_STATUS_AUTH_REJECTED, server_if,

              service.data(), service.size());

    return;

  }
@@ -448,7 +450,8 @@ static void btif_gatts_send_response_impl(int conn_id, int trans_id, int status, 


  BTA_GATTS_SendRsp(conn_id, trans_id, static_cast<tGATT_STATUS>(status), &rsp_struct);



  HAL_CBACK(bt_gatt_callbacks, server->response_confirmation_cb, 0, rsp_struct.attr_value.handle);

  auto callbacks = bt_gatt_callbacks;

  HAL_CBACK(callbacks, server->response_confirmation_cb, 0, rsp_struct.attr_value.handle);

}



static bt_status_t btif_gatts_send_response(int conn_id, int trans_id, int status,

system/rust/src/core/ffi/module.cc

+3 −2
Original line number Diff line number Diff line
@@ -53,7 +53,8 @@ namespace { 
future_t* Start() {

  auto fut = future_new();



  if (bt_gatt_callbacks == nullptr) {

  auto callbacks = bt_gatt_callbacks;

  if (callbacks == nullptr) {

    // We can't crash here since some adapter tests mis-use the stack

    // startup/cleanup logic and start the stack without GATT, but don't fully

    // mock out the native layer.
@@ -64,7 +65,7 @@ future_t* Start() { 
    return fut;

  }

  bluetooth::rust_shim::start(

          std::make_unique<bluetooth::gatt::GattServerCallbacks>(*bt_gatt_callbacks->server),

          std::make_unique<bluetooth::gatt::GattServerCallbacks>(*callbacks->server),

          std::make_unique<bluetooth::connection::LeAclManagerShim>(), *fut);



  return fut;