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

Commit a16ac3f7 authored by Chienyuan's avatar Chienyuan
Browse files

Fix memory leak of reactor and reactor_unittest 

* Fix memory leak happen in below two cases

Case 1

1. Reactable1 register
2. Reactable2 register
3. Reactable2 unregister during the callback event of Reactable2
4. Reactable1 unregister

In step 3, reactable_removed_ will set to true due to unregister while
reactable->is_executing_ is true, it makes sure we delete Reactable2
after the callback is executed. But it will cause Reactable1 to not be
deleted in step 4.

To avoid this, we can reset reactable_removed_ to false after Reactable
deleted.

Case 2

1. Reactable1 register
2. Reactable2 register
3. Reactable2 unregister during the callback event of Reactable2
4. Reactable1 unregister from different thread during step 3 processing

In step 3, although we reset reactable_removed_ to false after Reactable2
deleted immediately for case 1, if other thread unregister Reactable1
before Reactable2 deleted, Reactable1 will fail to be deleted.

To avoid thie, we add a local variable to check if the Reacable is
executing. If not, deleted the Reactable directly.

* Add unittest for these two cases
* Fix memory leak in reactor_unittest
* Turn on cfi and address flag

Test: sudo ./bluetooth_test_gd
Change-Id: I0a0ca79b439fd3a1bf3ec0fa2b2a43a88e037fbb
parent 51647827

system/gd/Android.bp

+1 −1
Original line number Diff line number Diff line
@@ -109,7 +109,7 @@ cc_test { 
        "libbluetooth_gd",

    ],

    sanitize: {

        cfi: false,

        address: true,

    },

}

system/gd/os/linux_generic/reactor.cc

+5 −1
Original line number Diff line number Diff line
@@ -123,6 +123,7 @@ void Reactor::Run() { 
      }

      if (reactable_removed_) {

        delete reactable;

        reactable_removed_ = false;

      }

    }

  }
@@ -161,6 +162,7 @@ void Reactor::Unregister(Reactor::Reactable* reactable) { 
    std::lock_guard<std::mutex> lock(mutex_);

    invalidation_list_.push_back(reactable);

  }

  bool delaying_delete_until_callback_finished = false;

  {

    int result;

    std::lock_guard<std::recursive_mutex> reactable_lock(reactable->lock_);
@@ -170,14 +172,16 @@ void Reactor::Unregister(Reactor::Reactable* reactable) { 
    } else {

      ASSERT(result != -1);

    }



    // If we are unregistering during the callback event from this reactable, we delete it after the callback is executed.

    // reactable->is_executing_ is protected by reactable->lock_, so it's thread safe.

    if (reactable->is_executing_) {

      reactable_removed_ = true;

      delaying_delete_until_callback_finished = true;

    }

  }

  // If we are unregistering outside of the callback event from this reactable, we delete it now

  if (!reactable_removed_) {

  if (!delaying_delete_until_callback_finished) {

    delete reactable;

  }

}

system/gd/os/linux_generic/reactor_unittest.cc

+58 −3
Original line number Diff line number Diff line
@@ -51,7 +51,7 @@ class ReactorTest : public ::testing::Test { 
class SampleReactable {

 public:

  SampleReactable() : fd_(eventfd(0, EFD_NONBLOCK)) {

    EXPECT_NE(fd_, 0);

    EXPECT_NE(fd_, -1);

  }



  ~SampleReactable() {
@@ -74,11 +74,11 @@ class FakeReactable { 
    kSampleOutputValue,

  };

  FakeReactable() : fd_(eventfd(0, 0)), reactor_(nullptr) {

    EXPECT_NE(fd_, 0);

    EXPECT_NE(fd_, -1);

  }



  FakeReactable(Reactor* reactor) : fd_(eventfd(0, 0)), reactor_(reactor) {

    EXPECT_NE(fd_, 0);

    EXPECT_NE(fd_, -1);

  }



  ~FakeReactable() {
@@ -109,6 +109,14 @@ class FakeReactable { 
    EXPECT_EQ(write_result, 0);

  }



  void UnregisterInCallback() {

    uint64_t value = 0;

    auto read_result = eventfd_read(fd_, &value);

    EXPECT_EQ(read_result, 0);

    g_promise->set_value(kReadReadyValue);

    reactor_->Unregister(reactable_);

  }



  SampleReactable sample_reactable_;

  Reactor::Reactable* reactable_ = nullptr;

  int fd_;
@@ -205,6 +213,11 @@ TEST_F(ReactorTest, hot_register_from_same_thread) { 
  auto write_result = eventfd_write(fake_reactable.fd_, FakeReactable::kRegisterSampleReactable);

  EXPECT_EQ(write_result, 0);

  EXPECT_EQ(future.get(), kReadReadyValue);

  delete g_promise;

  g_promise = new std::promise<int>;

  future = g_promise->get_future();

  write_result = eventfd_write(fake_reactable.fd_, FakeReactable::kUnregisterSampleReactable);

  EXPECT_EQ(write_result, 0);

  reactor_->Stop();

  reactor_thread.join();
@@ -233,6 +246,46 @@ TEST_F(ReactorTest, hot_unregister_from_same_thread) { 
  reactor_->Unregister(reactable);

}



TEST_F(ReactorTest, hot_unregister_from_callback) {

  auto reactor_thread = std::thread(&Reactor::Run, reactor_);



  FakeReactable fake_reactable1(reactor_);

  auto* reactable1 =

      reactor_->Register(fake_reactable1.fd_, std::bind(&FakeReactable::OnReadReady, &fake_reactable1), nullptr);



  FakeReactable fake_reactable2(reactor_);

  auto* reactable2 = reactor_->Register(fake_reactable2.fd_,

                                        std::bind(&FakeReactable::UnregisterInCallback, &fake_reactable2), nullptr);

  fake_reactable2.reactable_ = reactable2;

  auto write_result = eventfd_write(fake_reactable2.fd_, 1);

  EXPECT_EQ(write_result, 0);

  reactor_->Stop();

  reactor_thread.join();



  reactor_->Unregister(reactable1);

}



TEST_F(ReactorTest, hot_unregister_during_unregister_from_callback) {

  auto reactor_thread = std::thread(&Reactor::Run, reactor_);

  auto future = g_promise->get_future();



  FakeReactable fake_reactable1(reactor_);

  auto* reactable1 =

      reactor_->Register(fake_reactable1.fd_, std::bind(&FakeReactable::OnReadReady, &fake_reactable1), nullptr);



  FakeReactable fake_reactable2(reactor_);

  auto* reactable2 = reactor_->Register(fake_reactable2.fd_,

                                        std::bind(&FakeReactable::UnregisterInCallback, &fake_reactable2), nullptr);

  fake_reactable2.reactable_ = reactable2;

  auto write_result = eventfd_write(fake_reactable2.fd_, 1);

  EXPECT_EQ(write_result, 0);

  EXPECT_EQ(future.get(), kReadReadyValue);

  reactor_->Unregister(reactable1);



  reactor_->Stop();

  reactor_thread.join();

}



TEST_F(ReactorTest, start_and_stop_multi_times) {

  auto reactor_thread = std::thread(&Reactor::Run, reactor_);

  reactor_->Stop();
@@ -273,6 +326,8 @@ TEST_F(ReactorTest, modify_registration) { 


  reactor_->Stop();

  reactor_thread.join();



  reactor_->Unregister(reactable);

}



}  // namespace