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

Commit a9264e43 authored by Jakub Pawlowski's avatar Jakub Pawlowski
Browse files

Use vector instead of raw pointer in l2cap socket write handling 

Currently, when we receive data from upper layers for LE CoC socket,
we store it in memory and pass a raw pointer around, keeping memory
until the write is acknowledged by controller and event is sent back to
btif layer in on_l2cap_write*_done. If anything goes wrong, we might
loose the pointer to the data and have a memory leak.

From now on, we store the data in std::vector and pass its ownership
down the stack. The moment the data is copied into lower layer buffer,
vector is automatically freed. We no longer pass the data back to
on_l2cap_write*_done callback.

Bug: 68359837
Test: compilation test
Change-Id: If34c0fba8d4a040a242b8c655491b8967a03b96a
parent c732e883

system/bta/include/bta_jv_api.h

+4 −11
Original line number Diff line number Diff line
@@ -244,9 +244,7 @@ typedef struct { 
typedef struct {

  tBTA_JV_STATUS status; /* Whether the operation succeeded or failed. */

  uint32_t handle;       /* The connection handle */

  uint32_t req_id;       /* The req_id in the associated BTA_JvL2capWrite() */

  uint16_t len;          /* The length of the data written. */

  uint8_t* p_data;       /* The buffer where data is held */

  bool cong;             /* congestion status */

} tBTA_JV_L2CAP_WRITE;
@@ -255,8 +253,6 @@ typedef struct { 
  tBTA_JV_STATUS status; /* Whether the operation succeeded or failed. */

  uint16_t channel;      /* The connection channel */

  RawAddress addr;       /* The peer address */

  uint32_t req_id;       /* The req_id in the associated BTA_JvL2capWrite() */

  uint8_t* p_data;       /* The buffer where data is held */

  uint16_t len;          /* The length of the data written. */

  bool cong;             /* congestion status */

} tBTA_JV_L2CAP_WRITE_FIXED;
@@ -680,8 +676,7 @@ tBTA_JV_STATUS BTA_JvL2capReady(uint32_t handle, uint32_t* p_data_size); 
 *                  BTA_JV_FAILURE, otherwise.

 *

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

tBTA_JV_STATUS BTA_JvL2capWrite(uint32_t handle, uint32_t req_id,

                                uint8_t* p_data, uint16_t len,

tBTA_JV_STATUS BTA_JvL2capWrite(uint32_t handle, std::vector<uint8_t> data,

                                uint32_t user_id);



/*******************************************************************************
@@ -697,11 +692,9 @@ tBTA_JV_STATUS BTA_JvL2capWrite(uint32_t handle, uint32_t req_id, 
 *                  BTA_JV_FAILURE, otherwise.

 *

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

tBTA_JV_STATUS BTA_JvL2capWriteFixed(uint16_t channel, const RawAddress& addr,

                                     uint32_t req_id,

void BTA_JvL2capWriteFixed(uint16_t channel, const RawAddress& addr,

                           tBTA_JV_L2CAP_CBACK* p_cback,

                                     uint8_t* p_data, uint16_t len,

                                     uint32_t user_id);

                           std::vector<uint8_t> data, uint32_t user_id);



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

 *

system/bta/jv/bta_jv_act.cc

+10 −16
Original line number Diff line number Diff line
@@ -1132,8 +1132,8 @@ void bta_jv_l2cap_stop_server(uint16_t local_psm, uint32_t l2cap_socket_id) { 
}



/* Write data to an L2CAP connection */

void bta_jv_l2cap_write(uint32_t handle, uint32_t req_id, uint8_t* p_data,

                        uint16_t len, uint32_t user_id, tBTA_JV_L2C_CB* p_cb) {

void bta_jv_l2cap_write(uint32_t handle, const std::vector<uint8_t>& data,

                        uint32_t user_id, tBTA_JV_L2C_CB* p_cb) {

  /* As we check this callback exists before the tBTA_JV_API_L2CAP_WRITE can be

   * send through the API this check should not be needed. But the API is not

   * designed to be used (safely at least) in a multi-threaded scheduler, hence
@@ -1144,11 +1144,7 @@ void bta_jv_l2cap_write(uint32_t handle, uint32_t req_id, uint8_t* p_data, 
   * of 4 disconnects, as a disconnect on the server channel causes a disconnect

   * to be send on the client (notification) channel, but at the peer typically

   * disconnects both the OBEX disconnect request crosses the incoming l2cap

   * disconnect. If p_cback is cleared, we simply discard the data. RISK: The

   * caller must handle any cleanup based on another signal than

   * BTA_JV_L2CAP_WRITE_EVT, which is typically not possible, as the pointer to

   * the allocated buffer is stored in this message, and can therefore not be

   * freed, hence we have a mem-leak-by-design.*/

   * disconnect. If p_cback is cleared, we simply discard the data.*/

  if (!p_cb->p_cback) {

    /* As this pointer is checked in the API function, this occurs only when the

     * channel is disconnected after the API function is called, but before the
@@ -1160,13 +1156,12 @@ void bta_jv_l2cap_write(uint32_t handle, uint32_t req_id, uint8_t* p_data, 
  tBTA_JV_L2CAP_WRITE evt_data;

  evt_data.status = BTA_JV_FAILURE;

  evt_data.handle = handle;

  evt_data.req_id = req_id;

  evt_data.p_data = p_data;

  evt_data.cong = p_cb->cong;

  evt_data.len = 0;

  bta_jv_pm_conn_busy(p_cb->p_pm_cb);

  if (!evt_data.cong &&

      BT_PASS == GAP_ConnWriteData(handle, p_data, len, &evt_data.len)) {

      BT_PASS ==

          GAP_ConnWriteData(handle, data.data(), data.size(), &evt_data.len)) {

    evt_data.status = BTA_JV_SUCCESS;

  }

  tBTA_JV bta_jv;
@@ -1176,19 +1171,18 @@ void bta_jv_l2cap_write(uint32_t handle, uint32_t req_id, uint8_t* p_data, 


/* Write data to an L2CAP connection using Fixed channels */

void bta_jv_l2cap_write_fixed(uint16_t channel, const RawAddress& addr,

                              uint32_t req_id, uint8_t* p_data, uint16_t len,

                              const std::vector<uint8_t>& data,

                              uint32_t user_id, tBTA_JV_L2CAP_CBACK* p_cback) {

  tBTA_JV_L2CAP_WRITE_FIXED evt_data;

  evt_data.status = BTA_JV_FAILURE;

  evt_data.channel = channel;

  evt_data.addr = addr;

  evt_data.req_id = req_id;

  evt_data.p_data = p_data;

  evt_data.len = 0;



  BT_HDR* msg = (BT_HDR*)osi_malloc(sizeof(BT_HDR) + len + L2CAP_MIN_OFFSET);

  memcpy(((uint8_t*)(msg + 1)) + L2CAP_MIN_OFFSET, p_data, len);

  msg->len = len;

  BT_HDR* msg =

      (BT_HDR*)osi_malloc(sizeof(BT_HDR) + data.size() + L2CAP_MIN_OFFSET);

  memcpy(((uint8_t*)(msg + 1)) + L2CAP_MIN_OFFSET, data.data(), data.size());

  msg->len = data.size();

  msg->offset = L2CAP_MIN_OFFSET;



  L2CA_SendFixedChnlData(channel, addr, msg);

system/bta/jv/bta_jv_api.cc

+7 −15
Original line number Diff line number Diff line
@@ -512,16 +512,15 @@ tBTA_JV_STATUS BTA_JvL2capReady(uint32_t handle, uint32_t* p_data_size) { 
 *                  BTA_JV_FAILURE, otherwise.

 *

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

tBTA_JV_STATUS BTA_JvL2capWrite(uint32_t handle, uint32_t req_id,

                                uint8_t* p_data, uint16_t len,

tBTA_JV_STATUS BTA_JvL2capWrite(uint32_t handle, std::vector<uint8_t> data,

                                uint32_t user_id) {

  VLOG(2) << __func__;



  if (handle >= BTA_JV_MAX_L2C_CONN || !bta_jv_cb.l2c_cb[handle].p_cback)

    return BTA_JV_FAILURE;



  do_in_bta_thread(FROM_HERE, Bind(&bta_jv_l2cap_write, handle, req_id, p_data,

                                   len, user_id, &bta_jv_cb.l2c_cb[handle]));

  do_in_bta_thread(FROM_HERE, Bind(&bta_jv_l2cap_write, handle, std::move(data),

                                   user_id, &bta_jv_cb.l2c_cb[handle]));

  return BTA_JV_SUCCESS;

}
@@ -534,20 +533,13 @@ tBTA_JV_STATUS BTA_JvL2capWrite(uint32_t handle, uint32_t req_id, 
 *                  called with BTA_JV_L2CAP_WRITE_EVT. Works for

 *                  fixed-channel connections

 *

 * Returns          BTA_JV_SUCCESS, if the request is being processed.

 *                  BTA_JV_FAILURE, otherwise.

 *

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

tBTA_JV_STATUS BTA_JvL2capWriteFixed(uint16_t channel, const RawAddress& addr,

                                     uint32_t req_id,

void BTA_JvL2capWriteFixed(uint16_t channel, const RawAddress& addr,

                           tBTA_JV_L2CAP_CBACK* p_cback,

                                     uint8_t* p_data, uint16_t len,

                                     uint32_t user_id) {

                           std::vector<uint8_t> data, uint32_t user_id) {

  VLOG(2) << __func__;



  do_in_bta_thread(FROM_HERE, Bind(&bta_jv_l2cap_write_fixed, channel, addr,

                                   req_id, p_data, len, user_id, p_cback));

  return BTA_JV_SUCCESS;

                                   std::move(data), user_id, p_cback));

}



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

system/bta/jv/bta_jv_int.h

+5 −5
Original line number Diff line number Diff line
@@ -166,9 +166,9 @@ extern void bta_jv_l2cap_start_server( 
    uint32_t l2cap_socket_id);

extern void bta_jv_l2cap_stop_server(uint16_t local_psm,

                                     uint32_t l2cap_socket_id);

extern void bta_jv_l2cap_write(uint32_t handle, uint32_t req_id,

                               uint8_t* p_data, uint16_t len, uint32_t user_id,

                               tBTA_JV_L2C_CB* p_cb);

extern void bta_jv_l2cap_write(uint32_t handle,

                               const std::vector<uint8_t>& data,

                               uint32_t user_id, tBTA_JV_L2C_CB* p_cb);

extern void bta_jv_rfcomm_connect(tBTA_SEC sec_mask, tBTA_JV_ROLE role,

                                  uint8_t remote_scn,

                                  const RawAddress& peer_bd_addr,
@@ -193,8 +193,8 @@ extern void bta_jv_l2cap_start_server_le(uint16_t local_chan, 
                                         uint32_t l2cap_socket_id);

extern void bta_jv_l2cap_stop_server_le(uint16_t local_chan);

extern void bta_jv_l2cap_write_fixed(uint16_t channel, const RawAddress& addr,

                                     uint32_t req_id, uint8_t* p_data,

                                     uint16_t len, uint32_t user_id,

                                     const std::vector<uint8_t>& data,

                                     uint32_t user_id,

                                     tBTA_JV_L2CAP_CBACK* p_cback);

extern void bta_jv_l2cap_close_fixed(uint32_t handle);

system/btif/src/btif_sock_l2cap.cc

+13 −33
Original line number Diff line number Diff line
@@ -622,13 +622,9 @@ static void on_l2cap_outgoing_congest(tBTA_JV_L2CAP_CONG* p, uint32_t id) { 
  }

}



static void on_l2cap_write_done(void* req_id, uint16_t len, uint32_t id) {

static void on_l2cap_write_done(uint16_t len, uint32_t id) {

  l2cap_socket* sock;



  if (req_id != NULL) {

    osi_free(req_id);  // free the buffer

  }



  int app_uid = -1;



  std::unique_lock<std::mutex> lock(state_lock);
@@ -646,13 +642,9 @@ static void on_l2cap_write_done(void* req_id, uint16_t len, uint32_t id) { 
  uid_set_add_tx(uid_set, app_uid, len);

}



static void on_l2cap_write_fixed_done(void* req_id, uint16_t len, uint32_t id) {

static void on_l2cap_write_fixed_done(uint16_t len, uint32_t id) {

  l2cap_socket* sock;



  if (req_id != NULL) {

    osi_free(req_id);  // free the buffer

  }



  int app_uid = -1;

  std::unique_lock<std::mutex> lock(state_lock);

  sock = btsock_l2cap_find_by_id_l(id);
@@ -755,14 +747,12 @@ static void btsock_l2cap_cbk(tBTA_JV_EVT event, tBTA_JV* p_data, 


    case BTA_JV_L2CAP_WRITE_EVT:

      APPL_TRACE_DEBUG("BTA_JV_L2CAP_WRITE_EVT: id: %u", l2cap_socket_id);

      on_l2cap_write_done(p_data->l2c_write.p_data, p_data->l2c_write.len,

                          l2cap_socket_id);

      on_l2cap_write_done(p_data->l2c_write.len, l2cap_socket_id);

      break;



    case BTA_JV_L2CAP_WRITE_FIXED_EVT:

      APPL_TRACE_DEBUG("BTA_JV_L2CAP_WRITE_FIXED_EVT: id: %u", l2cap_socket_id);

      on_l2cap_write_fixed_done(p_data->l2c_write_fixed.p_data,

                                p_data->l2c_write.len, l2cap_socket_id);

      on_l2cap_write_fixed_done(p_data->l2c_write.len, l2cap_socket_id);

      break;



    case BTA_JV_L2CAP_CONG_EVT:
@@ -1000,7 +990,7 @@ void btsock_l2cap_signaled(int fd, int flags, uint32_t user_id) { 


      if (!(flags & SOCK_THREAD_FD_EXCEPTION) ||

          (ioctl(sock->our_fd, FIONREAD, &size) == 0 && size)) {

        uint8_t* buffer = (uint8_t*)osi_malloc(L2CAP_MAX_SDU_LENGTH);

        std::vector<uint8_t> buffer(L2CAP_MAX_SDU_LENGTH);

        /* The socket is created with SOCK_SEQPACKET, hence we read one message

         * at the time. The maximum size of a message is allocated to ensure

         * data is not lost. This is okay to do as Android uses virtual memory,
@@ -1014,29 +1004,19 @@ void btsock_l2cap_signaled(int fd, int flags, uint32_t user_id) { 
         * in a tight loop, hence we risk the ioctl will return the total amount

         * of data in the buffer, which could be multiple 64kbyte chunks.

         * UPDATE: As the stack cannot handle 64kbyte buffers, the size is

         * reduced to around 8kbyte - and using malloc for buffer allocation

         * here seems to be wrong

         * UPDATE: Since we are responsible for freeing the buffer in the

         * write_complete_ind, it is OK to use malloc. */

         * reduced to around 8kbyte */

        ssize_t count;

        OSI_NO_INTR(count = recv(fd, buffer, L2CAP_MAX_SDU_LENGTH,

        OSI_NO_INTR(count = recv(fd, buffer.data(), buffer.size(),

                                 MSG_NOSIGNAL | MSG_DONTWAIT));

        APPL_TRACE_DEBUG(

            "btsock_l2cap_signaled - %d bytes received from socket", count);

        APPL_TRACE_DEBUG("%s: %d bytes received from socket", __func__, count);



        buffer.resize((count == -1) ? 0 : count);



        if (sock->fixed_chan) {

          if (BTA_JvL2capWriteFixed(sock->channel, sock->addr,

                                    PTR_TO_UINT(buffer), btsock_l2cap_cbk,

                                    buffer, count, user_id) != BTA_JV_SUCCESS) {

            // On fail, free the buffer

            on_l2cap_write_fixed_done(buffer, count, user_id);

          }

          BTA_JvL2capWriteFixed(sock->channel, sock->addr, btsock_l2cap_cbk,

                                std::move(buffer), user_id);

        } else {

          if (BTA_JvL2capWrite(sock->handle, PTR_TO_UINT(buffer), buffer, count,

                               user_id) != BTA_JV_SUCCESS) {

            // On fail, free the buffer

            on_l2cap_write_done(buffer, count, user_id);

          }

          BTA_JvL2capWrite(sock->handle, std::move(buffer), user_id);

        }

      }

    } else