From 21e5ea537d44bcb89936958eede680aae3f44359 Mon Sep 17 00:00:00 2001 From: Paolo Cocchi Date: Thu, 25 Feb 2021 14:49:13 +0100 Subject: [PATCH] MB-44534: DelWithMeta fixes the datatype for empty value with dt:xattr Datatype Xattr is invalid for empty payloads. The current behaviour that the invalid payload is detected but we throw and close the connection. Fix is to just proceed the execution, KVBucket::deleteWithMeta will handle the datatype. Change-Id: If086ba71606401f72d165ee9d4c61c860164cc00 Reviewed-on: http://review.couchbase.org/c/kv_engine/+/147079 Tested-by: Build Bot Reviewed-by: Trond Norbye Well-Formed: Build Bot --- engines/ep/src/ep_engine.cc | 14 ++-- .../tests/module_tests/evp_store_with_meta.cc | 69 +++++++++++++------ 2 files changed, 57 insertions(+), 26 deletions(-) diff --git a/engines/ep/src/ep_engine.cc b/engines/ep/src/ep_engine.cc index 74f2f93b76..53ae408256 100644 --- a/engines/ep/src/ep_engine.cc +++ b/engines/ep/src/ep_engine.cc @@ -5564,12 +5564,14 @@ ENGINE_ERROR_CODE EventuallyPersistentEngine::deleteWithMeta( if (allowSanitizeValueInDeletion) { if (mcbp::datatype::is_xattr(datatype)) { - // Whatever we have in the value, just keep Xattrs - const auto valBuffer = cb::const_char_buffer{ - reinterpret_cast(value.data()), - value.size()}; - value = {reinterpret_cast(valBuffer.data()), - cb::xattr::get_body_offset(valBuffer)}; + if (!value.empty()) { + // Whatever we have in the value, just keep Xattrs + const auto valBuffer = cb::const_char_buffer{ + reinterpret_cast(value.data()), + value.size()}; + value = {reinterpret_cast(valBuffer.data()), + cb::xattr::get_body_offset(valBuffer)}; + } } else { // We may have nothing or only a Body, remove everything value = {}; diff --git a/engines/ep/tests/module_tests/evp_store_with_meta.cc b/engines/ep/tests/module_tests/evp_store_with_meta.cc index 77f1698da0..4caee62047 100644 --- a/engines/ep/tests/module_tests/evp_store_with_meta.cc +++ b/engines/ep/tests/module_tests/evp_store_with_meta.cc @@ -248,6 +248,16 @@ class WithMetaTest : public SingleThreadedEPBucketTest { void conflict_lose_xattr(cb::mcbp::ClientOpcode op, int options, bool withValue); + + /** + * The test verifies that Set/DelWithMeta sanitizes invalid payloads like + * {datatype:xattr, value-size=0} by resetting to datatype:raw. + * + * @param op The *WithMeta operation under test + * @throws std::invalid_argument if a different kind of operation is passed + */ + void testWithMetaXattrWithEmptyPayload(cb::mcbp::ClientOpcode op); + /** * Initialise an expiry value which allows us to set/get items without them * expiring, i.e. a few years of expiry wiggle room @@ -347,22 +357,25 @@ TEST_F(WithMetaTest, basicAdd) { ENGINE_SUCCESS); // can still get the key } -/** - * The test verifies that SetWithMeta sanitizes invalid payloads like - * {datatype:xattr, value-size=0} by resetting datatype:raw - */ -TEST_F(WithMetaTest, SetWithMetaXattrWithEmptyPayload) { +void WithMetaTest::testWithMetaXattrWithEmptyPayload( + cb::mcbp::ClientOpcode op) { + using ClientOpcode = cb::mcbp::ClientOpcode; + if (op != ClientOpcode::SetWithMeta && op != ClientOpcode::DelWithMeta) { + throw std::invalid_argument("testWithMetaXattrWithEmptyPayload: " + + std::to_string(static_cast(op))); + } + mock_set_datatype_support(cookie, PROTOCOL_BINARY_DATATYPE_JSON); - const auto& vb = store->getVBucket(vbid); - ASSERT_EQ(0, vb->getHighSeqno()); + auto& vb = *store->getVBucket(vbid); + ASSERT_EQ(0, vb.getHighSeqno()); const std::string key = "key"; const std::string value; // Empty value const auto datatype = PROTOCOL_BINARY_DATATYPE_XATTR; ItemMetaData meta{1 /*cas*/, 1 /*revSeqno*/, 0 /*flags*/, 0 /*expiry*/}; const std::vector extMeta = {}; - const auto packet = buildWithMetaPacket(cb::mcbp::ClientOpcode::SetWithMeta, + const auto packet = buildWithMetaPacket(op, datatype, vbid, 0 /*opaque*/, @@ -373,19 +386,26 @@ TEST_F(WithMetaTest, SetWithMetaXattrWithEmptyPayload) { extMeta, 0 /*options*/); const auto* req = reinterpret_cast(packet.data()); - EXPECT_EQ(ENGINE_SUCCESS, engine->setWithMeta(cookie, *req, addResponse)); + if (op == ClientOpcode::SetWithMeta) { + // Note: Before the fix this allows storing invalid paylaods. + EXPECT_EQ(ENGINE_SUCCESS, + engine->setWithMeta(cookie, *req, addResponse)); + } else { + // Note: Before the fix invalid payloads are detected but we throw and + // close the connection. + EXPECT_EQ(ENGINE_SUCCESS, + engine->deleteWithMeta(cookie, *req, addResponse)); + } // Check in memory - const auto res = store->get({key, DocKeyEncodesCollectionId::No}, - vbid, - cookie, - get_options_t::NONE); - EXPECT_EQ(ENGINE_SUCCESS, res.getStatus()); - EXPECT_EQ(1, res.item->getBySeqno()); - EXPECT_FALSE(res.item->isDeleted()); - EXPECT_EQ(PROTOCOL_BINARY_RAW_BYTES, res.item->getDataType()); - EXPECT_NE(nullptr, res.item->getData()); - EXPECT_EQ(0, res.item->getNBytes()); + { + const auto res = vb.ht.findOnlyCommitted(makeStoredDocKey(key)); + EXPECT_TRUE(res.storedValue); + EXPECT_EQ(1, res.storedValue->getBySeqno()); + EXPECT_EQ(op == ClientOpcode::DelWithMeta ? true : false, + res.storedValue->isDeleted()); + EXPECT_EQ(PROTOCOL_BINARY_RAW_BYTES, res.storedValue->getDatatype()); + } // Check on disk flush_vbucket_to_disk(vbid, 1 /*expectedNumFlushed*/); @@ -394,12 +414,21 @@ TEST_F(WithMetaTest, SetWithMetaXattrWithEmptyPayload) { auto gv = kvstore->get(makeDiskDocKey(key), vbid); EXPECT_EQ(ENGINE_SUCCESS, gv.getStatus()); EXPECT_EQ(1, gv.item->getBySeqno()); - EXPECT_FALSE(gv.item->isDeleted()); + EXPECT_EQ(op == ClientOpcode::DelWithMeta ? true : false, + gv.item->isDeleted()); EXPECT_EQ(PROTOCOL_BINARY_RAW_BYTES, gv.item->getDataType()); EXPECT_TRUE(gv.item->getValue()); EXPECT_EQ(0, gv.item->getNBytes()); } +TEST_F(WithMetaTest, SetWithMetaXattrWithEmptyPayload) { + testWithMetaXattrWithEmptyPayload(cb::mcbp::ClientOpcode::SetWithMeta); +} + +TEST_F(WithMetaTest, DelWithMetaXattrWithEmptyPayload) { + testWithMetaXattrWithEmptyPayload(cb::mcbp::ClientOpcode::DelWithMeta); +} + TEST_P(DelWithMetaTest, basic) { ItemMetaData itemMeta{0xdeadbeef, 0xf00dcafe, 0xfacefeed, expiry}; // A delete_w_meta against an empty bucket queues a BGFetch (get = ewblock)