diff --git a/cpp/src/arrow/json/parser.cc b/cpp/src/arrow/json/parser.cc index 2d2363fe4318..69373d07fb63 100644 --- a/cpp/src/arrow/json/parser.cc +++ b/cpp/src/arrow/json/parser.cc @@ -17,7 +17,6 @@ #include "arrow/json/parser.h" -#include #include #include #include @@ -47,11 +46,6 @@ namespace json { namespace sj = simdjson::ondemand; -template -static Status ParseError(T&&... t) { - return Status::Invalid("JSON parse error: ", std::forward(t)...); -} - const std::string& Kind::Name(Kind::type kind) { static const std::string names[] = { "null", "boolean", "number", "string", "array", "object", "number_or_string", @@ -121,6 +115,18 @@ Status Kind::ForType(const DataType& type, Kind::type* kind) { return VisitTypeInline(type, &visitor); } +namespace { + +// Tuned to not crash on any of our CI platforms in release mode. +// This may seem low but some platforms have very low thread stack sizes +// (128 kiB on musllinux). +constexpr int kMaxNestingDepth = 100; + +template +Status ParseError(T&&... t) { + return Status::Invalid("JSON parse error: ", std::forward(t)...); +} + /// \brief ArrayBuilder for parsed but unconverted arrays template class RawArrayBuilder; @@ -305,20 +311,17 @@ class RawArrayBuilder { return null_bitmap_builder_.Append(count, false); } - Status Finish(std::function*)> finish_child, - std::shared_ptr* out) { + Status Finish(std::shared_ptr child_values, std::shared_ptr* out) { RETURN_NOT_OK(offset_builder_.Append(offset_)); auto size = length(); auto null_count = null_bitmap_builder_.false_count(); std::shared_ptr offsets, null_bitmap; RETURN_NOT_OK(offset_builder_.Finish(&offsets)); RETURN_NOT_OK(null_bitmap_builder_.Finish(&null_bitmap)); - std::shared_ptr values; - RETURN_NOT_OK(finish_child(value_builder_, &values)); - auto type = list(field("item", values->type(), value_builder_.nullable, + auto type = list(field("item", child_values->type(), value_builder_.nullable, Kind::Tag(value_builder_.kind))); - *out = MakeArray(ArrayData::Make(type, size, {null_bitmap, offsets}, {values->data()}, - null_count)); + *out = MakeArray(ArrayData::Make(type, size, {null_bitmap, offsets}, + {child_values->data()}, null_count)); return Status::OK(); } @@ -404,21 +407,18 @@ class RawArrayBuilder { field_infos_[index].builder = builder; } - Status Finish(std::function*)> finish_child, + Status Finish(std::vector> child_data, std::shared_ptr* out) { auto size = length(); auto null_count = null_bitmap_builder_.false_count(); std::shared_ptr null_bitmap; RETURN_NOT_OK(null_bitmap_builder_.Finish(&null_bitmap)); + DCHECK_EQ(child_data.size(), static_cast(num_fields())); std::vector> fields(num_fields()); - std::vector> child_data(num_fields()); for (int i = 0; i < num_fields(); ++i) { const auto& info = field_infos_[i]; - std::shared_ptr field_values; - RETURN_NOT_OK(finish_child(info.builder, &field_values)); - child_data[i] = field_values->data(); - fields[i] = field(std::string(info.name), field_values->type(), + fields[i] = field(std::string(info.name), child_data[i]->type, info.builder.nullable, Kind::Tag(info.builder.kind)); } @@ -581,10 +581,6 @@ class RawBuilderSet { Status Finish(const std::shared_ptr& scalar_values, BuilderPtr builder, std::shared_ptr* out) { - auto finish_children = [this, &scalar_values](BuilderPtr child, - std::shared_ptr* out) { - return Finish(scalar_values, child, out); - }; switch (builder.kind) { case Kind::kNull: { auto length = static_cast(builder.index); @@ -603,11 +599,25 @@ class RawBuilderSet { case Kind::kNumberOrString: return FinishScalar(scalar_values, Cast(builder), out); - case Kind::kArray: - return Cast(builder)->Finish(std::move(finish_children), out); + case Kind::kArray: { + auto array_builder = Cast(builder); + auto child_builder = array_builder->value_builder(); + std::shared_ptr child_values; + RETURN_NOT_OK(Finish(scalar_values, child_builder, &child_values)); + return array_builder->Finish(std::move(child_values), out); + } - case Kind::kObject: - return Cast(builder)->Finish(std::move(finish_children), out); + case Kind::kObject: { + auto object_builder = Cast(builder); + std::vector> child_data(object_builder->num_fields()); + for (int i = 0; i < object_builder->num_fields(); ++i) { + auto child_builder = object_builder->field_builder(i); + std::shared_ptr child_values; + RETURN_NOT_OK(Finish(scalar_values, child_builder, &child_values)); + child_data[i] = child_values->data(); + } + return object_builder->Finish(std::move(child_data), out); + } default: return Status::NotImplemented("invalid builder kind"); @@ -662,13 +672,15 @@ class ParseImpl : public BlockParser { return builder_set_.AppendNull(builder_stack_.back(), field_index_, builder_); } - Status HandleUnexpectedField(std::string_view key, sj::value value) { + Status HandleUnexpectedField(std::string_view key, sj::value& value) { switch (unexpected_field_behavior_) { case UnexpectedFieldBehavior::Error: return ParseError("unexpected field"); case UnexpectedFieldBehavior::Ignore: - return internal::ConsumeJsonValue(value); + return internal::ValidateJsonValue( + std::move(value), kMaxNestingDepth, + /*depth=*/static_cast(builder_stack_.size() - 1)); case UnexpectedFieldBehavior::InferType: { // If an unexpected field is encountered, add a NullBuilder with leading nulls. @@ -833,7 +845,7 @@ class ParseImpl : public BlockParser { return Status::OK(); } - Status ParseValue(sj::value value) { + Status ParseValue(sj::value& value) { ARROW_ASSIGN_OR_RAISE(auto type, arrow::internal::ResolveSimdjsonResult( value.type(), "Failed to determine JSON type")); @@ -887,25 +899,22 @@ class ParseImpl : public BlockParser { return Status::OK(); } - Status ParseArray(sj::value value) { + Status ParseArray(sj::value& value) { constexpr auto kind = Kind::kArray; if (ARROW_PREDICT_FALSE(builder_.kind != kind)) { return IllegallyChangedTo(kind); } - StartNested(); - + RETURN_NOT_OK(StartNested()); builder_ = Cast(builder_)->value_builder(); ARROW_ASSIGN_OR_RAISE(auto array, arrow::internal::ResolveSimdjsonResult( value.get_array(), "Failed to get JSON array")); - int64_t size = 0; - for (auto element_result : array) { - ARROW_ASSIGN_OR_RAISE(auto element, - arrow::internal::ResolveSimdjsonResult( - element_result, "Failed to iterate JSON array")); + ARROW_ASSIGN_OR_RAISE( + auto element, arrow::internal::ResolveSimdjsonResult( + std::move(element_result), "Failed to iterate JSON array")); RETURN_NOT_OK(ParseValue(element)); ++size; @@ -918,7 +927,7 @@ class ParseImpl : public BlockParser { return list_builder->Append(static_cast(size)); } - Status ParseObject(sj::value value) { + Status ParseObject(sj::value& value) { constexpr auto kind = Kind::kObject; if (ARROW_PREDICT_FALSE(builder_.kind != kind)) { return IllegallyChangedTo(kind); @@ -926,7 +935,7 @@ class ParseImpl : public BlockParser { auto struct_builder = Cast(builder_); absent_fields_stack_.Push(struct_builder->num_fields(), true); - StartNested(); + RETURN_NOT_OK(StartNested()); RETURN_NOT_OK(struct_builder->Append()); ARROW_ASSIGN_OR_RAISE( @@ -934,10 +943,10 @@ class ParseImpl : public BlockParser { "Failed to get JSON object")); for (auto field_result : object) { - ARROW_ASSIGN_OR_RAISE( - auto field, - arrow::internal::ResolveSimdjsonResult( - field_result, "JSON parse error: Failed to iterate JSON object")); + ARROW_ASSIGN_OR_RAISE(auto field, + arrow::internal::ResolveSimdjsonResult( + std::move(field_result), + "JSON parse error: Failed to iterate JSON object")); ARROW_ASSIGN_OR_RAISE(auto key, arrow::internal::ResolveSimdjsonResult( @@ -964,7 +973,7 @@ class ParseImpl : public BlockParser { return Status::OK(); } - Status ParseObjectField(std::string_view key, sj::value value) { + Status ParseObjectField(std::string_view key, sj::value& value) { auto parent = Cast(builder_stack_.back()); field_index_ = parent->GetFieldIndex(key); if (ARROW_PREDICT_FALSE(field_index_ == -1)) { @@ -1002,10 +1011,16 @@ class ParseImpl : public BlockParser { /// helper method for ParseArray and ParseObject /// adds the current builder to a stack so its /// children can be visited and parsed. - void StartNested() { + Status StartNested() { + if (ARROW_PREDICT_FALSE(builder_stack_.size() >= + static_cast(kMaxNestingDepth))) { + return Status::Invalid("JSON too deeply nested: max nesting depth is ", + kMaxNestingDepth); + } field_index_stack_.push_back(field_index_); field_index_ = -1; builder_stack_.push_back(builder_); + return Status::OK(); } /// helper method for EndArray and EndObject @@ -1049,6 +1064,8 @@ class ParseImpl : public BlockParser { sj::parser parser_; }; +} // namespace + Status BlockParser::Make(MemoryPool* pool, const ParseOptions& options, std::unique_ptr* out) { DCHECK(options.unexpected_field_behavior == UnexpectedFieldBehavior::InferType || diff --git a/cpp/src/arrow/json/parser_test.cc b/cpp/src/arrow/json/parser_test.cc index 28c54b3c5cd5..6c32fd7cab3d 100644 --- a/cpp/src/arrow/json/parser_test.cc +++ b/cpp/src/arrow/json/parser_test.cc @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -355,5 +356,62 @@ TEST(BlockParser, NullTypeRejectsNonNullUnderError) { ASSERT_RAISES(Invalid, ParseFromString(options, R"({"a": 5})", &parsed)); } +TEST(BlockParser, NestingDepth) { + auto deeply_nested_json_object = [](int depth) { + std::stringstream ss; + for (int i = 0; i < depth; ++i) { + ss << "{\"a\":"; + } + ss << "1"; + for (int i = 0; i < depth; ++i) { + ss << "}"; + } + return std::move(ss).str(); + }; + + auto deeply_nested_json_array = [](int depth) { + std::stringstream ss; + ss << "{\"a\":"; + for (int i = 0; i < depth - 1; ++i) { + ss << "["; + } + ss << "1"; + for (int i = 0; i < depth - 1; ++i) { + ss << "]"; + } + ss << "}"; + return std::move(ss).str(); + }; + + const int kMaxDepth = 100; // hard-coded in parser.cc + ParseOptions options = ParseOptions::Defaults(); + std::shared_ptr 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. + for (int depth : {kMaxDepth + 1, 100'000}) { + EXPECT_RAISES_WITH_MESSAGE_THAT( + Invalid, + ::testing::HasSubstr("JSON too deeply nested: max nesting depth is 100"), + ParseFromString(options, deeply_nested_json_object(depth), &parsed)); + EXPECT_RAISES_WITH_MESSAGE_THAT( + Invalid, + ::testing::HasSubstr("JSON too deeply nested: max nesting depth is 100"), + ParseFromString(options, deeply_nested_json_array(depth), &parsed)); + } + } +} + } // namespace json } // namespace arrow diff --git a/cpp/src/arrow/util/simdjson_internal.cc b/cpp/src/arrow/util/simdjson_internal.cc index d845eab0d473..d8cea1a5536c 100644 --- a/cpp/src/arrow/util/simdjson_internal.cc +++ b/cpp/src/arrow/util/simdjson_internal.cc @@ -538,47 +538,57 @@ Result 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(ValidateJsonValue(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(ValidateJsonValue(value, max_depth, depth)); } - return Status::OK(); } +Status ValidateJsonValue(simdjson::ondemand::value value, int max_depth, int depth) { + return VisitJsonValue( + value, + [depth, max_depth](simdjson::ondemand::object object) { + return ValidateJsonObject(object, max_depth, depth + 1); + }, + [depth, max_depth](simdjson::ondemand::array array) { + return ValidateJsonArray(array, max_depth, depth + 1); + }, + [](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 ValidateJsonDocument(simdjson::ondemand::parser& parser, - simdjson::padded_string& json) { + simdjson::padded_string& json, int max_depth) { ARROW_ASSIGN_OR_RAISE( auto document, ResolveSimdjsonResult(parser.iterate(json), "Failed to parse JSON")); ARROW_ASSIGN_OR_RAISE(auto value, ResolveSimdjsonResult(document.get_value(), "Failed to get JSON value")); - return ConsumeJsonValue(value); + return ValidateJsonValue(value, max_depth); } /// Returns the position of the first non-whitespace character when trailing is false, diff --git a/cpp/src/arrow/util/simdjson_internal.h b/cpp/src/arrow/util/simdjson_internal.h index d5816522bee0..fbdf8639dad6 100644 --- a/cpp/src/arrow/util/simdjson_internal.h +++ b/cpp/src/arrow/util/simdjson_internal.h @@ -341,14 +341,11 @@ Result GetJsonField(simdjson::ondemand::object& object, std::string_view key) ARROW_EXPORT Result MinifyJson(std::string_view json); -ARROW_EXPORT Status ValidateJsonObject(simdjson::ondemand::object object); - -ARROW_EXPORT Status ValidateJsonArray(simdjson::ondemand::array array); - -ARROW_EXPORT Status ConsumeJsonValue(simdjson::ondemand::value value); +ARROW_EXPORT Status ValidateJsonValue(simdjson::ondemand::value value, int max_depth, + int depth = 0); ARROW_EXPORT Status ValidateJsonDocument(simdjson::ondemand::parser& parser, - simdjson::padded_string& json); + simdjson::padded_string& json, int max_depth); ARROW_EXPORT int64_t ConsumeJsonWhitespace(std::string_view view, bool trailing); diff --git a/cpp/src/parquet/geospatial/util_json_internal.cc b/cpp/src/parquet/geospatial/util_json_internal.cc index 6933d692795f..308c882fddc8 100644 --- a/cpp/src/parquet/geospatial/util_json_internal.cc +++ b/cpp/src/parquet/geospatial/util_json_internal.cc @@ -30,6 +30,10 @@ namespace parquet { namespace { + +// An upper bound on the expected nesting depth of a geospatial JSON metadata object +constexpr int kMaxJsonDepth = 20; + ::arrow::Result GeospatialGeoArrowCrsToParquetCrs( simdjson::ondemand::object object) { auto json_crs_result = @@ -195,7 +199,7 @@ ::arrow::Result EscapeCrsAsJsonIfRequired(std::string_view crs) { simdjson::ondemand::parser parser; simdjson::padded_string json(crs); - if (!::arrow::internal::ValidateJsonDocument(parser, json).ok()) { + if (!::arrow::internal::ValidateJsonDocument(parser, json, kMaxJsonDepth).ok()) { return EscapeJsonString(crs); } @@ -215,7 +219,7 @@ LogicalTypeFromGeoArrowMetadata(std::string_view serialized_data) { simdjson::ondemand::parser parser; simdjson::padded_string json(serialized_data); - RETURN_NOT_OK(::arrow::internal::ValidateJsonDocument(parser, json)); + RETURN_NOT_OK(::arrow::internal::ValidateJsonDocument(parser, json, kMaxJsonDepth)); // Reparse because validation consumes the On-Demand document. ARROW_ASSIGN_OR_RAISE(auto document, ::arrow::internal::ResolveSimdjsonResult( diff --git a/cpp/src/parquet/reader_test.cc b/cpp/src/parquet/reader_test.cc index b5487fa46899..05ce29848737 100644 --- a/cpp/src/parquet/reader_test.cc +++ b/cpp/src/parquet/reader_test.cc @@ -1445,7 +1445,8 @@ ::arrow::Status CheckJsonValid(std::string_view json_string) { simdjson::ondemand::parser parser; auto padded_json = simdjson::padded_string(json_string); - RETURN_NOT_OK(::arrow::internal::ValidateJsonDocument(parser, padded_json)); + RETURN_NOT_OK( + ::arrow::internal::ValidateJsonDocument(parser, padded_json, /*max_depth=*/10)); return ::arrow::Status::OK(); } diff --git a/python/pyarrow/tests/test_json.py b/python/pyarrow/tests/test_json.py index e7ab574ead48..2239c2f1bb8b 100644 --- a/python/pyarrow/tests/test_json.py +++ b/python/pyarrow/tests/test_json.py @@ -22,6 +22,7 @@ import itertools import json import string +import sys import unittest try: @@ -344,6 +345,32 @@ def test_stress_block_sizes(self): # Better error output assert table.to_pydict() == expected.to_pydict() + def test_max_nesting_depth(self): + if pa.build_info.build_type == 'debug' and sys.platform == 'darwin': + pytest.skip('test crashes in debug mode on macOS ' + 'due to small default thread stack size') + + def deeply_nested_json_object(depth): + return b'{"a":' * depth + b'1' + b'}' * depth + + def deeply_nested_json_array(depth): + return b'{"a":' + b'[' * (depth - 1) + b'1' + b']' * (depth - 1) + b'}' + + max_depth = 100 # Hard-coded in arrow/json/parser.cc + table = self.read_bytes(deeply_nested_json_object(max_depth)) + table.validate(full=True) + table = self.read_bytes(deeply_nested_json_array(max_depth)) + table.validate(full=True) + + with pytest.raises( + ValueError, + match=f"JSON too deeply nested: max nesting depth is {max_depth}"): + self.read_bytes(deeply_nested_json_object(max_depth + 1)) + with pytest.raises( + ValueError, + match=f"JSON too deeply nested: max nesting depth is {max_depth}"): + self.read_bytes(deeply_nested_json_array(max_depth + 1)) + class BaseTestJSONRead(BaseTestJSON):