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

Commit b4937607 authored by Ajay Panicker's avatar Ajay Panicker Committed by Hansong Zhang
Browse files

Use weak pointers to prevent callbacks from executing after cleanup 

Switch from using the instance directly to using weak pointers to the
instance. This causes any base::Callback objects that are bound to that
weak instance to be cleaned up if the weak pointer becomes invalid.

Bug: 78134184
Test: valgrind ./net_test_avrcp
--gtest_filter=*disconnectAfterCleanupTest* and see that there is an
invalid read before the fix and it is gone after.

Change-Id: I46c3175e62b87be165e468d014155f8d8154c08a
parent 6d6ce429

system/profile/avrcp/connection_handler.cc

+4 −2
Original line number Diff line number Diff line
@@ -76,6 +76,8 @@ bool ConnectionHandler::CleanUp() { 
  instance_->device_map_.clear();

  instance_->feature_map_.clear();



  instance_->weak_ptr_factory_.InvalidateWeakPtrs();



  delete instance_;

  instance_ = nullptr;
@@ -168,10 +170,10 @@ bool ConnectionHandler::AvrcpConnect(bool initiator, const RawAddress& bdaddr) { 
  tAVRC_CONN_CB open_cb;

  if (initiator) {

    open_cb.ctrl_cback = base::Bind(&ConnectionHandler::InitiatorControlCb,

                                    base::Unretained(this));

                                    weak_ptr_factory_.GetWeakPtr());

  } else {

    open_cb.ctrl_cback = base::Bind(&ConnectionHandler::AcceptorControlCb,

                                    base::Unretained(this));

                                    weak_ptr_factory_.GetWeakPtr());

  }

  open_cb.msg_cback =

      base::Bind(&ConnectionHandler::MessageCb, base::Unretained(this));

system/profile/avrcp/connection_handler.h

+5 −2
Original line number Diff line number Diff line
@@ -17,6 +17,7 @@ 
#pragma once



#include <base/bind.h>

#include <base/memory/weak_ptr.h>

#include <map>

#include <memory>
@@ -148,12 +149,14 @@ class ConnectionHandler { 
  void MessageCb(uint8_t handle, uint8_t label, uint8_t opcode,

                 tAVRC_MSG* p_msg);



  ConnectionHandler() = default;

  ConnectionHandler() : weak_ptr_factory_(this){};

  virtual ~ConnectionHandler() = default;



  // Callback for when sending a response to a device

  void SendMessage(uint8_t handle, uint8_t label, bool browse,

                   std::unique_ptr<::bluetooth::PacketBuilder> message);



  base::WeakPtrFactory<ConnectionHandler> weak_ptr_factory_;

  DISALLOW_COPY_AND_ASSIGN(ConnectionHandler);

};

system/profile/avrcp/tests/avrcp_connection_handler_test.cc

+24 −0
Original line number Diff line number Diff line
@@ -133,6 +133,30 @@ TEST_F(AvrcpConnectionHandlerTest, notConnectedDisconnectTest) { 
  ConnectionHandler::CleanUp();

};



// Test calling the connection callback after the handler is cleaned up

TEST_F(AvrcpConnectionHandlerTest, disconnectAfterCleanupTest) {

  // Set an Expectation that Open will be called twice as an acceptor and save

  // the connection callback once it is called.

  tAVRC_CONN_CB conn_cb;

  EXPECT_CALL(mock_avrcp_, Open(_, _, RawAddress::kAny))

      .Times(1)

      .WillOnce(

          DoAll(SetArgPointee<0>(1), SaveArgPointee<1>(&conn_cb), Return(0)));



  // Initialize the interface

  auto bound_callback = base::Bind(&MockFunction<void(device_ptr)>::Call,

                                   base::Unretained(&device_cb));

  ASSERT_TRUE(ConnectionHandler::Initialize(bound_callback, &mock_avrcp_,

                                            &mock_sdp_, &mock_volume_));

  connection_handler_ = ConnectionHandler::Get();



  connection_handler_ = nullptr;

  ConnectionHandler::CleanUp();



  // Call the callback with a message saying the connection has closed

  conn_cb.ctrl_cback.Run(1, AVRC_CLOSE_IND_EVT, 0, &RawAddress::kAny);

};



// Check that we can handle having a remote device connect to us, start SDP, and

// open another acceptor connection

TEST_F(AvrcpConnectionHandlerTest, remoteDeviceConnectionTest) {