Skip to content

Commit df40dd4

Browse files
pitroupitrou
authored
GH-51238: [C++][Python][Parquet] Limit schema nesting depth when reading (#51239)
### 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. * GitHub Issue: #51238 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
1 parent a811bcd commit df40dd4

15 files changed

Lines changed: 255 additions & 57 deletions

File tree

‎cpp/src/arrow/dataset/file_parquet.cc‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,14 +68,16 @@ parquet::ReaderProperties MakeReaderProperties(
6868
const ParquetFileFormat& format, ParquetFragmentScanOptions* parquet_scan_options,
6969
const std::string& path = "", std::shared_ptr<fs::FileSystem> filesystem = nullptr,
7070
MemoryPool* pool = default_memory_pool()) {
71-
// Can't mutate pool after construction
71+
// FIXME (GH-51264): Can't mutate pool after ReaderProperties construction.
7272
parquet::ReaderProperties properties(pool);
7373
if (parquet_scan_options->reader_properties->is_buffered_stream_enabled()) {
7474
properties.enable_buffered_stream();
7575
} else {
7676
properties.disable_buffered_stream();
7777
}
7878
properties.set_buffer_size(parquet_scan_options->reader_properties->buffer_size());
79+
properties.set_footer_read_size(
80+
parquet_scan_options->reader_properties->footer_read_size());
7981

8082
auto file_decryption_prop =
8183
parquet_scan_options->reader_properties->file_decryption_properties();
@@ -101,6 +103,8 @@ parquet::ReaderProperties MakeReaderProperties(
101103
parquet_scan_options->reader_properties->thrift_string_size_limit());
102104
properties.set_thrift_container_size_limit(
103105
parquet_scan_options->reader_properties->thrift_container_size_limit());
106+
properties.set_schema_depth_limit(
107+
parquet_scan_options->reader_properties->schema_depth_limit());
104108

105109
properties.set_page_checksum_verification(
106110
parquet_scan_options->reader_properties->page_checksum_verification());

‎cpp/src/parquet/arrow/arrow_schema_test.cc‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1902,8 +1902,10 @@ class TestConvertRoundTrip : public ::testing::Test {
19021902
::parquet::default_writer_properties();
19031903
RETURN_NOT_OK(ToParquetSchema(arrow_schema_.get(), *properties.get(),
19041904
*arrow_properties, &parquet_schema_));
1905-
::parquet::schema::ToParquet(parquet_schema_->group_node(), &parquet_format_schema_);
1906-
auto parquet_schema = ::parquet::schema::FromParquet(parquet_format_schema_);
1905+
::parquet::schema::SchemaToThrift(parquet_schema_->group_node(),
1906+
&parquet_format_schema_);
1907+
auto parquet_schema =
1908+
::parquet::schema::SchemaFromThrift(parquet_format_schema_, /*max_depth=*/100);
19071909
return FromParquetSchema(parquet_schema.get(), &result_schema_);
19081910
}
19091911

‎cpp/src/parquet/metadata.cc‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -778,9 +778,12 @@ class FileMetaData::FileMetaDataImpl {
778778
public:
779779
FileMetaDataImpl() = default;
780780

781-
explicit FileMetaDataImpl(
782-
const void* metadata, int64_t metadata_len, ReaderProperties properties,
783-
std::shared_ptr<InternalFileDecryptor> file_decryptor = nullptr)
781+
explicit FileMetaDataImpl(ReaderProperties properties)
782+
: properties_(std::move(properties)) {}
783+
784+
FileMetaDataImpl(const void* metadata, int64_t metadata_len,
785+
ReaderProperties properties,
786+
std::shared_ptr<InternalFileDecryptor> file_decryptor = nullptr)
784787
: properties_(std::move(properties)), file_decryptor_(std::move(file_decryptor)) {
785788
metadata_ = std::make_unique<format::FileMetaData>();
786789

@@ -1022,8 +1025,8 @@ class FileMetaData::FileMetaDataImpl {
10221025
if (metadata_->schema.empty()) {
10231026
throw ParquetException("Empty file schema (no root)");
10241027
}
1025-
schema_.Init(schema::Unflatten(&metadata_->schema[0],
1026-
static_cast<int>(metadata_->schema.size())));
1028+
schema_.Init(schema::Unflatten(metadata_->schema,
1029+
/*max_depth=*/properties_.schema_depth_limit()));
10271030
}
10281031

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

1080+
FileMetaData::FileMetaData(ReaderProperties properties)
1081+
: impl_(new FileMetaDataImpl(std::move(properties))) {}
1082+
10771083
FileMetaData::FileMetaData() : impl_(new FileMetaDataImpl()) {}
10781084

10791085
FileMetaData::~FileMetaData() = default;
@@ -2147,10 +2153,15 @@ class FileMetaDataBuilder::FileMetaDataBuilderImpl {
21472153
}
21482154
}
21492155

2150-
ToParquet(static_cast<parquet::schema::GroupNode*>(schema_->schema_root().get()),
2151-
&metadata_->schema);
2152-
auto file_meta_data = std::unique_ptr<FileMetaData>(new FileMetaData());
2156+
SchemaToThrift(static_cast<parquet::schema::GroupNode*>(schema_->schema_root().get()),
2157+
&metadata_->schema);
2158+
ReaderProperties properties;
2159+
// Disable schema nesting depth for schema restruction in InitSchema below.
2160+
properties.set_schema_depth_limit(std::numeric_limits<int32_t>::max());
2161+
auto file_meta_data =
2162+
std::unique_ptr<FileMetaData>(new FileMetaData(std::move(properties)));
21532163
file_meta_data->impl_->metadata_ = std::move(metadata_);
2164+
// XXX Why are we reconstructing the schema from the flattened Thrift structures?
21542165
file_meta_data->impl_->InitSchema();
21552166
file_meta_data->impl_->InitKeyValueMetadata();
21562167
return file_meta_data;

‎cpp/src/parquet/metadata.h‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -386,9 +386,10 @@ class PARQUET_EXPORT FileMetaData {
386386
friend class SerializedFile;
387387
friend class SerializedRowGroup;
388388

389-
explicit FileMetaData(const void* serialized_metadata, int64_t metadata_len,
390-
const ReaderProperties& properties,
391-
std::shared_ptr<InternalFileDecryptor> file_decryptor = NULLPTR);
389+
explicit FileMetaData(ReaderProperties properties);
390+
FileMetaData(const void* serialized_metadata, int64_t metadata_len,
391+
const ReaderProperties& properties,
392+
std::shared_ptr<InternalFileDecryptor> file_decryptor = NULLPTR);
392393

393394
void set_file_decryptor(std::shared_ptr<InternalFileDecryptor> file_decryptor);
394395
const std::shared_ptr<InternalFileDecryptor>& file_decryptor() const;

‎cpp/src/parquet/properties.h‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,10 @@ constexpr int32_t kDefaultThriftStringSizeLimit = 100 * 1000 * 1000;
6868
// kDefaultStringSizeLimit.
6969
constexpr int32_t kDefaultThriftContainerSizeLimit = 1000 * 1000;
7070

71+
// Maximum schema nesting depth. This default value is conservatively small as
72+
// some systems may not set a very large stack size.
73+
constexpr int32_t kDefaultSchemaDepthLimit = 100;
74+
7175
// PARQUET-978: Minimize footer reads by reading 64 KB from the end of the file
7276
constexpr int64_t kDefaultFooterReadSize = 64 * 1024;
7377

@@ -121,6 +125,15 @@ class PARQUET_EXPORT ReaderProperties {
121125
thrift_container_size_limit_ = size;
122126
}
123127

128+
/// \brief Return the schema nesting depth limit.
129+
///
130+
/// This limit helps prevent denial of service through excessive recursion
131+
/// (stack overflow) when reconstructing the Parquet schema from the file metadata.
132+
/// The default value is conservative enough for most use cases.
133+
int32_t schema_depth_limit() const { return schema_depth_limit_; }
134+
/// Set the schema nesting depth limit.
135+
void set_schema_depth_limit(int32_t size) { schema_depth_limit_ = size; }
136+
124137
/// Set the decryption properties.
125138
void file_decryption_properties(std::shared_ptr<FileDecryptionProperties> decryption) {
126139
file_decryption_properties_ = std::move(decryption);
@@ -146,6 +159,7 @@ class PARQUET_EXPORT ReaderProperties {
146159
int64_t buffer_size_ = kDefaultBufferSize;
147160
int32_t thrift_string_size_limit_ = kDefaultThriftStringSizeLimit;
148161
int32_t thrift_container_size_limit_ = kDefaultThriftContainerSizeLimit;
162+
int32_t schema_depth_limit_ = kDefaultSchemaDepthLimit;
149163
bool buffered_stream_enabled_ = false;
150164
bool page_checksum_verification_ = false;
151165
// Used with a RecordReader.

‎cpp/src/parquet/reader_test.cc‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,8 @@ std::string byte_stream_split_extended() {
138138
return data_file("byte_stream_split_extended.gzip.parquet");
139139
}
140140

141+
std::string nested_lists() { return data_file("nested_lists.snappy.parquet"); }
142+
141143
template <typename DType, typename ValueType = typename DType::c_type>
142144
std::vector<ValueType> ReadColumnValues(ParquetFileReader* file_reader, int row_group,
143145
int column, int64_t expected_values_read) {
@@ -705,14 +707,32 @@ TEST(TestFileReader, RecordReaderWithExposingDictionary) {
705707
}
706708
}
707709

710+
TEST(TestFileReader, SchemaDepthLimit) {
711+
#ifndef ARROW_WITH_SNAPPY
712+
GTEST_SKIP() << "Test requires Snappy compression";
713+
#endif
714+
ReaderProperties reader_props;
715+
// File has a column "a.list.element.list.element.list.element"
716+
// (nesting depth 8 including the root)
717+
reader_props.set_schema_depth_limit(8);
718+
std::unique_ptr<ParquetFileReader> file_reader =
719+
ParquetFileReader::OpenFile(nested_lists(), /*memory_map=*/false, reader_props);
720+
reader_props.set_schema_depth_limit(7);
721+
EXPECT_THAT(
722+
[&] {
723+
ParquetFileReader::OpenFile(nested_lists(), /*memory_map=*/false, reader_props);
724+
},
725+
::testing::ThrowsMessage<ParquetException>(
726+
::testing::HasSubstr("Parquet schema too deeply nested")));
727+
}
728+
708729
class TestLocalFile : public ::testing::Test {
709730
public:
710731
void SetUp() {
711732
std::string dir_string(test::get_data_dir());
712733

713734
std::stringstream ss;
714-
ss << dir_string << "/"
715-
<< "alltypes_plain.parquet";
735+
ss << dir_string << "/" << "alltypes_plain.parquet";
716736

717737
PARQUET_ASSIGN_OR_THROW(handle, ReadableFile::Open(ss.str()));
718738
fileno = handle->file_descriptor();

‎cpp/src/parquet/schema.cc‎

Lines changed: 44 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include <algorithm>
2121
#include <cstring>
2222
#include <memory>
23+
#include <sstream>
2324
#include <string>
2425
#include <type_traits>
2526
#include <utility>
@@ -544,11 +545,15 @@ void PrimitiveNode::ToParquet(void* opaque_element) const {
544545
// ----------------------------------------------------------------------
545546
// Schema converters
546547

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

561-
int pos = 0;
566+
size_t pos = 0;
567+
size_t num_reserved = 0;
562568

563-
std::function<std::unique_ptr<Node>()> NextNode = [&]() {
564-
if (pos == length) {
565-
throw ParquetException("Malformed schema: not enough elements");
569+
std::function<std::unique_ptr<Node>(int depth)> NextNode = [&](int depth) {
570+
if (pos == elements.size()) {
571+
throw ParquetException("Malformed Parquet schema: not enough elements");
566572
}
567573
const SchemaElement& element = elements[pos++];
568574
const void* opaque_element = static_cast<const void*>(&element);
@@ -572,22 +578,42 @@ std::unique_ptr<Node> Unflatten(const format::SchemaElement* elements, int lengt
572578
return PrimitiveNode::FromParquet(opaque_element);
573579
} else {
574580
// Group node (may have 0 children, but cannot have a type)
575-
NodeVector fields;
581+
// Protect against denial-of-service through stack exhaustion when parsing
582+
// deeply nested schemas.
583+
if (depth >= max_depth) {
584+
std::stringstream ss;
585+
ss << "Parquet schema too deeply nested, consider increasing schema depth limit "
586+
"(current limit is "
587+
<< max_depth << ")";
588+
throw ParquetException(ss.str());
589+
}
590+
if (element.num_children < 0) {
591+
throw ParquetException("Malformed Parquet schema: negative number of children");
592+
}
593+
// Guard against excessive pre-reservation by an invalid schema.
594+
// For example, a sequence of group nodes advertising N, N-1, etc. children
595+
// could lead to quadratic preallocation.
596+
num_reserved += static_cast<size_t>(element.num_children);
597+
if (num_reserved > elements.size()) {
598+
throw ParquetException("Malformed Parquet schema: not enough elements");
599+
}
600+
NodeVector fields(element.num_children);
576601
for (int i = 0; i < element.num_children; ++i) {
577-
std::unique_ptr<Node> field = NextNode();
578-
fields.push_back(NodePtr(field.release()));
602+
fields[i] = NextNode(depth + 1);
579603
}
580604
return GroupNode::FromParquet(opaque_element, std::move(fields));
581605
}
582606
};
583-
return NextNode();
607+
auto root = NextNode(/*depth=*/1);
608+
if (pos != elements.size()) {
609+
throw ParquetException("Malformed Parquet schema: too many elements");
610+
}
611+
return root;
584612
}
585613

586-
std::shared_ptr<SchemaDescriptor> FromParquet(const std::vector<SchemaElement>& schema) {
587-
if (schema.empty()) {
588-
throw ParquetException("Empty file schema (no root)");
589-
}
590-
std::unique_ptr<Node> root = Unflatten(&schema[0], static_cast<int>(schema.size()));
614+
std::shared_ptr<SchemaDescriptor> SchemaFromThrift(std::span<const SchemaElement> schema,
615+
int max_depth) {
616+
std::unique_ptr<Node> root = Unflatten(schema, max_depth);
591617
std::shared_ptr<SchemaDescriptor> descr = std::make_shared<SchemaDescriptor>();
592618
descr->Init(std::shared_ptr<GroupNode>(static_cast<GroupNode*>(root.release())));
593619
return descr;
@@ -615,7 +641,7 @@ class SchemaVisitor : public Node::ConstVisitor {
615641
std::vector<format::SchemaElement>* elements_;
616642
};
617643

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

717743
void Visit(const GroupNode* node) {
718744
PrintRepLevel(node->repetition(), stream_);
719-
stream_ << " group "
720-
<< "field_id=" << node->field_id() << " " << node->name();
745+
stream_ << " group " << "field_id=" << node->field_id() << " " << node->name();
721746
auto lt = node->converted_type();
722747
const auto& la = node->logical_type();
723748
if (la && la->is_valid() && !la->is_none()) {

‎cpp/src/parquet/schema_internal.h‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#pragma once
2121

2222
#include <memory>
23+
#include <span>
2324
#include <vector>
2425

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

4041
PARQUET_EXPORT
41-
std::shared_ptr<SchemaDescriptor> FromParquet(
42-
const std::vector<format::SchemaElement>& schema);
42+
std::shared_ptr<SchemaDescriptor> SchemaFromThrift(
43+
std::span<const format::SchemaElement> schema, int max_depth);
4344

4445
PARQUET_EXPORT
45-
std::unique_ptr<Node> Unflatten(const format::SchemaElement* elements, int length);
46+
std::unique_ptr<Node> Unflatten(std::span<const format::SchemaElement> schema,
47+
int max_depth);
4648

4749
// ----------------------------------------------------------------------
4850
// Conversion to Parquet Thrift metadata
4951

5052
PARQUET_EXPORT
51-
void ToParquet(const GroupNode* schema, std::vector<format::SchemaElement>* out);
53+
void SchemaToThrift(const GroupNode* schema, std::vector<format::SchemaElement>* out);
5254

5355
} // namespace schema
5456
} // namespace parquet

0 commit comments

Comments
 (0)