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

Commit 67952a5b authored by Łukasz Rymanowski's avatar Łukasz Rymanowski Committed by Android Build Coastguard Worker
Browse files

gatt: Fix incorrect write to the descriptor. 

This patch fixes regression after
https://android-review.googlesource.com/q/topic:gatt-racecondition-fix

In case of GATT Write error to descriptor or characteristic,
the value which was tried to be written is zeroed in the
onWriteDescriptor/Characteristic callback.
This cause an issue in following use case:

1) Application writes descriptor on unencrypted link to characteristic
which requires authentication
2) Remote device response with an error insufficient authenthication
3) Android repeats write descriptor with new authenthication
requrements with the descriptor value taken from the onWriteDescriptor
callbac - the one which was zeroed. This result in try to write
invalid value to the descriptor

This patch make sure that value in the callback is always the one which
was tried to write, no matter of status.

Bug: 235756799
Test: atest BluetoothInstrumentationTests
Tag: #stability
Merged-In: Icd18b95a3c44c082117d035cdf25961938de829d
Change-Id: Icd18b95a3c44c082117d035cdf25961938de829d
(cherry picked from commit 24f45364)
(cherry picked from commit b64432cf)
Merged-In: Icd18b95a3c44c082117d035cdf25961938de829d
parent 5467b904

android/app/jni/com_android_bluetooth_gatt.cpp

+4 −16
Original line number Diff line number Diff line
@@ -312,14 +312,8 @@ void btgattc_write_characteristic_cb(int conn_id, int status, uint16_t handle, 
  if (!sCallbackEnv.valid()) return;



  ScopedLocalRef<jbyteArray> jb(sCallbackEnv.get(), NULL);

  if (status == 0) {  // Success

  jb.reset(sCallbackEnv->NewByteArray(len));

  sCallbackEnv->SetByteArrayRegion(jb.get(), 0, len, (jbyte*)value);

  } else {

    uint8_t value = 0;

    jb.reset(sCallbackEnv->NewByteArray(1));

    sCallbackEnv->SetByteArrayRegion(jb.get(), 0, 1, (jbyte*)&value);

  }

  sCallbackEnv->CallVoidMethod(mCallbacksObj, method_onWriteCharacteristic,

                               conn_id, status, handle, jb.get());

}
@@ -356,14 +350,8 @@ void btgattc_write_descriptor_cb(int conn_id, int status, uint16_t handle, 
  if (!sCallbackEnv.valid()) return;



  ScopedLocalRef<jbyteArray> jb(sCallbackEnv.get(), NULL);

  if (status == 0) {  // Success

  jb.reset(sCallbackEnv->NewByteArray(len));

  sCallbackEnv->SetByteArrayRegion(jb.get(), 0, len, (jbyte*)value);

  } else {

    uint8_t value = 0;

    jb.reset(sCallbackEnv->NewByteArray(1));

    sCallbackEnv->SetByteArrayRegion(jb.get(), 0, 1, (jbyte*)&value);

  }

  sCallbackEnv->CallVoidMethod(mCallbacksObj, method_onWriteDescriptor, conn_id,

                               status, handle, jb.get());

}

system/bta/gatt/bta_gattc_act.cc

+20 −5
Original line number Diff line number Diff line
@@ -963,13 +963,28 @@ static void bta_gattc_write_cmpl(tBTA_GATTC_CLCB* p_clcb, 
  GATT_WRITE_OP_CB cb = p_clcb->p_q_cmd->api_write.write_cb;

  void* my_cb_data = p_clcb->p_q_cmd->api_write.write_cb_data;



  osi_free_and_reset((void**)&p_clcb->p_q_cmd);



  if (cb) {

    if (p_data->status == 0 &&

        p_clcb->p_q_cmd->api_write.write_type == BTA_GATTC_WRITE_PREPARE) {

      LOG_DEBUG("Handling prepare write success response: handle 0x%04x",

                p_data->p_cmpl->att_value.handle);

      /* If this is successful Prepare write, lets provide to the callback the

       * data provided by server */

      cb(p_clcb->bta_conn_id, p_data->status, p_data->p_cmpl->att_value.handle,

         p_data->p_cmpl->att_value.len, p_data->p_cmpl->att_value.value,

         my_cb_data);

    } else {

      LOG_DEBUG("Handling write response type: %d: handle 0x%04x",

                p_clcb->p_q_cmd->api_write.write_type,

                p_data->p_cmpl->att_value.handle);

      /* Otherwise, provide data which were intended to write. */

      cb(p_clcb->bta_conn_id, p_data->status, p_data->p_cmpl->att_value.handle,

         p_clcb->p_q_cmd->api_write.len, p_clcb->p_q_cmd->api_write.p_value,

         my_cb_data);

    }

  }



  osi_free_and_reset((void**)&p_clcb->p_q_cmd);

}



/** execute write complete */

system/btif/src/btif_gatt_client.cc

+32 −3
Original line number Diff line number Diff line
@@ -72,6 +72,15 @@ extern const btgatt_callbacks_t* bt_gatt_callbacks; 
/*******************************************************************************

 *  Constants & Macros

 ******************************************************************************/

#define CLI_CBACK_WRAP_IN_JNI(P_CBACK, P_CBACK_WRAP)                 \

  do {                                                               \

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

      BTIF_TRACE_API("HAL bt_gatt_callbacks->client->%s", #P_CBACK); \

      do_in_jni_thread(P_CBACK_WRAP);                                \

    } else {                                                         \

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

    }                                                                \

  } while (0)



#define CLI_CBACK_IN_JNI(P_CBACK, ...)                                         \

  do {                                                                         \
@@ -500,8 +509,17 @@ static bt_status_t btif_gattc_read_char_descr(int conn_id, uint16_t handle, 


void write_char_cb(uint16_t conn_id, tGATT_STATUS status, uint16_t handle,

                   uint16_t len, const uint8_t* value, void* data) {

  CLI_CBACK_IN_JNI(write_characteristic_cb, conn_id, status, handle, len,

                   value);

  std::vector<uint8_t> val(value, value + len);

  CLI_CBACK_WRAP_IN_JNI(

      write_characteristic_cb,

      base::BindOnce(

          [](write_characteristic_callback cb, uint16_t conn_id,

             tGATT_STATUS status, uint16_t handle,

             std::vector<uint8_t> moved_value) {

            cb(conn_id, status, handle, moved_value.size(), moved_value.data());

          },

          bt_gatt_callbacks->client->write_characteristic_cb, conn_id, status,

          handle, std::move(val)));

}



static bt_status_t btif_gattc_write_char(int conn_id, uint16_t handle,
@@ -520,7 +538,18 @@ static bt_status_t btif_gattc_write_char(int conn_id, uint16_t handle, 


void write_descr_cb(uint16_t conn_id, tGATT_STATUS status, uint16_t handle,

                    uint16_t len, const uint8_t* value, void* data) {

  CLI_CBACK_IN_JNI(write_descriptor_cb, conn_id, status, handle, len, value);

  std::vector<uint8_t> val(value, value + len);



  CLI_CBACK_WRAP_IN_JNI(

      write_descriptor_cb,

      base::BindOnce(

          [](write_descriptor_callback cb, uint16_t conn_id,

             tGATT_STATUS status, uint16_t handle,

             std::vector<uint8_t> moved_value) {

            cb(conn_id, status, handle, moved_value.size(), moved_value.data());

          },

          bt_gatt_callbacks->client->write_descriptor_cb, conn_id, status,

          handle, std::move(val)));

}



static bt_status_t btif_gattc_write_char_descr(int conn_id, uint16_t handle,