MB-31141: Account for nmeta in deleteWithMeta

When calculating the item size in deleteWithMeta we must
ensure nmeta is removed from the calculated size.

Tests added for both set and del withMeta

Change-Id: Iab31591c521e9556d436905be0177da773afc058
Reviewed-on: http://review.couchbase.org/99152
Tested-by: Build Bot <[email protected]>
Well-Formed: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: jimwwalker

engines/ep/src/ep_engine.cc

@@ -5122,9 +5122,12 @@ ENGINE_ERROR_CODE EventuallyPersistentEngine::deleteWithMeta(
         keyOffset += 2; // 2 bytes for nmeta
         nmeta = ntohs(nmeta);
         if (nmeta > 0) {
-            emd = cb::const_byte_buffer(
-                    request->bytes + sizeof(request->bytes) + nkey + keyOffset,
-                    nmeta);
+            // Correct the vallen
+            vallen -= nmeta;
+            emd = cb::const_byte_buffer(request->bytes +
+                                                sizeof(request->bytes) + nkey +
+                                                keyOffset + vallen,
+                                        nmeta);
         }
     }
 

engines/ep/src/stored-value.cc

@@ -433,6 +433,7 @@ std::ostream& operator<<(std::ostream& os, const StoredValue& sv) {
 
     // seqno, revid, expiry / purge time
     os << "seq:" << sv.getBySeqno() << " rev:" << sv.getRevSeqno();
+    os << " cas:" << sv.getCas();
     os << " key:\"" << sv.getKey() << "\"";
     if (sv.isOrdered() && sv.isDeleted()) {
         os << " del_time:" << sv.lock_expiry_or_delete_time;

engines/ep/tests/module_tests/evp_store_with_meta.cc

@@ -148,7 +148,8 @@ class WithMetaTest : public SingleThreadedEPBucketTest {
                int options,
                protocol_binary_response_status expectedResponseStatus,
                const std::string& key,
-               const std::string& value) {
+               const std::string& value,
+               const std::vector<char>& emd) {
         auto swm = buildWithMetaPacket(op,
                                        0 /*datatype*/,
                                        vbid,
@@ -157,7 +158,7 @@ class WithMetaTest : public SingleThreadedEPBucketTest {
                                        itemMeta,
                                        key,
                                        value,
-                                       {},
+                                       emd,
                                        options);
         if (expectedResponseStatus == PROTOCOL_BINARY_RESPONSE_NOT_MY_VBUCKET) {
             EXPECT_EQ(ENGINE_NOT_MY_VBUCKET, callEngine(op, swm));
@@ -175,18 +176,14 @@ class WithMetaTest : public SingleThreadedEPBucketTest {
                        int options,
                        bool withValue,
                        protocol_binary_response_status expectedResponseStatus,
-                       ENGINE_ERROR_CODE expectedGetReturnValue) {
+                       ENGINE_ERROR_CODE expectedGetReturnValue,
+                       const std::vector<char>& emd = {}) {
         std::string key = "mykey";
         std::string value;
         if (withValue) {
             value = createXattrValue("myvalue"); // xattr but stored as raw
         }
-        oneOp(op,
-              itemMeta,
-              options,
-              expectedResponseStatus,
-              key,
-              value);
+        oneOp(op, itemMeta, options, expectedResponseStatus, key, value, emd);
         checkGetItem(key, value, itemMeta, expectedGetReturnValue);
     }
 
@@ -655,7 +652,7 @@ void WithMetaTest::conflict_lose(protocol_binary_command op,
     EXPECT_EQ(PROTOCOL_BINARY_RESPONSE_SUCCESS, getAddResponseStatus());
 
     for (const auto& td : testData) {
-        oneOp(op, td.meta, options, td.expectedStatus, key, value);
+        oneOp(op, td.meta, options, td.expectedStatus, key, value, {});
     }
 }
 
@@ -1359,6 +1356,63 @@ TEST_P(DelWithMetaTest, setting_deleteTime) {
     EXPECT_EQ(itemMeta.exptime, metadata.exptime);
 }
 
+TEST_P(DelWithMetaTest, MB_31141) {
+    ItemMetaData itemMeta{0xdeadbeef, 0xf00dcafe, 0xfacefeed, expiry};
+    // Do a delete with valid extended meta
+    // see - ep-engine/docs/protocol/del_with_meta.md
+    oneOpAndCheck(op,
+                  itemMeta,
+                  0, // no-options
+                  withValue,
+                  PROTOCOL_BINARY_RESPONSE_SUCCESS,
+                  withValue ? ENGINE_SUCCESS : ENGINE_EWOULDBLOCK,
+                  {0x01, 0x01, 0x00, 0x01, 0x01});
+
+    EXPECT_EQ(std::make_pair(false, size_t(1)),
+              getEPBucket().flushVBucket(vbid));
+
+    auto options = get_options_t(QUEUE_BG_FETCH | GET_DELETED_VALUE);
+    auto result = store->get(
+            {"mykey", DocNamespace::DefaultCollection}, vbid, cookie, options);
+    EXPECT_EQ(ENGINE_EWOULDBLOCK, result.getStatus());
+
+    // Run the BGFetcher task
+    MockGlobalTask mockTask(engine->getTaskable(), TaskId::MultiBGFetcherTask);
+    store->getVBucket(vbid)->getShard()->getBgFetcher()->run(&mockTask);
+
+    result = store->get(
+            {"mykey", DocNamespace::DefaultCollection}, vbid, cookie, options);
+    ASSERT_EQ(ENGINE_SUCCESS, result.getStatus());
+
+    // Before the fix 5.0+ could of left the value as 5, the size of the
+    // extended metadata
+    int expectedSize = withValue ? 275 : 0;
+    EXPECT_EQ(expectedSize, result.item->getNBytes());
+}
+
+TEST_P(AddSetWithMetaTest, MB_31141) {
+    ItemMetaData itemMeta{0xdeadbeef, 0xf00dcafe, 0xfacefeed, expiry};
+    // Do a set/add with valid extended meta
+    // see - ep-engine/docs/protocol/del_with_meta.md
+    oneOpAndCheck(GetParam(),
+                  itemMeta,
+                  0, // no-options
+                  true,
+                  PROTOCOL_BINARY_RESPONSE_SUCCESS,
+                  ENGINE_SUCCESS,
+                  {0x01, 0x01, 0x00, 0x01, 0x01});
+
+    EXPECT_EQ(std::make_pair(false, size_t(1)),
+              getEPBucket().flushVBucket(vbid));
+
+    auto options = get_options_t(QUEUE_BG_FETCH);
+    auto result = store->get(
+            {"mykey", DocNamespace::DefaultCollection}, vbid, cookie, options);
+    ASSERT_EQ(ENGINE_SUCCESS, result.getStatus());
+    // Only the value should come back
+    EXPECT_EQ(275, result.item->getNBytes());
+}
+
 auto opcodeValues = ::testing::Values(PROTOCOL_BINARY_CMD_SET_WITH_META,
                                       PROTOCOL_BINARY_CMD_SETQ_WITH_META,
                                       PROTOCOL_BINARY_CMD_ADD_WITH_META,