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

Commit 02191dbf authored by David Anderson's avatar David Anderson
Browse files

snapuserd: Fix race condition in HandleManager shutdown. 

When HandlerManager shuts down, the monitor thread is left detached. The
monitor thread does not hold a shared_ptr reference to the
HandlerManager, so the pointer can be left dangling.

Fix this by not detaching the monitor merge thread.

This patch also changes the test harness to destroy
SnapshotHandlerManager on "shutdown", to avoid state leaking into the
next instance of snapuserd.

Bug: 288273605
Test: snapuserd_test
Change-Id: Iaaf96a37657c85cff4d2a8b15ccfde4aa03d3220
parent db70cbf7

fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp

+8 −5
Original line number Diff line number Diff line
@@ -201,9 +201,8 @@ bool SnapshotHandlerManager::StartMerge(std::lock_guard<std::mutex>* proof_of_lo 


    handler->snapuserd()->MonitorMerge();



    if (!is_merge_monitor_started_) {

        std::thread(&SnapshotHandlerManager::MonitorMerge, this).detach();

        is_merge_monitor_started_ = true;

    if (!merge_monitor_.joinable()) {

        merge_monitor_ = std::thread(&SnapshotHandlerManager::MonitorMerge, this);

    }



    merge_handlers_.push(handler);
@@ -357,8 +356,12 @@ void SnapshotHandlerManager::JoinAllThreads() { 
        if (th.joinable()) th.join();

    }



    if (merge_monitor_.joinable()) {

        stop_monitor_merge_thread_ = true;

        WakeupMonitorMergeThread();



        merge_monitor_.join();

    }

}



auto SnapshotHandlerManager::FindHandler(std::lock_guard<std::mutex>* proof_of_lock,

fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.h

+1 −1
Original line number Diff line number Diff line
@@ -122,9 +122,9 @@ class SnapshotHandlerManager final : public ISnapshotHandlerManager { 
    std::mutex lock_;

    HandlerList dm_users_;



    bool is_merge_monitor_started_ = false;

    bool stop_monitor_merge_thread_ = false;

    int active_merge_threads_ = 0;

    std::thread merge_monitor_;

    int num_partitions_merge_complete_ = 0;

    std::queue<std::shared_ptr<HandlerThread>> merge_handlers_;

    android::base::unique_fd monitor_merge_event_fd_;

fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_test.cpp

+11 −8
Original line number Diff line number Diff line
@@ -106,7 +106,7 @@ class SnapuserdTest : public ::testing::Test { 
    std::unique_ptr<TemporaryFile> cow_system_;

    std::unique_ptr<uint8_t[]> orig_buffer_;

    std::unique_ptr<uint8_t[]> merged_buffer_;

    SnapshotHandlerManager handlers_;

    std::unique_ptr<SnapshotHandlerManager> handlers_;

    bool setup_ok_ = false;

    bool merge_ok_ = false;

    size_t size_ = 100_MiB;
@@ -117,15 +117,18 @@ class SnapuserdTest : public ::testing::Test { 


void SnapuserdTest::SetUp() {

    harness_ = std::make_unique<DmUserTestHarness>();

    handlers_ = std::make_unique<SnapshotHandlerManager>();

}



void SnapuserdTest::Shutdown() {

    ASSERT_TRUE(dmuser_dev_->Destroy());



    auto misc_device = "/dev/dm-user/" + system_device_ctrl_name_;

    ASSERT_TRUE(handlers_.DeleteHandler(system_device_ctrl_name_));

    ASSERT_TRUE(handlers_->DeleteHandler(system_device_ctrl_name_));

    ASSERT_TRUE(android::fs_mgr::WaitForFileDeleted(misc_device, 10s));

    handlers_.TerminateMergeThreads();

    handlers_->TerminateMergeThreads();

    handlers_->JoinAllThreads();

    handlers_ = std::make_unique<SnapshotHandlerManager>();

}



bool SnapuserdTest::SetupDefault() {
@@ -524,7 +527,7 @@ void SnapuserdTest::InitCowDevice() { 
    auto factory = harness_->GetBlockServerFactory();

    auto opener = factory->CreateOpener(system_device_ctrl_name_);

    auto handler =

            handlers_.AddHandler(system_device_ctrl_name_, cow_system_->path, base_dev_->GetPath(),

            handlers_->AddHandler(system_device_ctrl_name_, cow_system_->path, base_dev_->GetPath(),

                                  base_dev_->GetPath(), opener, 1, use_iouring, false);

    ASSERT_NE(handler, nullptr);

    ASSERT_NE(handler->snapuserd(), nullptr);
@@ -557,12 +560,12 @@ void SnapuserdTest::CreateUserDevice() { 
}



void SnapuserdTest::InitDaemon() {

    ASSERT_TRUE(handlers_.StartHandler(system_device_ctrl_name_));

    ASSERT_TRUE(handlers_->StartHandler(system_device_ctrl_name_));

}



void SnapuserdTest::CheckMergeCompletion() {

    while (true) {

        double percentage = handlers_.GetMergePercentage();

        double percentage = handlers_->GetMergePercentage();

        if ((int)percentage == 100) {

            break;

        }
@@ -592,7 +595,7 @@ bool SnapuserdTest::Merge() { 
}



void SnapuserdTest::StartMerge() {

    ASSERT_TRUE(handlers_.InitiateMerge(system_device_ctrl_name_));

    ASSERT_TRUE(handlers_->InitiateMerge(system_device_ctrl_name_));

}



void SnapuserdTest::ValidateMerge() {