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

Commit 200b8c85 authored by Łukasz Rymanowski's avatar Łukasz Rymanowski
Browse files

btif: Fix possible races on bluetooth off 

Normally it is Java layer having access to btif interface, where calles
are binded to the blueototh mainthread.
However, some calls e.g. RemoveDevice are called from the bluetooth mainthread.
This happens when device got unbonded.
If we are unluckly, if during mainthread operation, there is Cleanup
scheduled due to Bluetooth being turning OFF, it might happen that
RemoveDevice operation will be scheduled, but when it start to execute,
services instances are already gone. This will lead to crash.

This patch add simple mechism to prevent that.

Bug: 226276953
Test: manual testing
Tag: #feature
Change-Id: Ib226e851981dadcc5b0091a1ac1428adcb7302a2
parent bc315587

system/bta/csis/csis_client.cc

+0 −1
Original line number Diff line number Diff line
@@ -273,7 +273,6 @@ class CsisClientImpl : public CsisClient { 
    Disconnect(addr);



    dev_groups_->RemoveDevice(addr);

    btif_storage_remove_csis_device(addr);

  }



  int GetGroupId(const RawAddress& addr, Uuid uuid) override {

system/bta/csis/csis_client_test.cc

+2 −1
Original line number Diff line number Diff line
@@ -1047,7 +1047,8 @@ TEST_F(CsisClientTest, test_storage_calls) { 


  ASSERT_EQ(0, get_func_call_count("btif_storage_remove_csis_device"));

  CsisClient::Get()->RemoveDevice(test_address);

  ASSERT_EQ(1, get_func_call_count("btif_storage_remove_csis_device"));

  /* It is 0 because btif_csis_client.cc calls that */

  ASSERT_EQ(0, get_func_call_count("btif_storage_remove_csis_device"));



  TestAppUnregister();

}

system/btif/src/btif_csis_client.cc

+47 −1
Original line number Diff line number Diff line
@@ -40,6 +40,8 @@ using bluetooth::csis::CsisClient; 


namespace {

std::unique_ptr<CsisClientInterface> csis_client_instance;

std::atomic_bool initialized = false;



class CsipSetCoordinatorServiceInterfaceImpl : public CsisClientInterface,

                                               public CsisClientCallbacks {

  ~CsipSetCoordinatorServiceInterfaceImpl() override = default;
@@ -53,36 +55,80 @@ class CsipSetCoordinatorServiceInterfaceImpl : public CsisClientInterface, 
        Bind(&CsisClient::Initialize, this,

             jni_thread_wrapper(FROM_HERE,

                                Bind(&btif_storage_load_bonded_csis_devices))));

    /* It might be not yet initialized, but setting this flag here is safe,

     * because other calls will check this and the native instance

     */

    initialized = true;

  }



  void Connect(const RawAddress& addr) override {

    if (!initialized || CsisClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    DVLOG(2) << __func__ << " addr: " << ADDRESS_TO_LOGGABLE_STR(addr);

    do_in_main_thread(FROM_HERE, Bind(&CsisClient::Connect,

                                      Unretained(CsisClient::Get()), addr));

  }



  void Disconnect(const RawAddress& addr) override {

    if (!initialized || CsisClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    DVLOG(2) << __func__ << " addr: " << ADDRESS_TO_LOGGABLE_STR(addr);

    do_in_main_thread(FROM_HERE, Bind(&CsisClient::Disconnect,

                                      Unretained(CsisClient::Get()), addr));

  }



  void RemoveDevice(const RawAddress& addr) override {

    if (!initialized || CsisClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not ready";



      /* Clear storage */

      do_in_jni_thread(FROM_HERE, Bind(&btif_storage_remove_csis_device, addr));

      return;

    }



    DVLOG(2) << __func__ << " addr: " << ADDRESS_TO_LOGGABLE_STR(addr);

    do_in_main_thread(FROM_HERE, Bind(&CsisClient::RemoveDevice,

                                      Unretained(CsisClient::Get()), addr));

    /* Clear storage */

    do_in_jni_thread(FROM_HERE, Bind(&btif_storage_remove_csis_device, addr));

  }



  void LockGroup(int group_id, bool lock) override {

    DVLOG(2) << __func__ << " group id: " << group_id << " lock: " << lock;

    if (!initialized || CsisClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    DVLOG(2) << __func__ << " group id: " << group_id << " lock: " << lock;

    do_in_main_thread(

        FROM_HERE, Bind(&CsisClient::LockGroup, Unretained(CsisClient::Get()),

                        group_id, lock, base::DoNothing()));

  }



  void Cleanup(void) override {

    if (!initialized || CsisClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    DVLOG(2) << __func__;

    initialized = false;

    do_in_main_thread(FROM_HERE, Bind(&CsisClient::CleanUp));

  }

system/btif/src/btif_le_audio.cc

+83 −0
Original line number Diff line number Diff line
@@ -38,6 +38,7 @@ using bluetooth::le_audio::LeAudioClientInterface; 
namespace {

class LeAudioClientInterfaceImpl;

std::unique_ptr<LeAudioClientInterface> leAudioInstance;

std::atomic_bool initialized = false;



class LeAudioClientInterfaceImpl : public LeAudioClientInterface,

                                   public LeAudioClientCallbacks {
@@ -123,10 +124,24 @@ class LeAudioClientInterfaceImpl : public LeAudioClientInterface, 
                          return LeAudioHalVerifier::SupportsLeAudio();

                        }),

                        offloading_preference));



    /* It might be not yet initialized, but setting this flag here is safe,

     * because other calls will check this and the native instance

     */

    initialized = true;

  }



  void Cleanup(void) override {

    DVLOG(2) << __func__;

    if (!initialized || LeAudioClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    initialized = false;



    do_in_main_thread(

        FROM_HERE,

        Bind(&LeAudioClient::Cleanup,
@@ -137,6 +152,16 @@ class LeAudioClientInterfaceImpl : public LeAudioClientInterface, 


  void RemoveDevice(const RawAddress& address) override {

    DVLOG(2) << __func__ << " address: " << ADDRESS_TO_LOGGABLE_STR(address);



    if (!initialized || LeAudioClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";



      do_in_jni_thread(FROM_HERE, Bind(&btif_storage_remove_leaudio, address));

      return;

    }



    do_in_main_thread(FROM_HERE,

                      Bind(&LeAudioClient::RemoveDevice,

                           Unretained(LeAudioClient::Get()), address));
@@ -146,6 +171,14 @@ class LeAudioClientInterfaceImpl : public LeAudioClientInterface, 


  void Connect(const RawAddress& address) override {

    DVLOG(2) << __func__ << " address: " << ADDRESS_TO_LOGGABLE_STR(address);



    if (!initialized || LeAudioClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(FROM_HERE,

                      Bind(&LeAudioClient::Connect,

                           Unretained(LeAudioClient::Get()), address));
@@ -153,6 +186,14 @@ class LeAudioClientInterfaceImpl : public LeAudioClientInterface, 


  void Disconnect(const RawAddress& address) override {

    DVLOG(2) << __func__ << " address: " << ADDRESS_TO_LOGGABLE_STR(address);



    if (!initialized || LeAudioClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(FROM_HERE,

                      Bind(&LeAudioClient::Disconnect,

                           Unretained(LeAudioClient::Get()), address));
@@ -161,6 +202,14 @@ class LeAudioClientInterfaceImpl : public LeAudioClientInterface, 
  void GroupAddNode(const int group_id, const RawAddress& address) override {

    DVLOG(2) << __func__ << " group_id: " << group_id

             << " address: " << ADDRESS_TO_LOGGABLE_STR(address);



    if (!initialized || LeAudioClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(

        FROM_HERE, Bind(&LeAudioClient::GroupAddNode,

                        Unretained(LeAudioClient::Get()), group_id, address));
@@ -169,6 +218,13 @@ class LeAudioClientInterfaceImpl : public LeAudioClientInterface, 
  void GroupRemoveNode(const int group_id, const RawAddress& address) override {

    DVLOG(2) << __func__ << " group_id: " << group_id

             << " address: " << ADDRESS_TO_LOGGABLE_STR(address);

    if (!initialized || LeAudioClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(

        FROM_HERE, Bind(&LeAudioClient::GroupRemoveNode,

                        Unretained(LeAudioClient::Get()), group_id, address));
@@ -176,6 +232,13 @@ class LeAudioClientInterfaceImpl : public LeAudioClientInterface, 


  void GroupSetActive(const int group_id) override {

    DVLOG(2) << __func__ << " group_id: " << group_id;

    if (!initialized || LeAudioClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(FROM_HERE,

                      Bind(&LeAudioClient::GroupSetActive,

                           Unretained(LeAudioClient::Get()), group_id));
@@ -185,6 +248,12 @@ class LeAudioClientInterfaceImpl : public LeAudioClientInterface, 
                                btle_audio_codec_config_t input_codec_config,

                                btle_audio_codec_config_t output_codec_config) {

    DVLOG(2) << __func__ << " group_id: " << group_id;

    if (!initialized || LeAudioClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }

    do_in_main_thread(FROM_HERE,

                      Bind(&LeAudioClient::SetCodecConfigPreference,

                           Unretained(LeAudioClient::Get()), group_id,
@@ -194,6 +263,13 @@ class LeAudioClientInterfaceImpl : public LeAudioClientInterface, 
  void SetCcidInformation(int ccid, int context_type) {

    DVLOG(2) << __func__ << " ccid: " << ccid << " context_type"

             << context_type;

    if (!initialized || LeAudioClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(

        FROM_HERE, Bind(&LeAudioClient::SetCcidInformation,

                        Unretained(LeAudioClient::Get()), ccid, context_type));
@@ -201,6 +277,13 @@ class LeAudioClientInterfaceImpl : public LeAudioClientInterface, 


  void SetInCall(bool in_call) {

    DVLOG(2) << __func__ << " in_call: " << in_call;

    if (!initialized || LeAudioClient::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(FROM_HERE,

                      Bind(&LeAudioClient::SetInCall,

                           Unretained(LeAudioClient::Get()), in_call));

system/btif/src/btif_vc.cc

+105 −0
Original line number Diff line number Diff line
@@ -36,6 +36,7 @@ using bluetooth::vc::VolumeControlInterface; 


namespace {

std::unique_ptr<VolumeControlInterface> vc_instance;

std::atomic_bool initialized = false;



class VolumeControlInterfaceImpl : public VolumeControlInterface,

                                   public VolumeControlCallbacks {
@@ -45,6 +46,11 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 
    DVLOG(2) << __func__;

    this->callbacks_ = callbacks;

    do_in_main_thread(FROM_HERE, Bind(&VolumeControl::Initialize, this));



    /* It might be not yet initialized, but setting this flag here is safe,

     * because other calls will check this and the native instance

     */

    initialized = true;

  }



  void OnConnectionState(ConnectionState state,
@@ -123,6 +129,14 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 


  void Connect(const RawAddress& address) override {

    DVLOG(2) << __func__ << " address: " << address;



    if (!initialized || VolumeControl::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(FROM_HERE,

                      Bind(&VolumeControl::Connect,

                           Unretained(VolumeControl::Get()), address));
@@ -130,6 +144,13 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 


  void Disconnect(const RawAddress& address) override {

    DVLOG(2) << __func__ << " address: " << address;



    if (!initialized || VolumeControl::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }

    do_in_main_thread(FROM_HERE,

                      Bind(&VolumeControl::Disconnect,

                           Unretained(VolumeControl::Get()), address));
@@ -138,6 +159,14 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 
  void SetVolume(std::variant<RawAddress, int> addr_or_group_id,

                 uint8_t volume) override {

    DVLOG(2) << __func__ << " volume: " << volume;



    if (!initialized || VolumeControl::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(FROM_HERE, Bind(&VolumeControl::SetVolume,

                                      Unretained(VolumeControl::Get()),

                                      std::move(addr_or_group_id), volume));
@@ -145,6 +174,14 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 


  void Mute(std::variant<RawAddress, int> addr_or_group_id) override {

    DVLOG(2) << __func__;



    if (!initialized || VolumeControl::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(

        FROM_HERE, Bind(&VolumeControl::Mute, Unretained(VolumeControl::Get()),

                        std::move(addr_or_group_id)));
@@ -152,6 +189,14 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 


  void Unmute(std::variant<RawAddress, int> addr_or_group_id) override {

    DVLOG(2) << __func__;



    if (!initialized || VolumeControl::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(FROM_HERE, Bind(&VolumeControl::UnMute,

                                      Unretained(VolumeControl::Get()),

                                      std::move(addr_or_group_id)));
@@ -160,6 +205,13 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 
  void RemoveDevice(const RawAddress& address) override {

    DVLOG(2) << __func__ << " address: " << address;



    if (!initialized || VolumeControl::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    /* RemoveDevice can be called on devices that don't have HA enabled */

    if (VolumeControl::IsVolumeControlRunning()) {

      do_in_main_thread(FROM_HERE,
@@ -174,6 +226,14 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 
                                  uint8_t ext_output_id) override {

    DVLOG(2) << __func__ << " address: " << address

             << "ext_output_id:" << ext_output_id;



    if (!initialized || VolumeControl::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(

        FROM_HERE,

        Bind(&VolumeControl::GetExtAudioOutVolumeOffset,
@@ -186,6 +246,14 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 
    DVLOG(2) << __func__ << " address: " << address

             << "ext_output_id:" << ext_output_id

             << "ext_output_id:" << offset_val;



    if (!initialized || VolumeControl::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(FROM_HERE,

                      Bind(&VolumeControl::SetExtAudioOutVolumeOffset,

                           Unretained(VolumeControl::Get()), address,
@@ -197,6 +265,13 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 
    DVLOG(2) << __func__ << " address: " << address

             << "ext_output_id:" << ext_output_id;



    if (!initialized || VolumeControl::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(FROM_HERE, Bind(&VolumeControl::GetExtAudioOutLocation,

                                      Unretained(VolumeControl::Get()), address,

                                      ext_output_id));
@@ -207,6 +282,14 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 
    DVLOG(2) << __func__ << " address: " << address

             << "ext_output_id:" << ext_output_id

             << "location:" << loghex(location);



    if (!initialized || VolumeControl::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(FROM_HERE, Bind(&VolumeControl::SetExtAudioOutLocation,

                                      Unretained(VolumeControl::Get()), address,

                                      ext_output_id, location));
@@ -217,6 +300,13 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 
    DVLOG(2) << __func__ << " address: " << address

             << "ext_output_id:" << ext_output_id;



    if (!initialized || VolumeControl::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(FROM_HERE, Bind(&VolumeControl::GetExtAudioOutDescription,

                                      Unretained(VolumeControl::Get()), address,

                                      ext_output_id));
@@ -228,6 +318,13 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 
    DVLOG(2) << __func__ << " address: " << address

             << "ext_output_id:" << ext_output_id << "description:" << descr;



    if (!initialized || VolumeControl::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    do_in_main_thread(FROM_HERE, Bind(&VolumeControl::SetExtAudioOutDescription,

                                      Unretained(VolumeControl::Get()), address,

                                      ext_output_id, descr));
@@ -235,6 +332,14 @@ class VolumeControlInterfaceImpl : public VolumeControlInterface, 


  void Cleanup(void) override {

    DVLOG(2) << __func__;

    if (!initialized || VolumeControl::Get() == nullptr) {

      DVLOG(2) << __func__

               << " call ignored, due to already started cleanup procedure or "

                  "service being not read";

      return;

    }



    initialized = false;

    do_in_main_thread(FROM_HERE, Bind(&VolumeControl::CleanUp));

  }