MB-41718: Fix crash due to vbucket nullptr dereference

Currently in DcpProducer::getHighSeqnoOfCollections() there is the
possibility that we can end up dereferencing a nullptr as we don't check
if getVBucket() returns a valid ptr. To fix this refactor
getHighSeqnoOfCollections() to take a ref to a VBucket object as this
method is only ever called within DcpProducer::streamRequest() which
gains a valid VBucketPtr already. Also move getHighSeqnoOfCollections()
to the protected namespace as its only ever used within DcpProducer.

Change-Id: I74f86662929503d5d81f04296b4ce30cf1870920
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/138055
Tested-by: Build Bot <[email protected]>
Reviewed-by: Jim Walker <[email protected]>

Author: rdemellow

engines/ep/src/dcp/producer.cc

@@ -408,16 +408,16 @@ ENGINE_ERROR_CODE DcpProducer::streamRequest(
         start_seqno = static_cast<uint64_t>(vb->getHighSeqno());
     }
 
-    std::pair<bool, std::string> need_rollback = vb->failovers->needsRollback(
-            start_seqno,
-            vb->getHighSeqno(),
-            vbucket_uuid,
-            snap_start_seqno,
-            snap_end_seqno,
-            vb->getPurgeSeqno(),
-            flags & DCP_ADD_STREAM_STRICT_VBUUID,
-            getHighSeqnoOfCollections(filter, vbucket),
-            rollback_seqno);
+    std::pair<bool, std::string> need_rollback =
+            vb->failovers->needsRollback(start_seqno,
+                                         vb->getHighSeqno(),
+                                         vbucket_uuid,
+                                         snap_start_seqno,
+                                         snap_end_seqno,
+                                         vb->getPurgeSeqno(),
+                                         flags & DCP_ADD_STREAM_STRICT_VBUUID,
+                                         getHighSeqnoOfCollections(filter, *vb),
+                                         rollback_seqno);
 
     if (need_rollback.first) {
         logger->warn(
@@ -2065,20 +2065,19 @@ bool DcpProducer::isOutOfOrderSnapshotsEnabled() const {
 }
 
 std::optional<uint64_t> DcpProducer::getHighSeqnoOfCollections(
-        const Collections::VB::Filter& filter, Vbid vbucket) const {
+        const Collections::VB::Filter& filter, VBucket& vbucket) const {
     if (filter.isPassThroughFilter()) {
         return std::nullopt;
     }
 
     uint64_t maxHighSeqno = 0;
-    auto vb = engine_.getVBucket(vbucket);
     for (auto& coll : filter) {
-        auto handle = vb->getManifest().lock(coll.first);
+        auto handle = vbucket.getManifest().lock(coll.first);
         if (!handle.valid()) {
             logger->warn(
                     "({}) DcpProducer::getHighSeqnoOfCollections(): failed "
                     "to find collectionID:{}, scopeID:{}, in the manifest",
-                    vbucket,
+                    vbucket.getId(),
                     coll.first,
                     coll.second);
             // return std::nullopt as we don't want our caller to use rollback

engines/ep/src/dcp/producer.h

@@ -371,6 +371,7 @@ class DcpProducer : public ConnHandler,
 
     bool isOutOfOrderSnapshotsEnabled() const;
 
+protected:
     /**
      * For filtered DCP, method returns the maximum of all the high-seqnos of
      * the collections in the filter. std::nullopt is returned for and
@@ -380,9 +381,8 @@ class DcpProducer : public ConnHandler,
      * @return high seqno of all the collection in the filter.
      */
     std::optional<uint64_t> getHighSeqnoOfCollections(
-            const Collections::VB::Filter& filter, Vbid vbucket) const;
+            const Collections::VB::Filter& filter, VBucket& vbucket) const;
 
-protected:
     /** We may disconnect if noop messages are enabled and the last time we
      *  received any message (including a noop) exceeds the dcpTimeout.
      *  Returns ENGINE_DISCONNECT if noop messages are enabled and the timeout