Merge remote-tracking branch 'couchbase/mad-hatter'

* couchbase/mad-hatter:
  MB-47151: Tighten packet validator for subdoc multi
  MB-47105: Add ep_item_commit_failed check at test_stats_seqno

Change-Id: I144033d6fa9c05a4757592c1a4a85d314a81791e

daemon/subdocument_validators.cc

@@ -337,7 +337,7 @@ cb::mcbp::Status subdoc_replace_body_with_xattr_validator(Cookie& cookie) {
  * Validate the multipath spec. This may be a multi mutation or lookup
  *
  * @param cookie The cookie representing the command context
- * @param ptr Pointer to the first byte of the encoded spec
+ * @param blob The remainder of the unparsed data
  * @param traits The traits to use to validate the spec
  * @param spec_len [OUT] The number of bytes used by this spec
  * @param xattr [OUT] Did this spec reference an extended attribute
@@ -353,7 +353,7 @@ cb::mcbp::Status subdoc_replace_body_with_xattr_validator(Cookie& cookie) {
  */
 static cb::mcbp::Status is_valid_multipath_spec(
         Cookie& cookie,
-        const char* ptr,
+        std::string_view blob,
         const SubdocMultiCmdTraits traits,
         size_t& spec_len,
         bool& xattr,
@@ -368,22 +368,38 @@ static cb::mcbp::Status is_valid_multipath_spec(
     size_t pathlen;
     size_t valuelen;
     if (traits.is_mutator) {
-        auto* spec =reinterpret_cast<const protocol_binary_subdoc_multi_mutation_spec*>
-            (ptr);
+        auto* spec = reinterpret_cast<
+                const protocol_binary_subdoc_multi_mutation_spec*>(blob.data());
         headerlen = sizeof(*spec);
+        if (headerlen > blob.size()) {
+            cookie.setErrorContext("Multi mutation spec truncated");
+            return cb::mcbp::Status::Einval;
+        }
         opcode = cb::mcbp::ClientOpcode(spec->opcode);
         flags = protocol_binary_subdoc_flag(spec->flags);
         pathlen = ntohs(spec->pathlen);
         valuelen = ntohl(spec->valuelen);
 
+        if (headerlen + pathlen + valuelen > blob.size()) {
+            cookie.setErrorContext("Multi mutation path and value truncated");
+            return cb::mcbp::Status::Einval;
+        }
     } else {
-        auto* spec = reinterpret_cast<const protocol_binary_subdoc_multi_lookup_spec*>
-            (ptr);
+        auto* spec = reinterpret_cast<
+                const protocol_binary_subdoc_multi_lookup_spec*>(blob.data());
         headerlen = sizeof(*spec);
+        if (headerlen > blob.size()) {
+            cookie.setErrorContext("Multi lookup spec truncated");
+            return cb::mcbp::Status::Einval;
+        }
         opcode = cb::mcbp::ClientOpcode(spec->opcode);
         flags = protocol_binary_subdoc_flag(spec->flags);
         pathlen = ntohs(spec->pathlen);
         valuelen = 0;
+        if (headerlen + pathlen > blob.size()) {
+            cookie.setErrorContext("Multi lookup path truncated");
+            return cb::mcbp::Status::Einval;
+        }
     }
 
     xattr = (flags & SUBDOC_FLAG_XATTR_PATH);
@@ -434,9 +450,8 @@ static cb::mcbp::Status is_valid_multipath_spec(
         return cb::mcbp::Status::Einval;
     }
 
-    std::string_view path{ptr + headerlen, pathlen};
-
-    std::string_view macro{ptr + headerlen + pathlen, valuelen};
+    std::string_view path{blob.data() + headerlen, pathlen};
+    std::string_view macro{blob.data() + headerlen + pathlen, valuelen};
 
     const auto status = validate_xattr_section(cookie,
                                                traits.is_mutator,
@@ -564,14 +579,15 @@ static cb::mcbp::Status subdoc_multi_validator(
         // true if the spec command needs to be alone in operating on the body
         bool is_isolationist;
 
-        const auto status = is_valid_multipath_spec(cookie,
-                                                    body_ptr + body_validated,
-                                                    traits,
-                                                    spec_len,
-                                                    is_xattr,
-                                                    xattr_key,
-                                                    doc_flags,
-                                                    is_isolationist);
+        const auto status = is_valid_multipath_spec(
+                cookie,
+                {body_ptr + body_validated, value.size() - body_validated},
+                traits,
+                spec_len,
+                is_xattr,
+                xattr_key,
+                doc_flags,
+                is_isolationist);
         if (status != cb::mcbp::Status::Success) {
             return status;
         }

engines/ep/tests/ep_testsuite.cc

@@ -1948,6 +1948,11 @@ static enum test_result test_stats_seqno(EngineIface* h) {
     }
     wait_for_flusher_to_settle(h);
 
+    // MB-47105: help identifing if anything related to MB-37920
+    if (isPersistentBucket(h)) {
+        checkeq(0, get_int_stat(h, "ep_item_commit_failed"), "Flush failures");
+    }
+
     checkeq(100,
             get_int_stat(h, "vb_0:high_seqno", "vbucket-seqno"),
             "Invalid seqno");

tests/testapp/testapp_regression.cc

@@ -534,3 +534,21 @@ TEST_P(RegressionTest, MB16197_malformed_sasl_auth) {
             cb::mcbp::ClientOpcode::SaslAuth, "PLAIN", std::string{"\0", 1}});
     ASSERT_EQ(cb::mcbp::Status::Einval, rsp.getStatus());
 }
+
+/// Check the subdoc validator also check the size of the internal spec
+/// elements
+TEST_P(RegressionTest, MB47151) {
+    Frame frame;
+    frame.payload = std::vector<uint8_t>{
+            {0x80, 0xd0, 0x00, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+             0x00, 0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x46,
+             0x46, 0x46, 0x46, 0x46, 0x24, 0x00, 0x09, 0x00, 0x04, 0x00}};
+
+    auto& conn = getConnection();
+    conn.sendFrame(frame);
+    BinprotResponse rsp;
+    conn.recvResponse(rsp);
+    EXPECT_EQ(cb::mcbp::Status::Einval, rsp.getStatus()) << rsp.getDataString();
+    EXPECT_EQ(R"({"error":{"context":"Multi lookup spec truncated"}})",
+              rsp.getDataString());
+}