Skip to content

Fix uncontrolled recursion in WKBGeometryBounder::MergeGeometryInternal #3

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

Open
wgtmac wants to merge 2 commits into
base: main
Choose a base branch
from
+51 −3
Open

Fix uncontrolled recursion in WKBGeometryBounder::MergeGeometryInternal #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e61f492
Fix uncontrolled recursion in WKBGeometryBounder::MergeGeometryInternal
google-labs-jules[bot] Mar 19, 2026
35710cd
Fix uncontrolled recursion in WKBGeometryBounder::MergeGeometryInternal
google-labs-jules[bot] Mar 20, 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
9 changes: 7 additions & 2 deletions cpp/src/parquet/geospatial/util_internal.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,12 @@ void WKBGeometryBounder::MergeGeometry(::arrow::util::span<const uint8_t> bytes_
}
}

void WKBGeometryBounder::MergeGeometryInternal(WKBBuffer* src, bool record_wkb_type) {
void WKBGeometryBounder::MergeGeometryInternal(WKBBuffer* src, bool record_wkb_type,
int depth) {
if (depth > 128) {
throw ParquetException("WKB geometry has too many levels of nesting");
}

uint8_t endian = src->ReadUInt8();
#if ARROW_LITTLE_ENDIAN
bool swap = endian != 0x01;
Expand Down Expand Up @@ -208,7 +213,7 @@ void WKBGeometryBounder::MergeGeometryInternal(WKBBuffer* src, bool record_wkb_t
case GeometryType::kGeometryCollection: {
uint32_t n_parts = src->ReadUInt32(swap);
for (uint32_t i = 0; i < n_parts; i++) {
MergeGeometryInternal(src, /*record_wkb_type*/ false);
MergeGeometryInternal(src, /*record_wkb_type*/ false, depth + 1);
}
break;
}
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/parquet/geospatial/util_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ class PARQUET_EXPORT WKBGeometryBounder {
BoundingBox box_;
std::unordered_set<int32_t> geospatial_types_;

void MergeGeometryInternal(WKBBuffer* src, bool record_wkb_type);
void MergeGeometryInternal(WKBBuffer* src, bool record_wkb_type, int depth = 0);

void MergeSequence(WKBBuffer* src, Dimensions dimensions, uint32_t n_coords, bool swap);
};
Expand Down
43 changes: 43 additions & 0 deletions cpp/src/parquet/geospatial/util_internal_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,4 +545,47 @@ INSTANTIATE_TEST_SUITE_P(
MakeWKBPointTestCase{{30, 10, 40, 300}, false, true},
MakeWKBPointTestCase{{30, 10, 40, 300}, true, true}));

TEST(TestGeometryUtil, TestWKBBounderErrorForDeepNesting) {
// Construct a nested GeometryCollection with 200 levels
std::vector<uint8_t> nested_wkb;
int num_levels = 200;

for (int i = 0; i < num_levels; i++) {
nested_wkb.push_back(0x01); // little endian
nested_wkb.push_back(0x07); // geometry collection
nested_wkb.push_back(0x00);
nested_wkb.push_back(0x00);
nested_wkb.push_back(0x00);

nested_wkb.push_back(0x01); // 1 part
nested_wkb.push_back(0x00);
nested_wkb.push_back(0x00);
nested_wkb.push_back(0x00);
}

// Final part is an empty geometry collection
nested_wkb.push_back(0x01); // little endian
nested_wkb.push_back(0x07); // geometry collection
nested_wkb.push_back(0x00);
nested_wkb.push_back(0x00);
nested_wkb.push_back(0x00);

nested_wkb.push_back(0x00); // 0 parts
nested_wkb.push_back(0x00);
nested_wkb.push_back(0x00);
nested_wkb.push_back(0x00);

WKBGeometryBounder bounder;
EXPECT_THROW(
{
try {
bounder.MergeGeometry(nested_wkb);
} catch (const ParquetException& e) {
EXPECT_THAT(e.what(), ::testing::HasSubstr("too many levels of nesting"));
throw;
}
},
ParquetException);
}

} // namespace parquet::geospatial