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

Commit 1004ddfe authored by Krzysztof Kopyściński's avatar Krzysztof Kopyściński
Browse files

le_audio: remove gatt_read_multi_op_simulate 

Using gatt_read_multi_op_simulate, that was waiting for all Read
operations to complete before returning read values, lead to a bug when
read value was updated by notification before function returned.
If EATT is not supported by peer, reads should pass read values as soon
as read response is received. This patch fixes following scenario:
1. Read Req sent for handle A
2. Read Rsp received for handle A
3. Read Req sent for handle B
4. Notification received for handle A
5. Read Rsp received for handle B
Value from step 2 would be overwritten by value from step 4

Bug: 369970309
Flag: EXEMPT, day-to-day bugfix
Test: atest --host --no-bazel-mode bluetooth_le_audio_client_test
Change-Id: Ie90e94ef8777a16dd2792fb06c13e96bae5e68c1
parent b5c7187e

system/bta/Android.bp

+1 −0
Original line number Diff line number Diff line
@@ -963,6 +963,7 @@ cc_test { 
        ":TestMockMainShimEntry",

        ":TestMockStackBtmInterface",

        ":TestMockStackBtmIso",

        ":TestMockStackGatt",

        ":TestMockStackL2cap",

        ":TestStubOsi",

        "gatt/database.cc",

system/bta/gatt/bta_gattc_queue.cc

+5 −68
Original line number Diff line number Diff line
@@ -145,51 +145,6 @@ void BtaGattQueue::gatt_read_multi_op_finished(tCONN_ID conn_id, tGATT_STATUS st 
  }

}



void BtaGattQueue::gatt_read_multi_op_simulate(tCONN_ID conn_id, tGATT_STATUS status,

                                               uint16_t handle, uint16_t len, uint8_t* value,

                                               void* data_read) {

  gatt_read_multi_simulate_op_data* data = (gatt_read_multi_simulate_op_data*)data_read;



  log::verbose("conn_id: 0x{:x} handle: 0x{:x} status: 0x{:x} len: {}", conn_id, handle, status,

               len);

  if (status == GATT_SUCCESS && ((data->values_end + 2 + len) < MAX_ATT_MTU)) {

    data->values[data->values_end] = (len & 0x00ff);

    data->values[data->values_end + 1] = (len & 0xff00) >> 8;

    data->values_end += 2;



    // concatenate all read values together

    std::copy(value, value + len, data->values.data() + data->values_end);

    data->values_end += len;



    if (data->read_index < data->handles.num_attr - 1) {

      // grab next handle and read it

      data->read_index++;

      uint16_t next_handle = data->handles.handles[data->read_index];



      BTA_GATTC_ReadCharacteristic(conn_id, next_handle, GATT_AUTH_REQ_NONE,

                                   gatt_read_multi_op_simulate, data_read);

      return;

    }

  }



  // all handles read, or bad status, or values too long

  GATT_READ_MULTI_OP_CB tmp_cb = data->cb;

  void* tmp_cb_data = data->cb_data;



  std::array<uint8_t, MAX_ATT_MTU> value_copy = data->values;

  uint16_t value_len = data->values_end;

  auto handles = data->handles;



  osi_free(data_read);



  mark_as_not_executing(conn_id);

  gatt_execute_next_op(conn_id);



  if (tmp_cb) {

    tmp_cb(conn_id, status, handles, value_len, value_copy.data(), tmp_cb_data);

  }

}



void BtaGattQueue::gatt_execute_next_op(tCONN_ID conn_id) {

  log::verbose("conn_id=0x{:x}", conn_id);

  if (gatt_op_queue.empty()) {
@@ -255,28 +210,6 @@ void BtaGattQueue::gatt_execute_next_op(tCONN_ID conn_id) { 
      data->cb_data = op.read_cb_data;

      BTA_GATTC_ReadMultiple(conn_id, op.handles, true, GATT_AUTH_REQ_NONE,

                             gatt_read_multi_op_finished, data);

    } else {

      /* This file contains just queue, and simulating reads should rather live in BTA or

       * stack/gatt. However, placing this logic in layers below would be significantly harder.

       * Having it here is a good balance - it's easy to add, and the API we expose to apps is same

       * as if it was in layers below.

       */

      log::verbose("EATT not supported, simulating read multi. conn_id: 0x{:x} num_handles: {}",

                   conn_id, op.handles.num_attr);

      gatt_read_multi_simulate_op_data* data = (gatt_read_multi_simulate_op_data*)osi_malloc(

              sizeof(gatt_read_multi_simulate_op_data));

      data->cb = op.read_multi_cb;

      data->cb_data = op.read_cb_data;



      std::fill(data->values.begin(), data->values.end(), 0);

      data->values_end = 0;



      data->handles = op.handles;

      data->read_index = 0;

      uint16_t handle = data->handles.handles[data->read_index];



      BTA_GATTC_ReadCharacteristic(conn_id, handle, GATT_AUTH_REQ_NONE, gatt_read_multi_op_simulate,

                                   data);

    }

  }
@@ -333,11 +266,15 @@ void BtaGattQueue::ConfigureMtu(tCONN_ID conn_id, uint16_t mtu) { 
  gatt_execute_next_op(conn_id);

}



void BtaGattQueue::ReadMultiCharacteristic(tCONN_ID conn_id, tBTA_GATTC_MULTI& handles,

bool BtaGattQueue::ReadMultiCharacteristic(tCONN_ID conn_id, tBTA_GATTC_MULTI& handles,

                                           GATT_READ_MULTI_OP_CB cb, void* cb_data) {

  if (!gatt_profile_get_eatt_support_by_conn_id(conn_id)) {

    return false;

  }

  gatt_op_queue[conn_id].push_back({.type = GATT_READ_MULTI,

                                    .handles = handles,

                                    .read_multi_cb = cb,

                                    .read_cb_data = cb_data});

  gatt_execute_next_op(conn_id);

  return true;

}

system/bta/include/bta_gatt_queue.h

+5 −6
Original line number Diff line number Diff line
@@ -44,11 +44,12 @@ public: 
  static void WriteDescriptor(tCONN_ID conn_id, uint16_t handle, std::vector<uint8_t> value,

                              tGATT_WRITE_TYPE write_type, GATT_WRITE_OP_CB cb, void* cb_data);

  static void ConfigureMtu(tCONN_ID conn_id, uint16_t mtu);

  /* This method uses "Read Multiple Variable Length Characteristic Values".

   * If EATT is not enabled on remote, it would send multiple regular Characteristic Reads, and

   * concatenate their values into Length Value Tuple List

  /* This method queues "Read Multiple Variable Length Characteristic Values".

   * Remote must support this method when it supports EATT.

   * Returns true when remote supports EATT and operation was successfully queued.

   * Returns false when remote doesn't support EATT and operation was not scheduled.

   */

  static void ReadMultiCharacteristic(tCONN_ID conn_id, tBTA_GATTC_MULTI& p_read_multi,

  static bool ReadMultiCharacteristic(tCONN_ID conn_id, tBTA_GATTC_MULTI& p_read_multi,

                                      GATT_READ_MULTI_OP_CB cb, void* cb_data);



  /* Holds pending GATT operations */
@@ -80,8 +81,6 @@ private: 
  static void gatt_read_multi_op_finished(tCONN_ID conn_id, tGATT_STATUS status,

                                          tBTA_GATTC_MULTI& handle, uint16_t len, uint8_t* value,

                                          void* data);

  static void gatt_read_multi_op_simulate(tCONN_ID conn_id, tGATT_STATUS status, uint16_t handle,

                                          uint16_t len, uint8_t* value, void* data_read);

  // maps connection id to operations waiting for execution

  static std::unordered_map<tCONN_ID, std::list<gatt_operation>> gatt_op_queue;

  // contain connection ids that currently execute operations

system/bta/le_audio/client.cc

+8 −3
Original line number Diff line number Diff line
@@ -86,6 +86,7 @@ 
#include "osi/include/osi.h"

#include "osi/include/properties.h"

#include "stack/btm/btm_sec.h"

#include "stack/gatt/gatt_int.h"

#include "stack/include/bt_types.h"

#include "stack/include/btm_client_interface.h"

#include "stack/include/btm_status.h"
@@ -617,6 +618,8 @@ public: 


  void AseInitialStateReadRequest(LeAudioDevice* leAudioDevice) {

    int ases_num = leAudioDevice->ases_.size();

    bool is_eatt_supported = gatt_profile_get_eatt_support_by_conn_id(leAudioDevice->conn_id_);



    void* notify_flag_ptr = NULL;



    tBTA_GATTC_MULTI multi_read{};
@@ -630,7 +633,7 @@ public: 
        notify_flag_ptr = INT_TO_PTR(leAudioDevice->notify_connected_after_read_);

      }



      if (!com::android::bluetooth::flags::le_ase_read_multiple_variable()) {

      if (!com::android::bluetooth::flags::le_ase_read_multiple_variable() || !is_eatt_supported) {

        BtaGattQueue::ReadCharacteristic(leAudioDevice->conn_id_,

                                         leAudioDevice->ases_[i].hdls.val_hdl, OnGattReadRspStatic,

                                         notify_flag_ptr);
@@ -2554,7 +2557,9 @@ public: 
  }



  void ReadMustHaveAttributesOnReconnect(LeAudioDevice* leAudioDevice) {

    log::verbose("{}", leAudioDevice->address_);

    bool is_eatt_supported = gatt_profile_get_eatt_support_by_conn_id(leAudioDevice->conn_id_);



    log::verbose("{}, eatt supported {}", leAudioDevice->address_, is_eatt_supported);

    /* Here we read

     * 1) ASCS Control Point CCC descriptor in order to validate proper

     *    behavior of remote device which should store CCC values for bonded device.
@@ -2563,7 +2568,7 @@ public: 
     *    it can change very often which, as we observed, might lead to not being sent by

     *    remote devices

     */

    if (!com::android::bluetooth::flags::le_ase_read_multiple_variable()) {

    if (!com::android::bluetooth::flags::le_ase_read_multiple_variable() || !is_eatt_supported) {

      BtaGattQueue::ReadCharacteristic(leAudioDevice->conn_id_,

                                       leAudioDevice->audio_avail_hdls_.val_hdl,

                                       OnGattReadRspStatic, NULL);

system/bta/test/common/bta_gatt_queue_mock.cc

+2 −1
Original line number Diff line number Diff line
@@ -49,8 +49,9 @@ void BtaGattQueue::ConfigureMtu(uint16_t conn_id, uint16_t mtu) { 
  gatt_queue->ConfigureMtu(conn_id, mtu);

}



void BtaGattQueue::ReadMultiCharacteristic(uint16_t conn_id, tBTA_GATTC_MULTI& p_read_multi,

bool BtaGattQueue::ReadMultiCharacteristic(uint16_t conn_id, tBTA_GATTC_MULTI& p_read_multi,

                                           GATT_READ_MULTI_OP_CB cb, void* cb_data) {

  bluetooth::log::assert_that(gatt_queue, "Mock GATT queue not set!");

  gatt_queue->ReadMultiCharacteristic(conn_id, p_read_multi, cb, cb_data);

  return true;

}