wgtmac commented on code in PR #51239:
URL: https://github.com/apache/arrow/pull/51239#discussion_r3964141465
##########
cpp/src/parquet/schema.cc:
##########
@@ -572,22 +577,35 @@ std::unique_ptr<Node> Unflatten(const
format::SchemaElement* elements, int lengt
return PrimitiveNode::FromParquet(opaque_element);
} else {
// Group node (may have 0 children, but cannot have a type)
- NodeVector fields;
+ // Protect against denial-of-service through stack exhaustion when
parsing
+ // deeply nested schemas.
+ if (depth >= max_depth) {
+ std::stringstream ss;
+ ss << "Parquet schema too deeply nested, consider increasing schema
depth limit "
+ "(current limit is "
+ << max_depth << ")";
+ throw ParquetException(ss.str());
+ }
+ if (element.num_children < 0) {
+ throw ParquetException("Invalid Parquet schema: negative number of
children");
+ }
+ NodeVector fields(element.num_children);
Review Comment:
Should we also check if `element.num_children` is too large?
##########
cpp/src/parquet/properties.h:
##########
@@ -121,6 +125,15 @@ class PARQUET_EXPORT ReaderProperties {
thrift_container_size_limit_ = size;
}
+ /// \brief Return the schema nesting depth limit.
+ ///
+ /// This limit helps prevent denial of service through excessive recursion
+ /// (stack overflow) when reconstructing the Parquet schema from the file
metadata.
+ /// The default value is conservative enough for most use cases.
+ int32_t schema_depth_limit() const { return schema_depth_limit_; }
+ /// Set the schema nesting depth limit.
+ void set_schema_depth_limit(int32_t size) { schema_depth_limit_ = size; }
Review Comment:
It was thinking if we need to reject a negative value here but it seems that
it will safely throw later so I'm fine to leave it simple here.
##########
cpp/src/parquet/schema.cc:
##########
@@ -572,22 +577,35 @@ std::unique_ptr<Node> Unflatten(const
format::SchemaElement* elements, int lengt
return PrimitiveNode::FromParquet(opaque_element);
} else {
// Group node (may have 0 children, but cannot have a type)
- NodeVector fields;
+ // Protect against denial-of-service through stack exhaustion when
parsing
+ // deeply nested schemas.
+ if (depth >= max_depth) {
Review Comment:
With the documented “including the root” depth semantics, this rejects an
empty group at exactly max_depth before checking num_children. A valid empty
struct at depth N is rejected, while a primitive at depth N is accepted. Could
the check happen on entry with depth > max_depth, or otherwise allow zero-child
groups at the boundary?
------
The above issue is spotted by Codex but I think it is too trivial.
##########
cpp/src/parquet/metadata.cc:
##########
@@ -1022,8 +1022,8 @@ class FileMetaData::FileMetaDataImpl {
if (metadata_->schema.empty()) {
throw ParquetException("Empty file schema (no root)");
}
- schema_.Init(schema::Unflatten(&metadata_->schema[0],
-
static_cast<int>(metadata_->schema.size())));
+ schema_.Init(schema::Unflatten(metadata_->schema,
+
/*max_depth=*/properties_.schema_depth_limit()));
Review Comment:
`FileMetaDataBuilder::Finish()` creates a default FileMetaData and calls
`InitSchema()`, so this applies the default depth limit of 100 which we cannot
change.
--
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]
