MB-23530: Prevent replace with CAS on deleted item

Against an ephemeral bucket or full eviction persistent bucket, a
replace operation could erroneously succeed if the correct CAS value was
given. A replace with a CAS resolves internally to OPERATION_CAS, which
is treated in the same manner as OPERATION_SET.

Prior to this change, we checked if an item was previously deleted if
the CAS did _not_ match, but even if the CAS is /correct/ a replace with
a CAS should fail if the old item is deleted.

The set should only be rejected if the new item is /not/ deleted, to
avoid breaking the delete implementation described in
http://review.couchbase.org/#/c/76119.

Change-Id: Id1eeaeb5326b7a143c7213f07c774f02623bff8d
Reviewed-on: http://review.couchbase.org/76261
Reviewed-by: Daniel Owen <owend@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>

diff --git a/src/vbucket.cc b/src/vbucket.cc
index cf5906e..238f60c 100644 (file)
--- a/src/vbucket.cc
+++ b/src/vbucket.cc
@@ -1990,7 +1990,18 @@ std::pair<MutationStatus, VBNotifyCtx> VBucket::processSet(
         }
         if (!hasMetaData) {
             itm.setRevSeqno(v->getRevSeqno() + 1);
+            /* MB-23530: We must ensure that a replace operation (i.e.
+             * set with a CAS) /fails/ if the old document is deleted; it
+             * logically "doesn't exist". However, if the new value is deleted
+             * this op is a /delete/ with a CAS and we must permit a
+             * deleted -> deleted transition for Deleted Bodies.
+             */
+            if (cas && (v->isDeleted() || v->isTempDeletedItem()) &&
+                !itm.isDeleted()) {
+                return {MutationStatus::NotFound, VBNotifyCtx()};
+            }
         }
+
         MutationStatus status;
         VBNotifyCtx notifyCtx;
         std::tie(v, status, notifyCtx) =

diff --git a/tests/module_tests/kv_bucket_test.cc b/tests/module_tests/kv_bucket_test.cc
index 5eb970f..4739fd8 100644 (file)
--- a/tests/module_tests/kv_bucket_test.cc
+++ b/tests/module_tests/kv_bucket_test.cc
@@ -329,13 +329,6 @@ TEST_P(KVBucketParamTest, SetCASDeleted) {
     item2.setCas(cas);
 
     // Store item
-    if (engine->getConfiguration().getBucketType() == "ephemeral" ||
-        engine->getConfiguration().getItemEvictionPolicy() == "full_eviction") {
-        // This currently fails due to MB-23712
-        // TODO: Reenable this check once fixed
-        return;
-    }
-
     if (engine->getConfiguration().getItemEvictionPolicy() ==
                "full_eviction") {
         EXPECT_EQ(ENGINE_EWOULDBLOCK, store->set(item2, cookie));