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

Commit 7870bd56 authored by Jakub Tyszkowski's avatar Jakub Tyszkowski Committed by Łukasz Rymanowski
Browse files

IsoManager: Improve callback calls

Handles the use-after-free issue the proper and recommended way using
weak pointers. The unit tests were also added to make sure no call on
the already released object is made.

Bug: 288297494
Tag: #feature
Test: atest --host net_test_btm_iso --no-bazel-mode

Change-Id: I69ab25c1c19875142360436c52a428499198a1c3
parent 775ae5a0
Loading
Loading
Loading
Loading
+15 −47
Original line number Diff line number Diff line
@@ -55,7 +55,6 @@ static constexpr uint8_t kStateFlagHasDataPathSet = 0x04;
static constexpr uint8_t kStateFlagIsBroadcast = 0x10;

constexpr char kBtmLogTag[] = "ISO";
static bool iso_impl_initialized_ = false;

struct iso_sync_info {
  uint32_t first_sync_ts;
@@ -96,10 +95,11 @@ struct iso_impl {
  iso_impl() {
    iso_credits_ = controller_get_interface()->get_iso_buffer_count();
    iso_buffer_size_ = controller_get_interface()->get_iso_data_size();
    iso_impl_initialized_ = true;
    LOG_INFO("%p created, iso credits: %d, buffer size: %d.", this,
             iso_credits_.load(), iso_buffer_size_);
  }

  ~iso_impl() { iso_impl_initialized_ = false; }
  ~iso_impl() { LOG_INFO("%p removed.", this); }

  void handle_register_cis_callbacks(CigCallbacks* callbacks) {
    LOG_ASSERT(callbacks != nullptr) << "Invalid CIG callbacks";
@@ -124,11 +124,6 @@ struct iso_impl {
    uint16_t conn_handle;
    cig_create_cmpl_evt evt;

    if (!iso_impl_initialized_) {
      LOG_WARN("iso_impl not available anymore");
      return;
    }

    LOG_ASSERT(cig_callbacks_ != nullptr) << "Invalid CIG callbacks";
    LOG_ASSERT(len >= 3) << "Invalid packet length: " << +len;

@@ -197,7 +192,7 @@ struct iso_impl {
        cig_params.sca, cig_params.packing, cig_params.framing,
        cig_params.max_trans_lat_stom, cig_params.max_trans_lat_mtos,
        cig_params.cis_cfgs.size(), cig_params.cis_cfgs.data(),
        base::BindOnce(&iso_impl::on_set_cig_params, base::Unretained(this),
        base::BindOnce(&iso_impl::on_set_cig_params, weak_factory_.GetWeakPtr(),
                       cig_id, cig_params.sdu_itv_mtos));

    BTM_LogHistory(
@@ -215,18 +210,13 @@ struct iso_impl {
        cig_params.sca, cig_params.packing, cig_params.framing,
        cig_params.max_trans_lat_stom, cig_params.max_trans_lat_mtos,
        cig_params.cis_cfgs.size(), cig_params.cis_cfgs.data(),
        base::BindOnce(&iso_impl::on_set_cig_params, base::Unretained(this),
        base::BindOnce(&iso_impl::on_set_cig_params, weak_factory_.GetWeakPtr(),
                       cig_id, cig_params.sdu_itv_mtos));
  }

  void on_remove_cig(uint8_t* stream, uint16_t len) {
    cig_remove_cmpl_evt evt;

    if (!iso_impl_initialized_) {
      LOG_WARN("iso_impl not available anymore");
      return;
    }

    LOG_ASSERT(cig_callbacks_ != nullptr) << "Invalid CIG callbacks";
    LOG_ASSERT(len == 2) << "Invalid packet length: " << +len;

@@ -268,7 +258,7 @@ struct iso_impl {
    }

    btsnd_hcic_remove_cig(cig_id, base::BindOnce(&iso_impl::on_remove_cig,
                                                 base::Unretained(this)));
                                                 weak_factory_.GetWeakPtr()));
    BTM_LogHistory(kBtmLogTag, RawAddress::kEmpty, "CIG Remove",
                   base::StringPrintf("cig_id:0x%02x (f:%d)", cig_id, force));
  }
@@ -278,11 +268,6 @@ struct iso_impl {
      uint16_t len) {
    uint8_t status;

    if (!iso_impl_initialized_) {
      LOG_WARN("iso_impl not available anymore");
      return;
    }

    LOG_ASSERT(len == 2) << "Invalid packet length: " << +len;

    STREAM_TO_UINT16(status, stream);
@@ -328,10 +313,10 @@ struct iso_impl {
                       base::StringPrintf("handle:0x%04x", el.acl_conn_handle));
      }
    }
    btsnd_hcic_create_cis(conn_params.conn_pairs.size(),
                          conn_params.conn_pairs.data(),
    btsnd_hcic_create_cis(
        conn_params.conn_pairs.size(), conn_params.conn_pairs.data(),
        base::BindOnce(&iso_impl::on_status_establish_cis,
                                         base::Unretained(this), conn_params));
                       weak_factory_.GetWeakPtr(), conn_params));
  }

  void disconnect_cis(uint16_t cis_handle, uint8_t reason) {
@@ -353,11 +338,6 @@ struct iso_impl {
    uint8_t status;
    uint16_t conn_handle;

    if (!iso_impl_initialized_) {
      LOG_WARN("iso_impl not available anymore");
      return;
    }

    STREAM_TO_UINT8(status, stream);
    STREAM_TO_UINT16(conn_handle, stream);

@@ -402,7 +382,7 @@ struct iso_impl {
        path_params.codec_id_vendor, path_params.controller_delay,
        std::move(path_params.codec_conf),
        base::BindOnce(&iso_impl::on_setup_iso_data_path,
                       base::Unretained(this)));
                       weak_factory_.GetWeakPtr()));
    BTM_LogHistory(
        kBtmLogTag, cis_hdl_to_addr[conn_handle], "Setup data path",
        base::StringPrintf(
@@ -422,11 +402,6 @@ struct iso_impl {
    STREAM_TO_UINT8(status, stream);
    STREAM_TO_UINT16(conn_handle, stream);

    if (!iso_impl_initialized_) {
      LOG_WARN("iso_impl not available anymore");
      return;
    }

    iso_base* iso = GetIsoIfKnown(conn_handle);
    if (iso == nullptr) {
      /* That could happen when ACL has been disconnected while removing data
@@ -462,7 +437,8 @@ struct iso_impl {
    btsnd_hcic_remove_iso_data_path(
        iso_handle, data_path_dir,
        base::BindOnce(&iso_impl::on_remove_iso_data_path,
                       base::Unretained(this)));
                       weak_factory_.GetWeakPtr()));

    BTM_LogHistory(kBtmLogTag, cis_hdl_to_addr[iso_handle], "Remove data path",
                   base::StringPrintf("handle:0x%04x, dir:0x%02x", iso_handle,
                                      data_path_dir));
@@ -495,10 +471,6 @@ struct iso_impl {

    STREAM_TO_UINT16(conn_handle, stream);

    if (!iso_impl_initialized_) {
      LOG_WARN("iso_impl not available anymore");
      return;
    }
    iso_base* iso = GetIsoIfKnown(conn_handle);
    if (iso == nullptr) {
      /* That could happen when ACL has been disconnected while waiting on the
@@ -531,7 +503,7 @@ struct iso_impl {

    btsnd_hcic_read_iso_link_quality(
        iso_handle, base::BindOnce(&iso_impl::on_iso_link_quality_read,
                                   base::Unretained(this)));
                                   weak_factory_.GetWeakPtr()));
  }

  BT_HDR* prepare_ts_hci_packet(uint16_t iso_handle, uint32_t ts,
@@ -657,11 +629,6 @@ struct iso_impl {
  }

  void disconnection_complete(uint16_t handle, uint8_t reason) {
    if (!iso_impl_initialized_) {
      LOG_WARN("iso_impl not available anymore");
      return;
    }

    /* Check if this is an ISO handle */
    auto cis = GetCisIfKnown(handle);
    if (cis == nullptr) return;
@@ -1051,6 +1018,7 @@ struct iso_impl {
  BigCallbacks* big_callbacks_ = nullptr;
  std::mutex on_iso_traffic_active_callbacks_list_mutex_;
  std::list<void (*)(bool)> on_iso_traffic_active_callbacks_list_;
  base::WeakPtrFactory<iso_impl> weak_factory_{this};
};

}  // namespace iso_manager
+296 −0
Original line number Diff line number Diff line
@@ -667,6 +667,41 @@ TEST_F(IsoManagerTest, CreateCigCallbackValid) {
                          std::vector<uint16_t>({0x0EFF, 0x00FF}).begin()));
}

TEST_F(IsoManagerTest, CreateCigLateArrivingCallback) {
  // Catch the callback
  base::OnceCallback<void(uint8_t*, uint16_t)> iso_cb;
  ON_CALL(hcic_interface_, SetCigParams)
      .WillByDefault([&](auto cig_id, auto,
                         base::OnceCallback<void(uint8_t*, uint16_t)> cb) {
        iso_cb = std::move(cb);
      });

  IsoManager::GetInstance()->CreateCig(
      volatile_test_cig_create_cmpl_evt_.cig_id, kDefaultCigParams);

  // Stop the IsoManager before calling the callback
  IsoManager::GetInstance()->Stop();

  // Call the callback and expect no call
  EXPECT_CALL(*cig_callbacks_, OnCigEvent(_, _)).Times(0);
  ASSERT_FALSE(iso_cb.is_null());

  uint8_t hci_mock_rsp_buffer[3 + sizeof(uint16_t) *
                                      volatile_test_cig_create_cmpl_evt_
                                          .conn_handles.size()];
  uint8_t* p = hci_mock_rsp_buffer;
  UINT8_TO_STREAM(p, volatile_test_cig_create_cmpl_evt_.status);
  UINT8_TO_STREAM(p, volatile_test_cig_create_cmpl_evt_.cig_id);
  UINT8_TO_STREAM(p, volatile_test_cig_create_cmpl_evt_.conn_handles.size());
  for (auto handle : volatile_test_cig_create_cmpl_evt_.conn_handles) {
    UINT16_TO_STREAM(p, handle);
  }
  std::move(iso_cb).Run(
      hci_mock_rsp_buffer,
      3 + sizeof(uint16_t) *
              volatile_test_cig_create_cmpl_evt_.conn_handles.size());
}

// Check if CIG reconfigure triggers HCI layer call
TEST_F(IsoManagerTest, ReconfigureCigHciCall) {
  IsoManager::GetInstance()->CreateCig(
@@ -780,6 +815,43 @@ TEST_F(IsoManagerTest, ReconfigureCigValid) {
      volatile_test_cig_create_cmpl_evt_.conn_handles.begin()));
}

TEST_F(IsoManagerTest, ReconfigureCigLateArrivingCallback) {
  IsoManager::GetInstance()->CreateCig(
      volatile_test_cig_create_cmpl_evt_.cig_id, kDefaultCigParams);

  // Catch the callback
  base::OnceCallback<void(uint8_t*, uint16_t)> iso_cb;
  ON_CALL(hcic_interface_, SetCigParams)
      .WillByDefault([&](auto cig_id, auto,
                         base::OnceCallback<void(uint8_t*, uint16_t)> cb) {
        iso_cb = std::move(cb);
      });
  IsoManager::GetInstance()->ReconfigureCig(
      volatile_test_cig_create_cmpl_evt_.cig_id, kDefaultCigParams2);

  // Stop the IsoManager before calling the callback
  IsoManager::GetInstance()->Stop();

  // Call the callback and expect no call
  EXPECT_CALL(*cig_callbacks_, OnCigEvent(_, _)).Times(0);
  ASSERT_FALSE(iso_cb.is_null());

  uint8_t hci_mock_rsp_buffer[3 + sizeof(uint16_t) *
                                      volatile_test_cig_create_cmpl_evt_
                                          .conn_handles.size()];
  uint8_t* p = hci_mock_rsp_buffer;
  UINT8_TO_STREAM(p, volatile_test_cig_create_cmpl_evt_.status);
  UINT8_TO_STREAM(p, volatile_test_cig_create_cmpl_evt_.cig_id);
  UINT8_TO_STREAM(p, volatile_test_cig_create_cmpl_evt_.conn_handles.size());
  for (auto handle : volatile_test_cig_create_cmpl_evt_.conn_handles) {
    UINT16_TO_STREAM(p, handle);
  }
  std::move(iso_cb).Run(
      hci_mock_rsp_buffer,
      3 + sizeof(uint16_t) *
              volatile_test_cig_create_cmpl_evt_.conn_handles.size());
}

TEST_F(IsoManagerTest, RemoveCigHciCall) {
  IsoManager::GetInstance()->CreateCig(
      volatile_test_cig_create_cmpl_evt_.cig_id, kDefaultCigParams);
@@ -1127,6 +1199,47 @@ TEST_F(IsoManagerTest, EstablishCisValid) {
  IsoManager::GetInstance()->EstablishCis(params);
}

TEST_F(IsoManagerTest, EstablishCisLateArrivingCallback) {
  IsoManager::GetInstance()->CreateCig(
      volatile_test_cig_create_cmpl_evt_.cig_id, kDefaultCigParams);

  // Catch the callback
  base::OnceCallback<void(uint8_t*, uint16_t)> iso_cb;
  EXT_CIS_CREATE_CFG cis_create_cfg;
  uint8_t cis_num = 0;
  ON_CALL(hcic_interface_, CreateCis)
      .WillByDefault([&](uint8_t num_cis, const EXT_CIS_CREATE_CFG* cis_cfg,
                         base::OnceCallback<void(uint8_t*, uint16_t)> cb) {
        cis_create_cfg = *cis_cfg;
        cis_num = num_cis;
        iso_cb = std::move(cb);
      });

  // Establish all CISes before setting up their data paths
  bluetooth::hci::iso_manager::cis_establish_params params;
  for (auto& handle : volatile_test_cig_create_cmpl_evt_.conn_handles) {
    params.conn_pairs.push_back({handle, 1});
  }
  IsoManager::GetInstance()->EstablishCis(params);

  // Stop the IsoManager before calling the callback
  IsoManager::GetInstance()->Stop();

  // Call the callback and expect no call
  EXPECT_CALL(
      *cig_callbacks_,
      OnCisEvent(bluetooth::hci::iso_manager::kIsoEventCisEstablishCmpl, _))
      .Times(0);
  ASSERT_FALSE(iso_cb.is_null());

  // Command complete with error will trigger the callback without
  // injecting any additional HCI events
  std::vector<uint8_t> buf(1);
  uint8_t* p = buf.data();
  UINT8_TO_STREAM(p, 0x01);  // status
  std::move(iso_cb).Run(buf.data(), buf.size());
}

TEST_F(IsoManagerTest, ReconnectCisValid) {
  IsoManager::GetInstance()->CreateCig(
      volatile_test_cig_create_cmpl_evt_.cig_id, kDefaultCigParams);
@@ -1697,6 +1810,55 @@ TEST_F(IsoManagerTest, SetupIsoDataPathInvalidStatus) {
  }
}

TEST_F(IsoManagerTest, SetupIsoDataPathLateArrivingCallback) {
  IsoManager::GetInstance()->CreateCig(
      volatile_test_cig_create_cmpl_evt_.cig_id, kDefaultCigParams);
  IsoManager::GetInstance()->CreateBig(volatile_test_big_params_evt_.big_id,
                                       kDefaultBigParams);

  // Establish all CISes before setting up their data paths
  bluetooth::hci::iso_manager::cis_establish_params params;
  for (auto& handle : volatile_test_cig_create_cmpl_evt_.conn_handles) {
    params.conn_pairs.push_back({handle, 1});
  }
  IsoManager::GetInstance()->EstablishCis(params);

  bluetooth::hci::iso_manager::iso_data_path_params path_params =
      kDefaultIsoDataPathParams;

  // Catch the callback
  base::OnceCallback<void(uint8_t*, uint16_t)> iso_cb;
  ON_CALL(hcic_interface_, SetupIsoDataPath)
      .WillByDefault(
          [&iso_cb](uint16_t iso_handle, uint8_t /* data_path_dir */,
                    uint8_t /* data_path_id */, uint8_t /* codec_id_format */,
                    uint16_t /* codec_id_company */,
                    uint16_t /* codec_id_vendor */,
                    uint32_t /* controller_delay */,
                    std::vector<uint8_t> /* codec_conf */,
                    base::OnceCallback<void(uint8_t*, uint16_t)> cb) {
            iso_cb = std::move(cb);
          });
  // Setup and remove data paths for all CISes
  path_params.data_path_dir =
      bluetooth::hci::iso_manager::kRemoveIsoDataPathDirectionInput;
  auto& handle = volatile_test_cig_create_cmpl_evt_.conn_handles[0];
  IsoManager::GetInstance()->SetupIsoDataPath(handle, path_params);

  // Stop the IsoManager before calling the callback
  IsoManager::GetInstance()->Stop();

  // Call the callback and expect no call
  EXPECT_CALL(*cig_callbacks_, OnSetupIsoDataPath(_, handle, _)).Times(0);
  ASSERT_FALSE(iso_cb.is_null());

  std::vector<uint8_t> buf(3);
  uint8_t* p = buf.data();
  UINT8_TO_STREAM(p, HCI_SUCCESS);
  UINT16_TO_STREAM(p, handle);
  std::move(iso_cb).Run(buf.data(), buf.size());
}

TEST_F(IsoManagerTest, RemoveIsoDataPathValid) {
  IsoManager::GetInstance()->CreateCig(
      volatile_test_cig_create_cmpl_evt_.cig_id, kDefaultCigParams);
@@ -1846,6 +2008,56 @@ TEST_F(IsoManagerTest, RemoveIsoDataPathInvalidStatus) {
      iso_handle, kDefaultIsoDataPathParams.data_path_dir);
}

TEST_F(IsoManagerTest, RemoveIsoDataPathLateArrivingCallback) {
  IsoManager::GetInstance()->CreateCig(
      volatile_test_cig_create_cmpl_evt_.cig_id, kDefaultCigParams);
  IsoManager::GetInstance()->CreateBig(volatile_test_big_params_evt_.big_id,
                                       kDefaultBigParams);

  // Establish all CISes before setting up their data paths
  bluetooth::hci::iso_manager::cis_establish_params params;
  for (auto& handle : volatile_test_cig_create_cmpl_evt_.conn_handles) {
    params.conn_pairs.push_back({handle, 1});
  }
  IsoManager::GetInstance()->EstablishCis(params);

  bluetooth::hci::iso_manager::iso_data_path_params path_params =
      kDefaultIsoDataPathParams;

  // Setup and remove data paths for all CISes
  path_params.data_path_dir =
      bluetooth::hci::iso_manager::kRemoveIsoDataPathDirectionInput;
  auto& handle = volatile_test_cig_create_cmpl_evt_.conn_handles[0];
  IsoManager::GetInstance()->SetupIsoDataPath(handle, path_params);

  // Catch the callback
  base::OnceCallback<void(uint8_t*, uint16_t)> iso_cb;
  ON_CALL(hcic_interface_, RemoveIsoDataPath)
      .WillByDefault(
          [&iso_cb](uint16_t iso_handle, uint8_t data_path_dir,
                    base::OnceCallback<void(uint8_t*, uint16_t)> cb) {
            iso_cb = std::move(cb);
          });
  IsoManager::GetInstance()->RemoveIsoDataPath(handle,
                                               path_params.data_path_dir);

  // Stop the IsoManager before calling the callback
  IsoManager::GetInstance()->Stop();

  // Call the callback and expect no call
  EXPECT_CALL(*cig_callbacks_,
              OnRemoveIsoDataPath(HCI_SUCCESS, handle,
                                  volatile_test_cig_create_cmpl_evt_.cig_id))
      .Times(0);
  ASSERT_FALSE(iso_cb.is_null());

  std::vector<uint8_t> buf(3);
  uint8_t* p = buf.data();
  UINT8_TO_STREAM(p, HCI_SUCCESS);
  UINT16_TO_STREAM(p, handle);
  std::move(iso_cb).Run(buf.data(), buf.size());
}

TEST_F(IsoManagerTest, SendIsoDataWithNoCigConnected) {
  std::vector<uint8_t> data_vec(108, 0);
  IsoManager::GetInstance()->CreateCig(
@@ -2250,6 +2462,28 @@ TEST_F(IsoManagerTest, HandleDisconnectDisconnectedCig) {
  IsoManager::GetInstance()->HandleDisconnect(handle, 16);
}

TEST_F(IsoManagerTest, HandleDisconnectLateArrivingCallback) {
  IsoManager::GetInstance()->CreateCig(
      volatile_test_cig_create_cmpl_evt_.cig_id, kDefaultCigParams);

  auto handle = volatile_test_cig_create_cmpl_evt_.conn_handles[0];
  IsoManager::GetInstance()->EstablishCis({{{handle, 1}}});

  EXPECT_CALL(*big_callbacks_, OnBigEvent).Times(0);
  EXPECT_CALL(*cig_callbacks_, OnCigEvent).Times(0);
  EXPECT_CALL(*cig_callbacks_, OnCisEvent).Times(0);

  // Expect disconnect event exactly once
  EXPECT_CALL(
      *cig_callbacks_,
      OnCisEvent(bluetooth::hci::iso_manager::kIsoEventCisDisconnected, _))
      .Times(0);

  // Expect no callback on late arriving event
  IsoManager::GetInstance()->Stop();
  IsoManager::GetInstance()->HandleDisconnect(handle, 16);
}

TEST_F(IsoManagerTest, HandleIsoData) {
  IsoManager::GetInstance()->CreateCig(
      volatile_test_cig_create_cmpl_evt_.cig_id, kDefaultCigParams);
@@ -2396,3 +2630,65 @@ TEST_F(IsoManagerTest, HandleIsoDataSameSeqNb) {
  IsoManager::GetInstance()->HandleIsoData(dummy_msg.data());
  IsoManager::GetInstance()->HandleIsoData(dummy_msg.data());
}

TEST_F(IsoManagerTest, ReadIsoLinkQualityLateArrivingCallback) {
  IsoManager::GetInstance()->CreateCig(
      volatile_test_cig_create_cmpl_evt_.cig_id, kDefaultCigParams);

  EXPECT_CALL(
      *cig_callbacks_,
      OnCisEvent(bluetooth::hci::iso_manager::kIsoEventCisEstablishCmpl, _))
      .Times(kDefaultCigParams.cis_cfgs.size())
      .WillRepeatedly([this](uint8_t type, void* data) {
        bluetooth::hci::iso_manager::cis_establish_cmpl_evt* evt =
            static_cast<bluetooth::hci::iso_manager::cis_establish_cmpl_evt*>(
                data);

        ASSERT_EQ(evt->status, HCI_SUCCESS);
        ASSERT_TRUE(
            std::find(volatile_test_cig_create_cmpl_evt_.conn_handles.begin(),
                      volatile_test_cig_create_cmpl_evt_.conn_handles.end(),
                      evt->cis_conn_hdl) !=
            volatile_test_cig_create_cmpl_evt_.conn_handles.end());
      });

  // Establish all CISes
  bluetooth::hci::iso_manager::cis_establish_params params;
  for (auto& handle : volatile_test_cig_create_cmpl_evt_.conn_handles) {
    params.conn_pairs.push_back({handle, 1});
  }
  IsoManager::GetInstance()->EstablishCis(params);

  // Catch the callback
  base::OnceCallback<void(uint8_t*, uint16_t)> iso_cb;
  ON_CALL(hcic_interface_, ReadIsoLinkQuality)
      .WillByDefault(
          [&iso_cb](uint16_t iso_handle,
                    base::OnceCallback<void(uint8_t*, uint16_t)> cb) {
            iso_cb = std::move(cb);
          });
  auto handle = volatile_test_cig_create_cmpl_evt_.conn_handles[0];
  IsoManager::GetInstance()->ReadIsoLinkQuality(handle);

  // Stop the IsoManager before calling the callback
  IsoManager::GetInstance()->Stop();

  // Call the callback and expect no call
  EXPECT_CALL(*cig_callbacks_,
              OnIsoLinkQualityRead(handle, _, _, _, _, _, _, _, _))
      .Times(0);
  ASSERT_FALSE(iso_cb.is_null());

  std::vector<uint8_t> buf(31);
  uint8_t* p = buf.data();
  UINT8_TO_STREAM(p, HCI_SUCCESS);
  UINT16_TO_STREAM(p, handle);
  UINT32_TO_STREAM(p, 0);
  UINT32_TO_STREAM(p, 0);
  UINT32_TO_STREAM(p, 0);
  UINT32_TO_STREAM(p, 0);
  UINT32_TO_STREAM(p, 0);
  UINT32_TO_STREAM(p, 0);
  UINT32_TO_STREAM(p, 0);
  std::move(iso_cb).Run(buf.data(), buf.size());
}