taepper commented on code in PR #51758:
URL: https://github.com/apache/arrow/pull/51758#discussion_r4196410654
##########
cpp/src/arrow/json/parser_test.cc:
##########
@@ -384,22 +384,33 @@ TEST(BlockParser, NestingDepth) {
};
const int kMaxDepth = 300; // hard-coded in parser.cc
+ ParseOptions options = ParseOptions::Defaults();
std::shared_ptr<Array> parsed;
- ASSERT_OK(ParseFromString(ParseOptions::Defaults(),
- deeply_nested_json_object(kMaxDepth), &parsed));
- ASSERT_OK(parsed->ValidateFull());
- ASSERT_OK(ParseFromString(ParseOptions::Defaults(),
deeply_nested_json_array(kMaxDepth),
- &parsed));
- ASSERT_OK(parsed->ValidateFull());
-
- EXPECT_RAISES_WITH_MESSAGE_THAT(
- Invalid, ::testing::HasSubstr("JSON too deeply nested: max nesting depth
is 300"),
- ParseFromString(ParseOptions::Defaults(),
deeply_nested_json_object(kMaxDepth + 1),
- &parsed));
- EXPECT_RAISES_WITH_MESSAGE_THAT(
- Invalid, ::testing::HasSubstr("JSON too deeply nested: max nesting depth
is 300"),
- ParseFromString(ParseOptions::Defaults(),
deeply_nested_json_array(kMaxDepth + 1),
- &parsed));
+
+ for (UnexpectedFieldBehavior unexpected_field_behavior :
+ {UnexpectedFieldBehavior::Ignore, UnexpectedFieldBehavior::InferType}) {
+ options.unexpected_field_behavior = unexpected_field_behavior;
+ options.explicit_schema = schema({{"not_here", int32()}});
+ ASSERT_OK(ParseFromString(options, deeply_nested_json_object(kMaxDepth),
&parsed));
+ ASSERT_OK(parsed->ValidateFull());
+ ASSERT_OK(ParseFromString(options, deeply_nested_json_array(kMaxDepth),
&parsed));
+ ASSERT_OK(parsed->ValidateFull());
+
+ // `kMaxDepth + 1` is the first value that triggers an error (but wouldn't
trigger
+ // a stack overflow otherwise).
+ // 100'000 would definitely trigger a stack overflow, validate that the
error is
+ // detected before that would happen.
Review Comment:
Note: in debug builds we probably did not run into stack overflows, but
`simdjson` assertions.
`simdjson::ondemand::parser` also has a depth limit (default 1024), and when
development checks are enabled, it will `assert` and thus possibly abort the
program, if we consume objects deeper than that limit:
https://github.com/simdjson/simdjson/blob/7fa77b1e4ba2a21c97d97adc48e4cd8b9d1e7faa/doc/basics.md?plain=1#L3985-L4032
I did not quite understand the purpose of this limit (other than aborting
debug code using the parser). I will open an issue in `simdjson` to find out a
bit more.
Not any change request, but just for context, which we should maybe document
somewhere?
##########
cpp/src/arrow/util/simdjson_internal.cc:
##########
@@ -538,47 +538,57 @@ Result<std::string> MinifyJson(std::string_view json) {
return minified;
}
-Status ConsumeJsonValue(simdjson::ondemand::value value) {
- return VisitJsonValue(
- value, ValidateJsonObject, ValidateJsonArray,
- [](std::string_view) { return Status::OK(); }, [](bool) { return
Status::OK(); },
- []() { return Status::OK(); }, [](int64_t) { return Status::OK(); },
- [](uint64_t) { return Status::OK(); }, [](double) { return Status::OK();
},
- [](simdjson::ondemand::value) { return Status::OK(); });
-}
-
-Status ValidateJsonObject(simdjson::ondemand::object object) {
+static Status ValidateJsonObject(simdjson::ondemand::object object, int
max_depth,
+ int depth) {
+ if (ARROW_PREDICT_FALSE(depth >= max_depth)) {
+ return Status::Invalid("JSON too deeply nested: max nesting depth is ",
max_depth);
+ }
for (auto field_result : object) {
ARROW_ASSIGN_OR_RAISE(
auto field, ResolveSimdjsonResult(field_result, "Failed to iterate
JSON object"));
-
- RETURN_NOT_OK(ConsumeJsonValue(field.value()));
+ RETURN_NOT_OK(ConsumeJsonValue(field.value(), max_depth, depth));
}
-
return Status::OK();
}
-Status ValidateJsonArray(simdjson::ondemand::array array) {
+static Status ValidateJsonArray(simdjson::ondemand::array array, int max_depth,
+ int depth) {
+ if (ARROW_PREDICT_FALSE(depth >= max_depth)) {
+ return Status::Invalid("JSON too deeply nested: max nesting depth is ",
max_depth);
+ }
for (auto element_result : array) {
ARROW_ASSIGN_OR_RAISE(
auto value,
ResolveSimdjsonResult(element_result, "Failed to iterate JSON array"));
-
- RETURN_NOT_OK(ConsumeJsonValue(value));
+ RETURN_NOT_OK(ConsumeJsonValue(value, max_depth, depth));
}
-
return Status::OK();
}
+Status ConsumeJsonValue(simdjson::ondemand::value value, int max_depth, int
depth) {
Review Comment:
Nit: As we are editing the signature: maybe we can rename this to
`ValidateJsonValue` for symmetry? As it is now, tt stands out between the other
functions for array, object and document, which can be confusing
--
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]
