wasphin commented on code in PR #3659:
URL: https://github.com/apache/brpc/pull/3659#discussion_r4444397213
##########
src/brpc/couchbase.cpp:
##########
@@ -124,9 +135,16 @@ bool CouchbaseManifestManager::jsonToCollectionManifest(
return false;
}
- // Parse JSON using RapidJSON
+ BUTIL_RAPIDJSON_NAMESPACE::Reader reader;
Review Comment:
Addressed in 883e8fe4. Moved the existing naming-service depth scan,
embedded-NUL rejection, and iterative DOM parsing into
src/brpc/details/json_util.h as ParseJsonWithDepthLimit(). Couchbase manifests
now use this shared helper. The naming-service entry points remain as wrappers,
preserving their existing behavior. A fresh CMake build succeeded; all 6
CouchbaseUnitTest tests and the existing
NamingServiceTest.naming_service_json_depth_limit test pass.
##########
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:
Addressed in 883e8fe4. The shared helper now defines kMaxJsonDepth = 100,
with an explicit comment that the count includes the root object/array: a
manifest root object may contain at most 99 nested containers below it. The
separate SAX handler and its duplicated literals have been removed.
##########
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:
Updated in 883e8fe4 to use explicit EXPECT_TRUE for 99 nested arrays under
the root and EXPECT_FALSE for 100 and 20000, with a comment explaining that the
root object plus 99 arrays is depth 100. I kept these fixed boundary inputs
independent of the implementation constant so an accidental change to the
100-level contract still fails the test rather than moving its inputs together
with the implementation. The original fixed-threshold expectation also checked
that contract; the new assertions make it easier to read. All 6
CouchbaseUnitTest tests pass.
--
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]
