Fix undefined behavior in BucketSnapshotManager

Author: marta-lokhova

src/bucket/BucketSnapshotManager.cpp

@@ -55,7 +55,24 @@ BucketSnapshotManager::BucketSnapshotManager(
 SearchableSnapshotConstPtr
 BucketSnapshotManager::copySearchableLiveBucketListSnapshot() const
 {
-    // SharedLockShared guard(mSnapshotMutex);
+    SharedLockShared guard(mSnapshotMutex);
+    // Can't use std::make_shared due to private constructor
+    return copySearchableLiveBucketListSnapshot(guard);
+}
+
+SearchableHotArchiveSnapshotConstPtr
+BucketSnapshotManager::copySearchableHotArchiveBucketListSnapshot() const
+{
+    SharedLockShared guard(mSnapshotMutex);
+    releaseAssert(mCurrHotArchiveSnapshot);
+    // Can't use std::make_shared due to private constructor
+    return copySearchableHotArchiveBucketListSnapshot(guard);
+}
+
+SearchableSnapshotConstPtr
+BucketSnapshotManager::copySearchableLiveBucketListSnapshot(
+    SharedLockShared const& guard) const
+{
     // Can't use std::make_shared due to private constructor
     return std::shared_ptr<SearchableLiveBucketListSnapshot>(
         new SearchableLiveBucketListSnapshot(
@@ -66,9 +83,9 @@ BucketSnapshotManager::copySearchableLiveBucketListSnapshot() const
 }
 
 SearchableHotArchiveSnapshotConstPtr
-BucketSnapshotManager::copySearchableHotArchiveBucketListSnapshot() const
+BucketSnapshotManager::copySearchableHotArchiveBucketListSnapshot(
+    SharedLockShared const& guard) const
 {
-    // SharedLockShared guard(mSnapshotMutex);
     releaseAssert(mCurrHotArchiveSnapshot);
     // Can't use std::make_shared due to private constructor
     return std::shared_ptr<SearchableHotArchiveBucketListSnapshot>(
@@ -90,7 +107,7 @@ BucketSnapshotManager::maybeCopySearchableBucketListSnapshot(
     if (!snapshot ||
         snapshot->getLedgerSeq() < mCurrLiveSnapshot->getLedgerSeq())
     {
-        snapshot = copySearchableLiveBucketListSnapshot();
+        snapshot = copySearchableLiveBucketListSnapshot(guard);
     }
 }
 
@@ -106,7 +123,7 @@ BucketSnapshotManager::maybeCopySearchableHotArchiveBucketListSnapshot(
     if (!snapshot ||
         snapshot->getLedgerSeq() < mCurrHotArchiveSnapshot->getLedgerSeq())
     {
-        snapshot = copySearchableHotArchiveBucketListSnapshot();
+        snapshot = copySearchableHotArchiveBucketListSnapshot(guard);
     }
 }
 

src/bucket/BucketSnapshotManager.h

@@ -88,6 +88,18 @@ class BucketSnapshotManager : NonMovableOrCopyable
     copySearchableHotArchiveBucketListSnapshot() const
         LOCKS_EXCLUDED(mSnapshotMutex);
 
+    // Copy the most recent snapshot for the live bucket list, while holding the
+    // lock
+    SearchableSnapshotConstPtr
+    copySearchableLiveBucketListSnapshot(SharedLockShared const& guard) const
+        REQUIRES_SHARED(mSnapshotMutex);
+
+    // Copy the most recent snapshot for the hot archive bucket list, while
+    // holding the lock
+    SearchableHotArchiveSnapshotConstPtr
+    copySearchableHotArchiveBucketListSnapshot(
+        SharedLockShared const& guard) const REQUIRES_SHARED(mSnapshotMutex);
+
     // `maybeCopy` interface refreshes `snapshot` if a newer snapshot is
     // available. It's a no-op otherwise. This is useful to avoid unnecessary
     // copying.

src/bucket/SearchableBucketList.h

@@ -36,7 +36,8 @@ class SearchableLiveBucketListSnapshot
         StateArchivalSettings const& sas, uint32_t ledgerVers) const;
 
     friend SearchableSnapshotConstPtr
-    BucketSnapshotManager::copySearchableLiveBucketListSnapshot() const;
+    BucketSnapshotManager::copySearchableLiveBucketListSnapshot(
+        SharedLockShared const& guard) const;
 };
 
 class SearchableHotArchiveBucketListSnapshot
@@ -54,6 +55,7 @@ class SearchableHotArchiveBucketListSnapshot
     loadKeys(std::set<LedgerKey, LedgerEntryIdCmp> const& inKeys) const;
 
     friend SearchableHotArchiveSnapshotConstPtr
-    BucketSnapshotManager::copySearchableHotArchiveBucketListSnapshot() const;
+    BucketSnapshotManager::copySearchableHotArchiveBucketListSnapshot(
+        SharedLockShared const& guard) const;
 };
 }