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

Commit 1bdbe93c authored by Jakub Pawlowski's avatar Jakub Pawlowski
Browse files

Make AdvertiseManager shutdown gracefully 

If the Bluetooth stack is shutdown while AdvertiseManager has a pending HCI
operation being executed, the callback to that operation might be
executed after the manager was deleted. In order to fix any potential
issues, always use a weak pointer to the advertisement manager. This will
prevent the callbacks from getting executed.

In case of callbacks that are not executed on the AdvertiseManager
instance, one can check if the weak pointer is still valid, and properly
handle the stack shutdown.

Test: BleAdvertisingManagerTest:test_cleanup_during_execution
Change-Id: I8d33de4f64267a880880082c5498375c083addef
parent e155d2af

system/stack/btm/btm_ble_multi_adv.cc

+124 −43
Original line number Diff line number Diff line
@@ -20,6 +20,7 @@ 
#include <base/bind.h>

#include <base/location.h>

#include <base/logging.h>

#include <base/memory/weak_ptr.h>

#include <base/strings/string_number_conversions.h>

#include <base/time/time.h>

#include <string.h>
@@ -139,7 +140,7 @@ class BleAdvertisingManagerImpl; 
/* a temporary type for holding all the data needed in callbacks below*/

struct CreatorParams {

  uint8_t inst_id;

  BleAdvertisingManagerImpl* self;

  base::WeakPtr<BleAdvertisingManagerImpl> self;

  IdTxPowerStatusCb cb;

  tBTM_BLE_ADV_PARAMS params;

  std::vector<uint8_t> advertise_data;
@@ -157,11 +158,11 @@ class BleAdvertisingManagerImpl 
    : public BleAdvertisingManager,

      public BleAdvertiserHciInterface::AdvertisingEventObserver {

 public:

  BleAdvertisingManagerImpl(BleAdvertiserHciInterface* interface) {

    this->hci_interface = interface;

  BleAdvertisingManagerImpl(BleAdvertiserHciInterface* interface)

      : hci_interface(interface), weak_factory_(this) {

    hci_interface->ReadInstanceCount(

        base::Bind(&BleAdvertisingManagerImpl::ReadInstanceCountCb,

                   base::Unretained(this)));

                   weak_factory_.GetWeakPtr()));

  }



  ~BleAdvertisingManagerImpl() { adv_inst.clear(); }
@@ -210,7 +211,7 @@ class BleAdvertisingManagerImpl 
  void GenerateRpa(base::Callback<void(RawAddress)> cb) {

    btm_gen_resolvable_private_addr(

        Bind(&BleAdvertisingManagerImpl::OnRpaGenerationComplete,

             base::Unretained(this), std::move(cb)));

             weak_factory_.GetWeakPtr(), std::move(cb)));

  }



  void ConfigureRpa(AdvertisingInstance* p_inst, MultiAdvCb configuredCb) {
@@ -308,7 +309,7 @@ class BleAdvertisingManagerImpl 
    /* a temporary type for holding all the data needed in callbacks below*/

    struct CreatorParams {

      uint8_t inst_id;

      BleAdvertisingManagerImpl* self;

      base::WeakPtr<BleAdvertisingManagerImpl> self;

      MultiAdvCb cb;

      tBTM_BLE_ADV_PARAMS params;

      std::vector<uint8_t> advertise_data;
@@ -320,7 +321,7 @@ class BleAdvertisingManagerImpl 
    std::unique_ptr<CreatorParams> c;

    c.reset(new CreatorParams());



    c->self = this;

    c->self = weak_factory_.GetWeakPtr();

    c->cb = std::move(cb);

    c->params = *params;

    c->advertise_data = std::move(advertise_data);
@@ -336,7 +337,12 @@ class BleAdvertisingManagerImpl 
    // clang-format off

    c->self->SetParameters(c->inst_id, &c->params, Bind(

      [](c_type c, uint8_t status, int8_t tx_power) {

        if (status != 0) {

        if (!c->self) {

          LOG(INFO) << "Stack was shut down";

          return;

        }



        if (status) {

          LOG(ERROR) << "setting parameters failed, status: " << +status;

          c->cb.Run(status);

          return;
@@ -347,6 +353,11 @@ class BleAdvertisingManagerImpl 
        const RawAddress& rpa = c->self->adv_inst[c->inst_id].own_address;

        c->self->GetHciInterface()->SetRandomAddress(c->inst_id, rpa, Bind(

          [](c_type c, uint8_t status) {

            if (!c->self) {

              LOG(INFO) << "Stack was shut down";

              return;

            }



            if (status != 0) {

              LOG(ERROR) << "setting random address failed, status: " << +status;

              c->cb.Run(status);
@@ -355,6 +366,11 @@ class BleAdvertisingManagerImpl 


            c->self->SetData(c->inst_id, false, std::move(c->advertise_data), Bind(

              [](c_type c, uint8_t status) {

                if (!c->self) {

                  LOG(INFO) << "Stack was shut down";

                  return;

                }



                if (status != 0) {

                  LOG(ERROR) << "setting advertise data failed, status: " << +status;

                  c->cb.Run(status);
@@ -363,6 +379,11 @@ class BleAdvertisingManagerImpl 


                c->self->SetData(c->inst_id, true, std::move(c->scan_response_data), Bind(

                  [](c_type c, uint8_t status) {

                    if (!c->self) {

                      LOG(INFO) << "Stack was shut down";

                      return;

                    }



                    if (status != 0) {

                      LOG(ERROR) << "setting scan response data failed, status: " << +status;

                      c->cb.Run(status);
@@ -388,7 +409,7 @@ class BleAdvertisingManagerImpl 
    std::unique_ptr<CreatorParams> c;

    c.reset(new CreatorParams());



    c->self = this;

    c->self = weak_factory_.GetWeakPtr();

    c->cb = std::move(cb);

    c->params = *params;

    c->advertise_data = std::move(advertise_data);
@@ -404,8 +425,13 @@ class BleAdvertisingManagerImpl 
    // clang-format off

    c->self->RegisterAdvertiser(Bind(

      [](c_type c, uint8_t advertiser_id, uint8_t status) {

        if (!c->self) {

          LOG(INFO) << "Stack was shut down";

          return;

        }



        if (status != 0) {

          LOG(ERROR) << "registering advertiser failed, status: " << +status;

          LOG(ERROR) << " failed, status: " << +status;

          c->cb.Run(0, 0, status);

          return;

        }
@@ -414,6 +440,11 @@ class BleAdvertisingManagerImpl 


        c->self->SetParameters(c->inst_id, &c->params, Bind(

          [](c_type c, uint8_t status, int8_t tx_power) {

            if (!c->self) {

              LOG(INFO) << "Stack was shut down";

              return;

            }



            if (status != 0) {

              c->self->Unregister(c->inst_id);

              LOG(ERROR) << "setting parameters failed, status: " << +status;
@@ -432,6 +463,11 @@ class BleAdvertisingManagerImpl 
            const RawAddress& rpa = c->self->adv_inst[c->inst_id].own_address;

            c->self->GetHciInterface()->SetRandomAddress(c->inst_id, rpa, Bind(

              [](c_type c, uint8_t status) {

                if (!c->self) {

                  LOG(INFO) << "Stack was shut down";

                  return;

                }



                if (status != 0) {

                  c->self->Unregister(c->inst_id);

                  LOG(ERROR) << "setting random address failed, status: " << +status;
@@ -447,31 +483,51 @@ class BleAdvertisingManagerImpl 
  }



  void StartAdvertisingSetAfterAddressPart(c_type c) {

    c->self->SetData(c->inst_id, false, std::move(c->advertise_data), Bind(

    c->self->SetData(

        c->inst_id, false, std::move(c->advertise_data),

        Bind(

            [](c_type c, uint8_t status) {

              if (!c->self) {

                LOG(INFO) << "Stack was shut down";

                return;

              }



              if (status != 0) {

                c->self->Unregister(c->inst_id);

          LOG(ERROR) << "setting advertise data failed, status: " << +status;

                LOG(ERROR) << "setting advertise data failed, status: "

                           << +status;

                c->cb.Run(0, 0, status);

                return;

              }



        c->self->SetData(c->inst_id, true, std::move(c->scan_response_data), Bind(

              c->self->SetData(

                  c->inst_id, true, std::move(c->scan_response_data),

                  Bind(

                      [](c_type c, uint8_t status) {

                        if (!c->self) {

                          LOG(INFO) << "Stack was shut down";

                          return;

                        }



                        if (status != 0) {

                          c->self->Unregister(c->inst_id);

              LOG(ERROR) << "setting scan response data failed, status: " << +status;

                          LOG(ERROR)

                              << "setting scan response data failed, status: "

                              << +status;

                          c->cb.Run(0, 0, status);

                          return;

                        }



                        if (c->periodic_params.enable) {

              c->self->StartAdvertisingSetPeriodicPart(std::move(c));

                          c->self->StartAdvertisingSetPeriodicPart(

                              std::move(c));

                        } else {

                          c->self->StartAdvertisingSetFinish(std::move(c));

                        }

        }, base::Passed(&c)));

    }, base::Passed(&c)));

                      },

                      base::Passed(&c)));

            },

            base::Passed(&c)));

  }



  void StartAdvertisingSetPeriodicPart(c_type c) {
@@ -480,6 +536,11 @@ class BleAdvertisingManagerImpl 
    // clang-format off

    c->self->SetPeriodicAdvertisingParameters(c->inst_id, &c->periodic_params, Bind(

      [](c_type c, uint8_t status) {

        if (!c->self) {

          LOG(INFO) << "Stack was shut down";

          return;

        }



        if (status != 0) {

          c->self->Unregister(c->inst_id);

          LOG(ERROR) << "setting periodic parameters failed, status: " << +status;
@@ -489,6 +550,11 @@ class BleAdvertisingManagerImpl 


        c->self->SetPeriodicAdvertisingData(c->inst_id, std::move(c->periodic_data), Bind(

          [](c_type c, uint8_t status) {

            if (!c->self) {

              LOG(INFO) << "Stack was shut down";

              return;

            }



            if (status != 0) {

              c->self->Unregister(c->inst_id);

              LOG(ERROR) << "setting periodic parameters failed, status: " << +status;
@@ -498,6 +564,11 @@ class BleAdvertisingManagerImpl 


            c->self->SetPeriodicAdvertisingEnable(c->inst_id, true, Bind(

              [](c_type c, uint8_t status) {

                if (!c->self) {

                  LOG(INFO) << "Stack was shut down";

                  return;

                }



                if (status != 0) {

                  c->self->Unregister(c->inst_id);

                  LOG(ERROR) << "enabling periodic advertising failed, status: " << +status;
@@ -518,9 +589,14 @@ class BleAdvertisingManagerImpl 
    uint16_t duration = c->duration;

    uint8_t maxExtAdvEvents = c->maxExtAdvEvents;

    RegisterCb timeout_cb = std::move(c->timeout_cb);

    BleAdvertisingManagerImpl* self = c->self;

    base::WeakPtr<BleAdvertisingManagerImpl> self = c->self;

    MultiAdvCb enable_cb = Bind(

        [](c_type c, uint8_t status) {

          if (!c->self) {

            LOG(INFO) << "Stack was shut down";

            return;

          }



          if (status != 0) {

            c->self->Unregister(c->inst_id);

            LOG(ERROR) << "enabling advertiser failed, status: " << +status;
@@ -546,9 +622,9 @@ class BleAdvertisingManagerImpl 


    p_inst->timeout_timer = alarm_new("btm_ble.adv_timeout");



    base::Closure cb = Bind(&BleAdvertisingManagerImpl::Enable,

                            base::Unretained(this), inst_id, 0 /* disable */,

                            std::move(timeout_cb), 0, 0, base::Bind(DoNothing));

    base::Closure cb = Bind(

        &BleAdvertisingManagerImpl::Enable, weak_factory_.GetWeakPtr(), inst_id,

        0 /* disable */, std::move(timeout_cb), 0, 0, base::Bind(DoNothing));



    // schedule disable when the timeout passes

    alarm_set_closure(FROM_HERE, p_inst->timeout_timer, duration * 10,
@@ -581,8 +657,8 @@ class BleAdvertisingManagerImpl 
    if (enable && p_inst->address_update_required) {

      p_inst->address_update_required = false;

      ConfigureRpa(p_inst, base::Bind(&BleAdvertisingManagerImpl::EnableFinish,

                                      base::Unretained(this), p_inst, enable,

                                      std::move(cb)));

                                      weak_factory_.GetWeakPtr(), p_inst,

                                      enable, std::move(cb)));

      return;

    }
@@ -596,7 +672,7 @@ class BleAdvertisingManagerImpl 
      // TODO(jpawlowski): HCI implementation that can't do duration should

      // emulate it, not EnableWithTimerCb.

      myCb = Bind(&BleAdvertisingManagerImpl::EnableWithTimerCb,

                  base::Unretained(this), p_inst->inst_id, std::move(cb),

                  weak_factory_.GetWeakPtr(), p_inst->inst_id, std::move(cb),

                  p_inst->duration, p_inst->timeout_cb);

    } else {

      myCb = std::move(cb);
@@ -694,7 +770,7 @@ class BleAdvertisingManagerImpl 
    DivideAndSendData(

        inst_id, data, cb,

        base::Bind(&BleAdvertisingManagerImpl::SetDataAdvDataSender,

                   base::Unretained(this), is_scan_rsp));

                   weak_factory_.GetWeakPtr(), is_scan_rsp));

  }



  void SetDataAdvDataSender(uint8_t is_scan_rsp, uint8_t inst_id,
@@ -932,6 +1008,11 @@ class BleAdvertisingManagerImpl 
  BleAdvertiserHciInterface* hci_interface = nullptr;

  std::vector<AdvertisingInstance> adv_inst;

  uint8_t inst_count;



  // Member variables should appear before the WeakPtrFactory, to ensure

  // that any WeakPtrs are invalidated before its members

  // variable's destructors are executed, rendering them invalid.

  base::WeakPtrFactory<BleAdvertisingManagerImpl> weak_factory_;

};



BleAdvertisingManager* instance;

system/stack/test/ble_advertiser_test.cc

+47 −0
Original line number Diff line number Diff line
@@ -1064,6 +1064,53 @@ TEST_F(BleAdvertisingManagerTest, test_duration_update_during_timeout) { 
  remove_cb.Run(0);

}



/* This test verifies that stack cleanup, and shutdown happening while there is

 * outstanding HCI command is not triggering the callback */

TEST_F(BleAdvertisingManagerTest, test_cleanup_during_execution) {

  std::vector<uint8_t> adv_data;

  std::vector<uint8_t> scan_resp;

  tBTM_BLE_ADV_PARAMS params;

  tBLE_PERIODIC_ADV_PARAMS periodic_params;

  periodic_params.enable = false;

  std::vector<uint8_t> periodic_data;



  parameters_cb set_params_cb;

  status_cb set_address_cb;

  status_cb set_data_cb;

  EXPECT_CALL(*hci_mock, SetParameters1(_, _, _, _, _, _, _, _, _)).Times(1);

  EXPECT_CALL(*hci_mock, SetParameters2(_, _, _, _, _, _, _, _))

      .Times(1)

      .WillOnce(SaveArg<7>(&set_params_cb));

  EXPECT_CALL(*hci_mock, SetRandomAddress(_, _, _))

      .Times(1)

      .WillOnce(SaveArg<2>(&set_address_cb));

  EXPECT_CALL(*hci_mock, SetAdvertisingData(_, _, _, _, _, _))

      .Times(1)

      .WillOnce(SaveArg<5>(&set_data_cb));



  BleAdvertisingManager::Get()->StartAdvertisingSet(

      Bind(&BleAdvertisingManagerTest::StartAdvertisingSetCb,

           base::Unretained(this)),

      &params, adv_data, scan_resp, &periodic_params, periodic_data,

      0 /* duration */, 0 /* maxExtAdvEvents */, Bind(DoNothing2));



  // we are a truly gracious fake controller, let the commands succeed!

  int selected_tx_power = -15;

  set_params_cb.Run(0, selected_tx_power);

  set_address_cb.Run(0);



  // Someone shut down the stack in the middle of flow, when the HCI Set

  // Advertise Data was scheduled!

  BleAdvertisingManager::Get()->CleanUp();



  // The HCI call returns with status, and tries to execute the callback. This

  // should just silently drop the call. If it got executed, we would get crash,

  // because BleAdvertisingManager object was already deleted.

  set_data_cb.Run(0);



  ::testing::Mock::VerifyAndClearExpectations(hci_mock.get());

}



extern void testRecomputeTimeout1();

extern void testRecomputeTimeout2();

extern void testRecomputeTimeout3();