MB-23530: Prevent replace with CAS on deleted item 61/76261/9
authorJames Harrison <00jamesh@gmail.com>
Tue, 4 Apr 2017 16:32:15 +0000 (17:32 +0100)
committerDave Rigby <daver@couchbase.com>
Thu, 13 Apr 2017 14:31:51 +0000 (14:31 +0000)
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>
src/vbucket.cc
tests/module_tests/kv_bucket_test.cc

index cf5906e..238f60c 100644 (file)
@@ -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) =
index 5eb970f..4739fd8 100644 (file)
@@ -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));