From 02191dbfac2c7a555d9d154ec9b2aa23a8313ebc Mon Sep 17 00:00:00 2001
From: David Anderson <dvander@google.com>
Date: Wed, 5 Jul 2023 16:54:42 -0700
Subject: [PATCH] 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
---
 .../user-space-merge/handler_manager.cpp      | 13 ++++++++-----
 .../user-space-merge/handler_manager.h        |  2 +-
 .../user-space-merge/snapuserd_test.cpp       | 19 +++++++++++--------
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp
index fd41cd419..50b9d4331 100644
--- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp
+++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.cpp
@@ -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();
     }
 
-    stop_monitor_merge_thread_ = true;
-    WakeupMonitorMergeThread();
+    if (merge_monitor_.joinable()) {
+        stop_monitor_merge_thread_ = true;
+        WakeupMonitorMergeThread();
+
+        merge_monitor_.join();
+    }
 }
 
 auto SnapshotHandlerManager::FindHandler(std::lock_guard<std::mutex>* proof_of_lock,
diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.h b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.h
index 2a6da964e..b1605f098 100644
--- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.h
+++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/handler_manager.h
@@ -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_;
diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_test.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_test.cpp
index 7e1b2c40f..0e02f0b85 100644
--- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_test.cpp
+++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_test.cpp
@@ -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,8 +527,8 @@ 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(),
-                                 base_dev_->GetPath(), opener, 1, use_iouring, false);
+            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);
 #ifdef __ANDROID__
@@ -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() {