Skip to content

GH-51238: [C++][Python][Parquet] Limit schema nesting depth when reading #51239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pitrou merged 4 commits into from
Sep 9, 2026
Merged

GH-51238: [C++][Python][Parquet] Limit schema nesting depth when reading #51239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f4e71f7
GH-51238: [C++][Parquet] Limit schema nesting depth when reading
pitrou Sep 8, 2026
0e5e51a
Avoid UB when num_children is negative
pitrou Sep 8, 2026
84d8a65
Address review comments
pitrou Sep 9, 2026
ea20af7
Add issue reference
pitrou Sep 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion cpp/src/arrow/dataset/file_parquet.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,16 @@ parquet::ReaderProperties MakeReaderProperties(
const ParquetFileFormat& format, ParquetFragmentScanOptions* parquet_scan_options,
const std::string& path = "", std::shared_ptr<fs::FileSystem> filesystem = nullptr,
MemoryPool* pool = default_memory_pool()) {
// Can't mutate pool after construction
// FIXME (GH-51264): Can't mutate pool after ReaderProperties construction.
parquet::ReaderProperties properties(pool);
if (parquet_scan_options->reader_properties->is_buffered_stream_enabled()) {
properties.enable_buffered_stream();
} else {
properties.disable_buffered_stream();
}
properties.set_buffer_size(parquet_scan_options->reader_properties->buffer_size());
properties.set_footer_read_size(
parquet_scan_options->reader_properties->footer_read_size());
Comment on lines +79 to +80

@pitrou pitrou Sep 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a drive-by fix for an unrelated buglet.


auto file_decryption_prop =
parquet_scan_options->reader_properties->file_decryption_properties();
Expand All @@ -101,6 +103,8 @@ parquet::ReaderProperties MakeReaderProperties(
parquet_scan_options->reader_properties->thrift_string_size_limit());
properties.set_thrift_container_size_limit(
parquet_scan_options->reader_properties->thrift_container_size_limit());
properties.set_schema_depth_limit(
parquet_scan_options->reader_properties->schema_depth_limit());

properties.set_page_checksum_verification(
parquet_scan_options->reader_properties->page_checksum_verification());
Expand Down
6 changes: 4 additions & 2 deletions cpp/src/parquet/arrow/arrow_schema_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1902,8 +1902,10 @@ class TestConvertRoundTrip : public ::testing::Test {
::parquet::default_writer_properties();
RETURN_NOT_OK(ToParquetSchema(arrow_schema_.get(), *properties.get(),
*arrow_properties, &parquet_schema_));
::parquet::schema::ToParquet(parquet_schema_->group_node(), &parquet_format_schema_);
auto parquet_schema = ::parquet::schema::FromParquet(parquet_format_schema_);
::parquet::schema::SchemaToThrift(parquet_schema_->group_node(),
&parquet_format_schema_);
auto parquet_schema =
::parquet::schema::SchemaFromThrift(parquet_format_schema_, /*max_depth=*/100);
return FromParquetSchema(parquet_schema.get(), &result_schema_);
}

Expand Down
27 changes: 19 additions & 8 deletions cpp/src/parquet/metadata.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,9 +778,12 @@ class FileMetaData::FileMetaDataImpl {
public:
FileMetaDataImpl() = default;

explicit FileMetaDataImpl(
const void* metadata, int64_t metadata_len, ReaderProperties properties,
std::shared_ptr<InternalFileDecryptor> file_decryptor = nullptr)
explicit FileMetaDataImpl(ReaderProperties properties)
: properties_(std::move(properties)) {}

FileMetaDataImpl(const void* metadata, int64_t metadata_len,
ReaderProperties properties,
std::shared_ptr<InternalFileDecryptor> file_decryptor = nullptr)
: properties_(std::move(properties)), file_decryptor_(std::move(file_decryptor)) {
metadata_ = std::make_unique<format::FileMetaData>();

Expand Down Expand Up @@ -1022,8 +1025,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()));
Comment thread
pitrou marked this conversation as resolved.
}

void InitColumnOrders() {
Expand Down Expand Up @@ -1074,6 +1077,9 @@ FileMetaData::FileMetaData(const void* metadata, int64_t metadata_len,
: impl_(new FileMetaDataImpl(metadata, metadata_len, properties,
std::move(file_decryptor))) {}

FileMetaData::FileMetaData(ReaderProperties properties)
: impl_(new FileMetaDataImpl(std::move(properties))) {}

FileMetaData::FileMetaData() : impl_(new FileMetaDataImpl()) {}

FileMetaData::~FileMetaData() = default;
Expand Down Expand Up @@ -2147,10 +2153,15 @@ class FileMetaDataBuilder::FileMetaDataBuilderImpl {
}
}

ToParquet(static_cast<parquet::schema::GroupNode*>(schema_->schema_root().get()),
&metadata_->schema);
auto file_meta_data = std::unique_ptr<FileMetaData>(new FileMetaData());
SchemaToThrift(static_cast<parquet::schema::GroupNode*>(schema_->schema_root().get()),
&metadata_->schema);
ReaderProperties properties;
// Disable schema nesting depth for schema restruction in InitSchema below.
properties.set_schema_depth_limit(std::numeric_limits<int32_t>::max());
auto file_meta_data =
std::unique_ptr<FileMetaData>(new FileMetaData(std::move(properties)));
file_meta_data->impl_->metadata_ = std::move(metadata_);
// XXX Why are we reconstructing the schema from the flattened Thrift structures?
file_meta_data->impl_->InitSchema();
file_meta_data->impl_->InitKeyValueMetadata();
return file_meta_data;
Expand Down
7 changes: 4 additions & 3 deletions cpp/src/parquet/metadata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,9 +386,10 @@ class PARQUET_EXPORT FileMetaData {
friend class SerializedFile;
friend class SerializedRowGroup;

explicit FileMetaData(const void* serialized_metadata, int64_t metadata_len,
const ReaderProperties& properties,
std::shared_ptr<InternalFileDecryptor> file_decryptor = NULLPTR);
explicit FileMetaData(ReaderProperties properties);
FileMetaData(const void* serialized_metadata, int64_t metadata_len,
const ReaderProperties& properties,
std::shared_ptr<InternalFileDecryptor> file_decryptor = NULLPTR);

void set_file_decryptor(std::shared_ptr<InternalFileDecryptor> file_decryptor);
const std::shared_ptr<InternalFileDecryptor>& file_decryptor() const;
Expand Down
14 changes: 14 additions & 0 deletions cpp/src/parquet/properties.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ constexpr int32_t kDefaultThriftStringSizeLimit = 100 * 1000 * 1000;
// kDefaultStringSizeLimit.
constexpr int32_t kDefaultThriftContainerSizeLimit = 1000 * 1000;

// Maximum schema nesting depth. This default value is conservatively small as
// some systems may not set a very large stack size.
constexpr int32_t kDefaultSchemaDepthLimit = 100;

// PARQUET-978: Minimize footer reads by reading 64 KB from the end of the file
constexpr int64_t kDefaultFooterReadSize = 64 * 1024;

Expand Down Expand Up @@ -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; }

@wgtmac wgtmac Sep 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@HuaHuaY HuaHuaY Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any conventions regarding the use of int32_t? Could we use uint32_t here?

@pitrou pitrou Sep 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use signed integers in most public APIs, we should probably not deviate here.


/// Set the decryption properties.
void file_decryption_properties(std::shared_ptr<FileDecryptionProperties> decryption) {
file_decryption_properties_ = std::move(decryption);
Expand All @@ -146,6 +159,7 @@ class PARQUET_EXPORT ReaderProperties {
int64_t buffer_size_ = kDefaultBufferSize;
int32_t thrift_string_size_limit_ = kDefaultThriftStringSizeLimit;
int32_t thrift_container_size_limit_ = kDefaultThriftContainerSizeLimit;
int32_t schema_depth_limit_ = kDefaultSchemaDepthLimit;
bool buffered_stream_enabled_ = false;
bool page_checksum_verification_ = false;
// Used with a RecordReader.
Expand Down
24 changes: 22 additions & 2 deletions cpp/src/parquet/reader_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ std::string byte_stream_split_extended() {
return data_file("byte_stream_split_extended.gzip.parquet");
}

std::string nested_lists() { return data_file("nested_lists.snappy.parquet"); }

template <typename DType, typename ValueType = typename DType::c_type>
std::vector<ValueType> ReadColumnValues(ParquetFileReader* file_reader, int row_group,
int column, int64_t expected_values_read) {
Expand Down Expand Up @@ -705,14 +707,32 @@ TEST(TestFileReader, RecordReaderWithExposingDictionary) {
}
}

TEST(TestFileReader, SchemaDepthLimit) {
#ifndef ARROW_WITH_SNAPPY
GTEST_SKIP() << "Test requires Snappy compression";
#endif
ReaderProperties reader_props;
// File has a column "a.list.element.list.element.list.element"
// (nesting depth 8 including the root)
reader_props.set_schema_depth_limit(8);
std::unique_ptr<ParquetFileReader> file_reader =
ParquetFileReader::OpenFile(nested_lists(), /*memory_map=*/false, reader_props);
reader_props.set_schema_depth_limit(7);
EXPECT_THAT(
[&] {
ParquetFileReader::OpenFile(nested_lists(), /*memory_map=*/false, reader_props);
},
::testing::ThrowsMessage<ParquetException>(
::testing::HasSubstr("Parquet schema too deeply nested")));
}

class TestLocalFile : public ::testing::Test {
public:
void SetUp() {
std::string dir_string(test::get_data_dir());

std::stringstream ss;
ss << dir_string << "/"
<< "alltypes_plain.parquet";
ss << dir_string << "/" << "alltypes_plain.parquet";

PARQUET_ASSIGN_OR_THROW(handle, ReadableFile::Open(ss.str()));
fileno = handle->file_descriptor();
Expand Down
63 changes: 44 additions & 19 deletions cpp/src/parquet/schema.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <algorithm>
#include <cstring>
#include <memory>
#include <sstream>
#include <string>
#include <type_traits>
#include <utility>
Expand Down Expand Up @@ -544,11 +545,15 @@ void PrimitiveNode::ToParquet(void* opaque_element) const {
// ----------------------------------------------------------------------
// Schema converters

std::unique_ptr<Node> Unflatten(const format::SchemaElement* elements, int length) {
std::unique_ptr<Node> Unflatten(std::span<const format::SchemaElement> elements,
int max_depth) {
if (elements.empty()) {
throw ParquetException("Empty Parquet schema (no root)");
}
if (elements[0].num_children == 0) {
if (length == 1) {
if (elements.size() == 1) {
// Degenerate case of Parquet file with no columns
return GroupNode::FromParquet(elements, {});
return GroupNode::FromParquet(&elements[0], {});
} else {
throw ParquetException(
"Parquet schema had multiple nodes but root had no children");
Expand All @@ -558,11 +563,12 @@ std::unique_ptr<Node> Unflatten(const format::SchemaElement* elements, int lengt
// We don't check that the root node is repeated since this is not
// consistently set by implementations

int pos = 0;
size_t pos = 0;
size_t num_reserved = 0;

std::function<std::unique_ptr<Node>()> NextNode = [&]() {
if (pos == length) {
throw ParquetException("Malformed schema: not enough elements");
std::function<std::unique_ptr<Node>(int depth)> NextNode = [&](int depth) {
if (pos == elements.size()) {
throw ParquetException("Malformed Parquet schema: not enough elements");
}
const SchemaElement& element = elements[pos++];
const void* opaque_element = static_cast<const void*>(&element);
Expand All @@ -572,22 +578,42 @@ 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) {
Comment thread
pitrou marked this conversation as resolved.
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("Malformed Parquet schema: negative number of children");
}
// Guard against excessive pre-reservation by an invalid schema.
// For example, a sequence of group nodes advertising N, N-1, etc. children
// could lead to quadratic preallocation.
num_reserved += static_cast<size_t>(element.num_children);
if (num_reserved > elements.size()) {
throw ParquetException("Malformed Parquet schema: not enough elements");
}
NodeVector fields(element.num_children);
Comment thread
pitrou marked this conversation as resolved.
for (int i = 0; i < element.num_children; ++i) {
std::unique_ptr<Node> field = NextNode();
fields.push_back(NodePtr(field.release()));
fields[i] = NextNode(depth + 1);
}
return GroupNode::FromParquet(opaque_element, std::move(fields));
}
};
return NextNode();
auto root = NextNode(/*depth=*/1);
if (pos != elements.size()) {
throw ParquetException("Malformed Parquet schema: too many elements");
}
return root;
}

std::shared_ptr<SchemaDescriptor> FromParquet(const std::vector<SchemaElement>& schema) {
if (schema.empty()) {
throw ParquetException("Empty file schema (no root)");
}
std::unique_ptr<Node> root = Unflatten(&schema[0], static_cast<int>(schema.size()));
std::shared_ptr<SchemaDescriptor> SchemaFromThrift(std::span<const SchemaElement> schema,
int max_depth) {
std::unique_ptr<Node> root = Unflatten(schema, max_depth);
std::shared_ptr<SchemaDescriptor> descr = std::make_shared<SchemaDescriptor>();
descr->Init(std::shared_ptr<GroupNode>(static_cast<GroupNode*>(root.release())));
return descr;
Expand Down Expand Up @@ -615,7 +641,7 @@ class SchemaVisitor : public Node::ConstVisitor {
std::vector<format::SchemaElement>* elements_;
};

void ToParquet(const GroupNode* schema, std::vector<format::SchemaElement>* out) {
void SchemaToThrift(const GroupNode* schema, std::vector<format::SchemaElement>* out) {
SchemaVisitor visitor(out);
schema->VisitConst(&visitor);
}
Expand Down Expand Up @@ -716,8 +742,7 @@ struct SchemaPrinter : public Node::ConstVisitor {

void Visit(const GroupNode* node) {
PrintRepLevel(node->repetition(), stream_);
stream_ << " group "
<< "field_id=" << node->field_id() << " " << node->name();
stream_ << " group " << "field_id=" << node->field_id() << " " << node->name();
auto lt = node->converted_type();
const auto& la = node->logical_type();
if (la && la->is_valid() && !la->is_none()) {
Expand Down
10 changes: 6 additions & 4 deletions cpp/src/parquet/schema_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#pragma once

#include <memory>
#include <span>
#include <vector>

#include "parquet/platform.h"
Expand All @@ -38,17 +39,18 @@ namespace schema {
// Conversion from Parquet Thrift metadata

PARQUET_EXPORT
std::shared_ptr<SchemaDescriptor> FromParquet(
const std::vector<format::SchemaElement>& schema);
std::shared_ptr<SchemaDescriptor> SchemaFromThrift(
std::span<const format::SchemaElement> schema, int max_depth);

PARQUET_EXPORT
std::unique_ptr<Node> Unflatten(const format::SchemaElement* elements, int length);
std::unique_ptr<Node> Unflatten(std::span<const format::SchemaElement> schema,
int max_depth);

// ----------------------------------------------------------------------
// Conversion to Parquet Thrift metadata

PARQUET_EXPORT
void ToParquet(const GroupNode* schema, std::vector<format::SchemaElement>* out);
void SchemaToThrift(const GroupNode* schema, std::vector<format::SchemaElement>* out);

} // namespace schema
} // namespace parquet
Loading
Loading