GH-51238: [C++][Parquet] Limit schema nesting depth when reading - #51239
GH-51238: [C++][Parquet] Limit schema nesting depth when reading#51239pitrou wants to merge 2 commits into
Conversation
pitrou
added
backport-candidate
Critical Fix
a062326 to
76fba0f
Compare
| properties.set_footer_read_size( | ||
| parquet_scan_options->reader_properties->footer_read_size()); |
There was a problem hiding this comment.
This is a drive-by fix for an unrelated buglet.
github-actions
Bot
added
awaiting committer review
|
@github-actions crossbow submit -g cpp |
|
Revision: 131c58c Submitted crossbow builds: ursacomputing/crossbow @ actions-25d1b23663
|
|
@wgtmac @adamreeve @HuaHuaY Would you like to review this? |
|
|
||
| // Maximum schema nesting depth. This default value is conservatively small as | ||
| // some systems may not set a very large stack size. | ||
| constexpr int32_t kSchemaDepthLimit = 100; |
There was a problem hiding this comment.
Nit: Should probably be called kDefaultSchemaDepthLimit for consistency with the other constants and to be clear it's a default that can be overridden.
| /// 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; } |
There was a problem hiding this 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.
There was a problem hiding this comment.
Do we have any conventions regarding the use of int32_t? Could we use uint32_t here?
| if (element.num_children < 0) { | ||
| throw ParquetException("Invalid Parquet schema: negative number of children"); | ||
| } | ||
| NodeVector fields(element.num_children); |
There was a problem hiding this comment.
Should we also check if element.num_children is too large to avoid allocating too much memory?
| 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())); |
There was a problem hiding this comment.
FileMetaDataBuilder::Finish() creates a default FileMetaData and calls InitSchema(), so this applies the default depth limit of 100 which we cannot change.
| NodeVector fields; | ||
| // Protect against denial-of-service through stack exhaustion when parsing | ||
| // deeply nested schemas. | ||
| if (depth >= max_depth) { |
There was a problem hiding this 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.
4 participants
Rationale for this change
Reconstructing a nested Schema from the Parquet Thrift metadata implies a recursive call that can blow up the stack on pathologically-nested schemas (with thousands of nesting levels or more).
By adding a limit on the schema nesting depth, we turn a stack overflow-induced crash into a regular Parquet error.
Are these changes tested?
By additional unit tests; also privately with a proof-of-concept reproducer that induces a stack overflow exhaustion.
Are there any user-facing changes?
In the unlikely case where a legitimate Parquet file has a deeper schema than the default schema nesting limit in this PR (100), an error will be raised when reading where it used to succeed. The user can bump the limit to circumvent the error.
This PR contains a "Critical Fix". It fixes a crash on a deeply nested Parquet schema that would provoke a stack overflow. It is not an exploitable vulnerability except through denial of service.
Thanks to "1K0CT" for the initial report.