From 783480935031d2d671c2731f137a7584d056c667 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 6 Sep 2022 18:34:13 -0700 Subject: [PATCH] vts_libsnapshot_test: Fix test flakiness. This patch fixes a few lingering issues in vts_libsnapshot_test. The most important fix is a crash in snapuserd when handler deletion races with the merge monitor thread. Since tests issue lots of snapshot-related requests in rapid succession, this was easy to hit in presubmit, and resulted in a null-pointer deref. SnapuserdClient's CloseConnection does the same thing as the destructor, but leaves SnapuserdClient in an unusable state. This method is removed in favor of RAII. Fix a bug in SnapshotManager where CloseConnection could be called without zapping snapuserd_client_. Fix a bug where POLLHUP was checked before calling recv(). Add test name logging so presubmit failures can be diagnosed via logcat dumps. Bug: N/A Test: vts_libsnapshot_test on cuttlefish Change-Id: I8f22a45e537c24a3c6d327ac47bf8b1352108706 --- fs_mgr/libsnapshot/snapshot.cpp | 4 +--- fs_mgr/libsnapshot/snapshot_test.cpp | 14 +++++++++++++- .../include/snapuserd/snapuserd_client.h | 2 -- .../user-space-merge/snapuserd_server.cpp | 15 ++++++++++----- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index bc3efd99b..59abd6f82 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -1498,7 +1498,6 @@ void SnapshotManager::AcknowledgeMergeSuccess(LockedFile* lock) { if (UpdateUsesUserSnapshots(lock) && !device()->IsTestDevice()) { if (snapuserd_client_) { snapuserd_client_->DetachSnapuserd(); - snapuserd_client_->CloseConnection(); snapuserd_client_ = nullptr; } } @@ -2794,7 +2793,6 @@ bool SnapshotManager::UnmapAllSnapshots(LockedFile* lock) { if (snapuserd_client_) { LOG(INFO) << "Shutdown snapuserd daemon"; snapuserd_client_->DetachSnapuserd(); - snapuserd_client_->CloseConnection(); snapuserd_client_ = nullptr; } @@ -3317,7 +3315,7 @@ Return SnapshotManager::CreateUpdateSnapshots(const DeltaArchiveManifest& manife } if (snapuserd_client) { snapuserd_client->DetachSnapuserd(); - snapuserd_client->CloseConnection(); + snapuserd_client = nullptr; } } diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 2233a383a..2c01cf649 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -107,6 +107,12 @@ class SnapshotTest : public ::testing::Test { protected: void SetUp() override { + const testing::TestInfo* const test_info = + testing::UnitTest::GetInstance()->current_test_info(); + test_name_ = test_info->test_suite_name() + "/"s + test_info->name(); + + LOG(INFO) << "Starting test: " << test_name_; + SKIP_IF_NON_VIRTUAL_AB(); SetupProperties(); @@ -152,10 +158,14 @@ class SnapshotTest : public ::testing::Test { void TearDown() override { RETURN_IF_NON_VIRTUAL_AB(); + LOG(INFO) << "Tearing down SnapshotTest test: " << test_name_; + lock_ = nullptr; CleanupTestArtifacts(); SnapshotTestPropertyFetcher::TearDown(); + + LOG(INFO) << "Teardown complete for test: " << test_name_; } void InitializeState() { @@ -487,6 +497,7 @@ class SnapshotTest : public ::testing::Test { android::fiemap::IImageManager* image_manager_ = nullptr; std::string fake_super_; bool snapuserd_required_ = false; + std::string test_name_; }; TEST_F(SnapshotTest, CreateSnapshot) { @@ -1003,6 +1014,8 @@ class SnapshotUpdateTest : public SnapshotTest { void TearDown() override { RETURN_IF_NON_VIRTUAL_AB(); + LOG(INFO) << "Tearing down SnapshotUpdateTest test: " << test_name_; + Cleanup(); SnapshotTest::TearDown(); } @@ -2797,7 +2810,6 @@ void KillSnapuserd() { return; } snapuserd_client->DetachSnapuserd(); - snapuserd_client->CloseConnection(); } } // namespace snapshot diff --git a/fs_mgr/libsnapshot/snapuserd/include/snapuserd/snapuserd_client.h b/fs_mgr/libsnapshot/snapuserd/include/snapuserd/snapuserd_client.h index 9a69d588c..4b62b20e2 100644 --- a/fs_mgr/libsnapshot/snapuserd/include/snapuserd/snapuserd_client.h +++ b/fs_mgr/libsnapshot/snapuserd/include/snapuserd/snapuserd_client.h @@ -71,8 +71,6 @@ class SnapuserdClient { // must ONLY be called if the control device has already been deleted. bool WaitForDeviceDelete(const std::string& control_device); - void CloseConnection() { sockfd_ = {}; } - // Detach snapuserd. This shuts down the listener socket, and will cause // snapuserd to gracefully exit once all handler threads have terminated. // This should only be used on first-stage instances of snapuserd. diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp index 1bf33c8fb..d437d3286 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp @@ -462,15 +462,14 @@ void UserSnapshotServer::AcceptClient() { } bool UserSnapshotServer::HandleClient(android::base::borrowed_fd fd, int revents) { - if (revents & POLLHUP) { - LOG(DEBUG) << "Snapuserd client disconnected"; - return false; - } - std::string str; if (!Recv(fd, &str)) { return false; } + if (str.empty() && (revents & POLLHUP)) { + LOG(DEBUG) << "Snapuserd client disconnected"; + return false; + } if (!Receivemsg(fd, str)) { LOG(ERROR) << "Encountered error handling client message, revents: " << revents; return false; @@ -650,6 +649,12 @@ void UserSnapshotServer::MonitorMerge() { while (active_merge_threads_ < kMaxMergeThreads && merge_handlers_.size() > 0) { auto handler = merge_handlers_.front(); merge_handlers_.pop(); + + if (!handler->snapuserd()) { + LOG(INFO) << "MonitorMerge: skipping deleted handler: " << handler->misc_name(); + continue; + } + LOG(INFO) << "Starting merge for partition: " << handler->snapuserd()->GetMiscName(); handler->snapuserd()->InitiateMerge();