diff --git a/engines/ep/src/ep_engine.cc b/engines/ep/src/ep_engine.cc index ee8641dec9..e3eb71b209 100644 --- a/engines/ep/src/ep_engine.cc +++ b/engines/ep/src/ep_engine.cc @@ -5716,11 +5716,14 @@ cb::engine_errc EventuallyPersistentEngine::deleteWithMeta( if (allowSanitizeValueInDeletion) { if (mcbp::datatype::is_xattr(datatype)) { - // Whatever we have in the value, just keep Xattrs - const auto valBuffer = std::string_view{ - reinterpret_cast(value.data()), - value.size()}; - value = {value.data(), cb::xattr::get_body_offset(valBuffer)}; + if (!value.empty()) { + // Whatever we have in the value, just keep Xattrs + const auto valBuffer = std::string_view{ + reinterpret_cast(value.data()), + value.size()}; + value = {value.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 c2866b59f8..0fca1d7d6d 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) { cb::engine_errc::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,20 +386,26 @@ TEST_F(WithMetaTest, SetWithMetaXattrWithEmptyPayload) { extMeta, 0 /*options*/); const auto* req = reinterpret_cast(packet.data()); - EXPECT_EQ(cb::engine_errc::success, - engine->setWithMeta(cookie, *req, addResponse)); + if (op == ClientOpcode::SetWithMeta) { + // Note: Before the fix this allows storing invalid paylaods. + EXPECT_EQ(cb::engine_errc::success, + engine->setWithMeta(cookie, *req, addResponse)); + } else { + // Note: Before the fix invalid payloads are detected but we throw and + // close the connection. + EXPECT_EQ(cb::engine_errc::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(cb::engine_errc::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*/); @@ -395,12 +414,21 @@ TEST_F(WithMetaTest, SetWithMetaXattrWithEmptyPayload) { auto gv = kvstore->get(makeDiskDocKey(key), vbid); EXPECT_EQ(cb::engine_errc::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)