MB-24159: Fix segfault in ephemeral backfill

DCPBackfillMemory::run would segfault if the underlying rangeRead
returned no items.

This was because the front and back of the UniqueItemPtr vector were
unconditionally dereferenced even if non-existent. This was to call
getBySeqno().

This patch replaces these calls with the startSeqno and endSeqno
specified when the DCPBackfillMemory task was created. This is
consistent with the behaviour of DCPBackfillDisk.

Change-Id: I952a78ef3d931bc0832cfffb9e392b394d412fb3
Reviewed-on: http://review.couchbase.org/77780
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>

diff --git a/src/dcp/backfill_memory.cc b/src/dcp/backfill_memory.cc
index e9ef052..63491ab 100644 (file)
--- a/src/dcp/backfill_memory.cc
+++ b/src/dcp/backfill_memory.cc
@@ -61,9 +61,11 @@ backfill_status_t DCPBackfillMemory::run() {
     }
 
     /* Get sequence of items (backfill) from memory */
-    std::vector<UniqueItemPtr> items;
     ENGINE_ERROR_CODE status;
-    std::tie(status, items) = evb->inMemoryBackfill(startSeqno, endSeqno);
+    std::vector<UniqueItemPtr> items;
+    seqno_t adjustedEndSeqno;
+    std::tie(status, items, adjustedEndSeqno) =
+            evb->inMemoryBackfill(startSeqno, endSeqno);
 
     /* Handle any failures */
     if (status != ENGINE_SUCCESS) {
@@ -86,8 +88,7 @@ backfill_status_t DCPBackfillMemory::run() {
     stream->incrBackfillRemaining(items.size());
 
     /* Mark disk snapshot */
-    stream->markDiskSnapshot(items.front()->getBySeqno(),
-                             items.back()->getBySeqno());
+    stream->markDiskSnapshot(startSeqno, adjustedEndSeqno);
 
     /* Move every item to the stream */
     for (auto& item : items) {

diff --git a/src/ephemeral_vb.cc b/src/ephemeral_vb.cc
index 2379690..68e573c 100644 (file)
--- a/src/ephemeral_vb.cc
+++ b/src/ephemeral_vb.cc
@@ -241,7 +241,7 @@ std::unique_ptr<DCPBackfill> EphemeralVBucket::createDCPBackfill(
             evb, stream, startSeqno, endSeqno);
 }
 
-std::pair<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>>
+std::tuple<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>, seqno_t>
 EphemeralVBucket::inMemoryBackfill(uint64_t start, uint64_t end) {
     return seqList->rangeRead(start, end);
 }

diff --git a/src/ephemeral_vb.h b/src/ephemeral_vb.h
index ddfb754..22cd85a 100644 (file)
--- a/src/ephemeral_vb.h
+++ b/src/ephemeral_vb.h
@@ -105,17 +105,23 @@ public:
             uint64_t endSeqno) override;
 
     /**
-     * Reads backfill items from in memory ordered data structure
+     * Reads backfill items from in memory ordered data structure.
+     *
+     * Because the backfill may have to be extended to ensure consistency (e.g.,
+     * an item in the range has been updated and the new version is
+     * outside of the original range would result in a missing item), the
+     * end of the range may be at a higher seqno than was requested; this new
+     * end value is returned.
      *
      * @param startSeqno requested start sequence number of the backfill
      * @param endSeqno requested end sequence number of the backfill
      *
-     * @return ENGINE_SUCCESS and items in the snapshot
+     * @return ENGINE_SUCCESS, items in the snapshot, adjusted endSeqno
      *         ENGINE_ENOMEM on no memory to copy items
      *         ENGINE_ERANGE on incorrect start and end
      */
-    std::pair<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>> inMemoryBackfill(
-            uint64_t start, uint64_t end);
+    std::tuple<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>, seqno_t>
+    inMemoryBackfill(uint64_t start, uint64_t end);
 
     void dump() const override;
 

diff --git a/src/linked_list.cc b/src/linked_list.cc
index aa1dcda..63b3a80 100644 (file)
--- a/src/linked_list.cc
+++ b/src/linked_list.cc
@@ -81,7 +81,7 @@ SequenceList::UpdateStatus BasicLinkedList::updateListElem(
     return UpdateStatus::Success;
 }
 
-std::pair<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>>
+std::tuple<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>, seqno_t>
 BasicLinkedList::rangeRead(seqno_t start, seqno_t end) {
     std::vector<UniqueItemPtr> empty;
     if ((start > end) || (start <= 0)) {
@@ -91,8 +91,11 @@ BasicLinkedList::rangeRead(seqno_t start, seqno_t end) {
             vbid,
             start,
             end);
-        return {ENGINE_ERANGE, std::move(empty)
-                /* MSVC not happy with std::vector<UniqueItemPtr>() */};
+        return std::make_tuple(
+                ENGINE_ERANGE,
+                std::move(empty)
+                /* MSVC not happy with std::vector<UniqueItemPtr>() */,
+                0);
     }
 
     /* Allows only 1 rangeRead for now */
@@ -109,7 +112,7 @@ BasicLinkedList::rangeRead(seqno_t start, seqno_t end) {
                 static_cast<seqno_t>(highSeqno));
             /* If the request is for an invalid range, return before iterating
                through the list */
-            return {ENGINE_ERANGE, std::move(empty)};
+            return std::make_tuple(ENGINE_ERANGE, std::move(empty), 0);
         }
 
         /* Mark the initial read range */
@@ -152,7 +155,7 @@ BasicLinkedList::rangeRead(seqno_t start, seqno_t end) {
                 "item with seqno %" PRIi64 "before streaming it",
                 vbid,
                 osv.getBySeqno());
-            return {ENGINE_ENOMEM, std::move(empty)};
+            return std::make_tuple(ENGINE_ENOMEM, std::move(empty), 0);
         }
     }
 
@@ -163,7 +166,7 @@ BasicLinkedList::rangeRead(seqno_t start, seqno_t end) {
     }
 
     /* Return all the range read items */
-    return {ENGINE_SUCCESS, std::move(items)};
+    return std::make_tuple(ENGINE_SUCCESS, std::move(items), end);
 }
 
 void BasicLinkedList::updateHighSeqno(

diff --git a/src/linked_list.h b/src/linked_list.h
index 0c437c9..73d019b 100644 (file)
--- a/src/linked_list.h
+++ b/src/linked_list.h
@@ -143,8 +143,8 @@ public:
             std::lock_guard<std::mutex>& seqLock,
             OrderedStoredValue& v) override;
 
-    std::pair<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>> rangeRead(
-            seqno_t start, seqno_t end) override;
+    std::tuple<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>, seqno_t>
+    rangeRead(seqno_t start, seqno_t end) override;
 
     void updateHighSeqno(std::lock_guard<std::mutex>& highSeqnoLock,
                          const OrderedStoredValue& v) override;

diff --git a/src/seqlist.h b/src/seqlist.h
index 6e359bb..96d5315 100644 (file)
--- a/src/seqlist.h
+++ b/src/seqlist.h
@@ -114,12 +114,12 @@ public:
      * @param start requested start seqno
      * @param end requested end seqno
      *
-     * @return ENGINE_SUCCESS and items in the snapshot
+     * @return ENGINE_SUCCESS, items in the snapshot and adjusted endSeqNo
      *         ENGINE_ENOMEM on no memory to copy items
      *         ENGINE_ERANGE on incorrect start and end
      */
-    virtual std::pair<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>> rangeRead(
-            seqno_t start, seqno_t end) = 0;
+    virtual std::tuple<ENGINE_ERROR_CODE, std::vector<UniqueItemPtr>, seqno_t>
+    rangeRead(seqno_t start, seqno_t end) = 0;
 
     /**
      * Updates the highSeqno in the list. Since seqno is generated and managed

diff --git a/tests/module_tests/basic_ll_test.cc b/tests/module_tests/basic_ll_test.cc
index 52d290d..0fbabcc 100644 (file)
--- a/tests/module_tests/basic_ll_test.cc
+++ b/tests/module_tests/basic_ll_test.cc
@@ -186,11 +186,13 @@ TEST_F(BasicLinkedListTest, TestRangeRead) {
     /* Now do a range read */
     ENGINE_ERROR_CODE status;
     std::vector<UniqueItemPtr> items;
-    std::tie(status, items) = basicLL->rangeRead(1, numItems);
+    seqno_t endSeqno;
+    std::tie(status, items, endSeqno) = basicLL->rangeRead(1, numItems);
 
     EXPECT_EQ(ENGINE_SUCCESS, status);
     EXPECT_EQ(numItems, items.size());
     EXPECT_EQ(numItems, items.back()->getBySeqno());
+    EXPECT_EQ(numItems, endSeqno);
 }
 
 TEST_F(BasicLinkedListTest, TestRangeReadTillInf) {
@@ -202,12 +204,14 @@ TEST_F(BasicLinkedListTest, TestRangeReadTillInf) {
     /* Now do a range read */
     ENGINE_ERROR_CODE status;
     std::vector<UniqueItemPtr> items;
-    std::tie(status, items) =
+    seqno_t endSeqno;
+    std::tie(status, items, endSeqno) =
             basicLL->rangeRead(1, std::numeric_limits<seqno_t>::max());
 
     EXPECT_EQ(ENGINE_SUCCESS, status);
     EXPECT_EQ(numItems, items.size());
     EXPECT_EQ(numItems, items.back()->getBySeqno());
+    EXPECT_EQ(numItems, endSeqno);
 }
 
 TEST_F(BasicLinkedListTest, TestRangeReadFromMid) {
@@ -219,11 +223,13 @@ TEST_F(BasicLinkedListTest, TestRangeReadFromMid) {
     /* Now do a range read */
     ENGINE_ERROR_CODE status;
     std::vector<UniqueItemPtr> items;
-    std::tie(status, items) = basicLL->rangeRead(2, numItems);
+    seqno_t endSeqno;
+    std::tie(status, items, endSeqno) = basicLL->rangeRead(2, numItems);
 
     EXPECT_EQ(ENGINE_SUCCESS, status);
     EXPECT_EQ(numItems - 1, items.size());
     EXPECT_EQ(numItems, items.back()->getBySeqno());
+    EXPECT_EQ(numItems, endSeqno);
 }
 
 TEST_F(BasicLinkedListTest, TestRangeReadStopBeforeEnd) {
@@ -235,11 +241,13 @@ TEST_F(BasicLinkedListTest, TestRangeReadStopBeforeEnd) {
     /* Now request for a range read of just 2 items */
     ENGINE_ERROR_CODE status;
     std::vector<UniqueItemPtr> items;
-    std::tie(status, items) = basicLL->rangeRead(1, numItems - 1);
+    seqno_t endSeqno;
+    std::tie(status, items, endSeqno) = basicLL->rangeRead(1, numItems - 1);
 
     EXPECT_EQ(ENGINE_SUCCESS, status);
     EXPECT_EQ(numItems - 1, items.size());
     EXPECT_EQ(numItems - 1, items.back()->getBySeqno());
+    EXPECT_EQ(numItems - 1, endSeqno);
 }
 
 TEST_F(BasicLinkedListTest, TestRangeReadNegatives) {
@@ -252,11 +260,12 @@ TEST_F(BasicLinkedListTest, TestRangeReadNegatives) {
     std::vector<UniqueItemPtr> items;
 
     /* Now do a range read with start > end */
-    std::tie(status, items) = basicLL->rangeRead(2, 1);
+    std::tie(status, items, std::ignore) = basicLL->rangeRead(2, 1);
     EXPECT_EQ(ENGINE_ERANGE, status);
 
     /* Now do a range read with start > highSeqno */
-    std::tie(status, items) = basicLL->rangeRead(numItems + 1, numItems + 2);
+    std::tie(status, items, std::ignore) =
+            basicLL->rangeRead(numItems + 1, numItems + 2);
     EXPECT_EQ(ENGINE_ERANGE, status);
 }
 

diff --git a/tests/module_tests/dcp_test.cc b/tests/module_tests/dcp_test.cc
index 9661da0..a896948 100644 (file)
--- a/tests/module_tests/dcp_test.cc
+++ b/tests/module_tests/dcp_test.cc
@@ -34,6 +34,7 @@
 #include "test_helpers.h"
 
 #include <gtest/gtest.h>
+#include <dcp/backfill_memory.h>
 
 /*
  * Mock of the DcpConnMap class.  Wraps the real DcpConnMap, but exposes
@@ -174,6 +175,27 @@ TEST_P(StreamTest, test_streamSetMutationTypeKeyOnly) {
     producer.get()->closeAllStreams();
 }
 
+/* MB-24159 - Test to confirm a dcp stream backfill from an ephemeral bucket
+ * over a range which includes /no/ items doesn't cause the producer to
+ * segfault.
+ * */
+
+TEST_P(StreamTest, backfillGetsNoItems) {
+    if (engine->getConfiguration().getBucketType() == "ephemeral") {
+        setup_dcp_stream(DcpProducer::MutationType::KeyOnly);
+        store_item(vbid, "key", "value1");
+        store_item(vbid, "key", "value2");
+
+        active_stream_t aStream = dynamic_cast<ActiveStream *>(stream.get());
+
+        auto evb = std::shared_ptr<EphemeralVBucket>(
+                std::dynamic_pointer_cast<EphemeralVBucket>(vb0));
+        auto dcpbfm = DCPBackfillMemory(evb, aStream, 1, 1);
+        dcpbfm.run();
+        producer.get()->closeAllStreams();
+    }
+}
+
 /*
  * Test that when have a producer with MutationType set to KeyAndValue an active
  * stream created via a streamRequest returns false for isKeyOnly.

diff --git a/tests/module_tests/ephemeral_vb_test.cc b/tests/module_tests/ephemeral_vb_test.cc
index 1a32c84..ad9478e 100644 (file)
--- a/tests/module_tests/ephemeral_vb_test.cc
+++ b/tests/module_tests/ephemeral_vb_test.cc
@@ -187,8 +187,8 @@ TEST_F(EphemeralVBucketTest, Backfill) {
     setMany(keys, MutationStatus::WasClean);
 
     auto res = mockEpheVB->inMemoryBackfill(1, numItems);
-    EXPECT_EQ(ENGINE_SUCCESS, res.first);
-    EXPECT_EQ(numItems, res.second.size());
+    EXPECT_EQ(ENGINE_SUCCESS, std::get<0>(res));
+    EXPECT_EQ(numItems, std::get<1>(res).size());
 }
 
 TEST_F(EphemeralVBucketTest, UpdateDuringBackfill) {