wwbmmm commented on code in PR #3659:
URL: https://github.com/apache/brpc/pull/3659#discussion_r4430907825
##########
src/brpc/couchbase.cpp:
##########
@@ -124,9 +135,16 @@ bool CouchbaseManifestManager::jsonToCollectionManifest(
return false;
}
- // Parse JSON using RapidJSON
+ BUTIL_RAPIDJSON_NAMESPACE::Reader reader;
Review Comment:
[replied by brpc-oncall robot] This mirrors the mitigation that already
exists in `src/brpc/policy/naming_service_json.h`
(`kMaxNamingServiceJsonDepth`, `MaxJsonDepth()` and
`ParseNamingServiceJson()`): a 100-level depth cap, rejection of embedded NULs,
and `kParseIterativeFlag` parsing. Keeping two independent implementations of
the same protection makes it easy for them to drift apart. Please factor that
logic into a shared helper (e.g. move it out of the policy header into a common
json utility) and reuse it here.
##########
src/brpc/couchbase.cpp:
##########
@@ -50,6 +50,17 @@ constexpr uint32_t APPLE_VBUCKET_COUNT ALLOW_UNUSED = 64;
constexpr uint32_t DEFAULT_VBUCKET_COUNT ALLOW_UNUSED = 1024;
constexpr int CONNECTION_ID_SIZE = 33;
constexpr size_t RANDOM_ID_HEX_SIZE = 67; // 33 bytes * 2 + null terminator
+
+// Bound nesting before building the DOM, whose destruction is recursive.
+struct ManifestDepthHandler
+ : BUTIL_RAPIDJSON_NAMESPACE::BaseReaderHandler<
+ BUTIL_RAPIDJSON_NAMESPACE::UTF8<>, ManifestDepthHandler> {
+ unsigned depth = 0;
+ bool StartObject() { return ++depth <= 100; }
+ bool StartArray() { return ++depth <= 100; }
Review Comment:
[replied by brpc-oncall robot] Please replace the literal `100` with a named
constant (e.g. `kMaxManifestJsonDepth`) used by both
`StartObject()`/`StartArray()`, and state in a comment that the counted depth
includes the root container, i.e. a manifest may contain at most 99 nested
containers below the root. Right now that off-by-one semantic is only implicit
and is easy to break later.
##########
test/brpc_couchbase_unittest.cpp:
##########
@@ -40,6 +40,24 @@ namespace {
// Unit Tests - No Server Required
class CouchbaseUnitTest : public testing::Test {};
+TEST_F(CouchbaseUnitTest, ManifestJsonNesting) {
+ brpc::CouchbaseManifestManager manager;
+ brpc::CouchbaseManifestManager::CollectionManifest manifest;
+ const std::string prefix = "{\"uid\":\"1\",\"scopes\":[],\"extra\":";
+ for (size_t depth : {size_t(99), size_t(100), size_t(20000)}) {
+ const std::string json = prefix + std::string(depth, '[') + "0"
+ + std::string(depth, ']') + "}";
+ EXPECT_EQ(depth < 100, manager.jsonToCollectionManifest(json, &manifest))
Review Comment:
[replied by brpc-oncall robot] `EXPECT_EQ(depth < 100, ...)` re-encodes the
limit and its off-by-one semantics inside the test, so the test would happily
agree with a wrong threshold. Prefer the named constant used by the
implementation and spell the expectations out (e.g. `EXPECT_TRUE` for 99 and
`EXPECT_FALSE` for 100/20000), plus a short comment that 99 nested arrays under
the root object is exactly the boundary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
