From 3983f9aa6ec3961a7581f95f817f189a8ca5c492 Mon Sep 17 00:00:00 2001
From: Akilesh Kailash <akailash@google.com>
Date: Wed, 9 Aug 2023 23:08:21 -0700
Subject: [PATCH] libsnapshot: Check for valid snapshots based on current slot

We may have snapshot files in /metadata/ota/snapshot/ which ends with
.tmp such as system_a.tmp - This happens if the device
reboots just before `rename` in `WriteStringToFileAtomic`. This
can lead to spurious merge failures.

Log the error and skip these snapshot files. It is ok to skip
as we will still have original snapshot status files since
we are already in the merge path. Additionally, try to remove
these files when snapshot is deleted.

Bug: 292198189
Test: OTA
Change-Id: I5db3dbd5a919b263ae577185de3e7f79a5e9b89a
Signed-off-by: Akilesh Kailash <akailash@google.com>
---
 fs_mgr/libsnapshot/snapshot.cpp      | 18 +++++++++++++-----
 fs_mgr/libsnapshot/snapshot_test.cpp | 23 ++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp
index 86ff5f710..51389a0db 100644
--- a/fs_mgr/libsnapshot/snapshot.cpp
+++ b/fs_mgr/libsnapshot/snapshot.cpp
@@ -729,6 +729,14 @@ bool SnapshotManager::DeleteSnapshot(LockedFile* lock, const std::string& name)
         LOG(ERROR) << "Failed to remove status file " << file_path << ": " << error;
         return false;
     }
+
+    // This path may never exist. If it is present, then it's a stale
+    // snapshot status file. Just remove the file and log the message.
+    const std::string tmp_path = file_path + ".tmp";
+    if (!android::base::RemoveFileIfExists(tmp_path, &error)) {
+        LOG(ERROR) << "Failed to remove stale snapshot file " << tmp_path;
+    }
+
     return true;
 }
 
@@ -754,10 +762,10 @@ bool SnapshotManager::InitiateMerge() {
         return false;
     }
 
-    auto other_suffix = device_->GetOtherSlotSuffix();
+    auto current_slot_suffix = device_->GetSlotSuffix();
 
     for (const auto& snapshot : snapshots) {
-        if (android::base::EndsWith(snapshot, other_suffix)) {
+        if (!android::base::EndsWith(snapshot, current_slot_suffix)) {
             // Allow the merge to continue, but log this unexpected case.
             LOG(ERROR) << "Unexpected snapshot found during merge: " << snapshot;
             continue;
@@ -1123,7 +1131,7 @@ auto SnapshotManager::CheckMergeState(LockedFile* lock, const std::function<bool
         return MergeResult(UpdateState::MergeFailed, MergeFailureCode::ListSnapshots);
     }
 
-    auto other_suffix = device_->GetOtherSlotSuffix();
+    auto current_slot_suffix = device_->GetSlotSuffix();
 
     bool cancelled = false;
     bool merging = false;
@@ -1131,9 +1139,9 @@ auto SnapshotManager::CheckMergeState(LockedFile* lock, const std::function<bool
     bool wrong_phase = false;
     MergeFailureCode failure_code = MergeFailureCode::Ok;
     for (const auto& snapshot : snapshots) {
-        if (android::base::EndsWith(snapshot, other_suffix)) {
+        if (!android::base::EndsWith(snapshot, current_slot_suffix)) {
             // This will have triggered an error message in InitiateMerge already.
-            LOG(INFO) << "Skipping merge validation of unexpected snapshot: " << snapshot;
+            LOG(ERROR) << "Skipping merge validation of unexpected snapshot: " << snapshot;
             continue;
         }
 
diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp
index 0a85489bd..3b6d26aa1 100644
--- a/fs_mgr/libsnapshot/snapshot_test.cpp
+++ b/fs_mgr/libsnapshot/snapshot_test.cpp
@@ -685,6 +685,17 @@ TEST_F(SnapshotTest, Merge) {
     }
     ASSERT_TRUE(sm->InitiateMerge());
 
+    // Create stale files in snapshot directory. Merge should skip these files
+    // as the suffix doesn't match the current slot.
+    auto tmp_path = test_device->GetMetadataDir() + "/snapshots/test_partition_b.tmp";
+    auto other_slot = test_device->GetMetadataDir() + "/snapshots/test_partition_a";
+
+    unique_fd fd(open(tmp_path.c_str(), O_RDWR | O_CLOEXEC | O_CREAT, 0644));
+    ASSERT_GE(fd, 0);
+
+    fd.reset(open(other_slot.c_str(), O_RDWR | O_CLOEXEC | O_CREAT, 0644));
+    ASSERT_GE(fd, 0);
+
     // The device should have been switched to a snapshot-merge target.
     DeviceMapper::TargetInfo target;
     ASSERT_TRUE(sm->IsSnapshotDevice("test_partition_b", &target));
@@ -700,13 +711,23 @@ TEST_F(SnapshotTest, Merge) {
     ASSERT_EQ(sm->ProcessUpdateState(), UpdateState::MergeCompleted);
     ASSERT_EQ(sm->GetUpdateState(), UpdateState::None);
 
+    // Make sure that snapshot states are cleared and all stale files
+    // are deleted
+    {
+        ASSERT_TRUE(AcquireLock());
+        auto local_lock = std::move(lock_);
+        std::vector<std::string> snapshots;
+        ASSERT_TRUE(sm->ListSnapshots(local_lock.get(), &snapshots));
+        ASSERT_TRUE(snapshots.empty());
+    }
+
     // The device should no longer be a snapshot or snapshot-merge.
     ASSERT_FALSE(sm->IsSnapshotDevice("test_partition_b"));
 
     // Test that we can read back the string we wrote to the snapshot. Note
     // that the base device is gone now. |snap_device| contains the correct
     // partition.
-    unique_fd fd(open("/dev/block/mapper/test_partition_b", O_RDONLY | O_CLOEXEC));
+    fd.reset(open("/dev/block/mapper/test_partition_b", O_RDONLY | O_CLOEXEC));
     ASSERT_GE(fd, 0);
 
     std::string buffer(test_string.size(), '\0');