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
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 fd41cd4..50b9d43 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 @@
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 @@
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 2a6da96..b1605f0 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 @@
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 7e1b2c4..0e02f0b 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 @@
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 @@
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 @@
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::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 @@
}
void SnapuserdTest::StartMerge() {
- ASSERT_TRUE(handlers_.InitiateMerge(system_device_ctrl_name_));
+ ASSERT_TRUE(handlers_->InitiateMerge(system_device_ctrl_name_));
}
void SnapuserdTest::ValidateMerge() {