Skip to content

Commit 4e25461

Browse files
GH-50051: [C++][Parquet] Avoid size overflow in WKBBuffer::ReadCoords (#50036)
### Rationale for this change `WKBBuffer::ReadCoords()` computes the coordinate sequence byte size using `n_coords * sizeof(Coord)` before validating the remaining buffer size. On 32-bit targets such as wasm32, this multiplication can overflow when `n_coords` is derived from malformed WKB input, causing the bounds check to validate a truncated value before the subsequent coordinate read loop processes the coordinate sequence. This change widens the computation before validation so the bounds check remains correct across supported architectures. Related issue: #50051 ### What changes are included in this PR? * Widen the coordinate sequence byte-size computation in `WKBBuffer::ReadCoords()` * Prevent overflow-before-bounds-check behavior on 32-bit targets * Audit and harden closely related WKB parsing size computations with the same externally controlled arithmetic pattern ### Are these changes tested? Yes. Added coverage for malformed WKB inputs that exercise overflow-sensitive coordinate size calculations and verified the parser rejects invalid input safely. ### Are there any user-facing changes? No. **This PR contains a "Critical Fix".** Malformed WKB input could cause coordinate sequence size calculations to overflow on 32-bit targets before bounds validation occurs, potentially resulting in out-of-bounds reads during parsing. This change ensures bounds validation uses widened arithmetic before parsing coordinate sequences. * GitHub Issue: #50051 Authored-by: jmestwa-coder <jmestwa@gmail.com> Signed-off-by: Gang Wu <ustcwg@gmail.com>
1 parent 9265a71 commit 4e25461

2 files changed

Lines changed: 21 additions & 2 deletions

File tree

cpp/src/parquet/geospatial/util_internal.cc

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <sstream>
2222

2323
#include "arrow/util/endian.h"
24+
#include "arrow/util/int_util_overflow.h"
2425
#include "arrow/util/macros.h"
2526
#include "arrow/util/ubsan.h"
2627
#include "parquet/exception.h"
@@ -64,8 +65,11 @@ class WKBBuffer {
6465

6566
template <typename Coord, typename Visit>
6667
void ReadCoords(uint32_t n_coords, bool swap, Visit&& visit) {
67-
size_t total_bytes = n_coords * sizeof(Coord);
68-
if (size_ < total_bytes) {
68+
uint64_t total_bytes = 0;
69+
if (::arrow::internal::MultiplyWithOverflow(static_cast<uint64_t>(n_coords),
70+
static_cast<uint64_t>(sizeof(Coord)),
71+
&total_bytes) ||
72+
size_ < total_bytes) {
6973
throw ParquetException("Can't read coordinate sequence of ", total_bytes,
7074
" bytes from WKBBuffer with ", size_, " remaining");
7175
}

cpp/src/parquet/geospatial/util_internal_test.cc

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,21 @@ TEST_P(WKBTestFixture, TestWKBBounderErrorForInputWithTooManyBytes) {
166166
ASSERT_THROW(bounder.MergeGeometry(wkb_with_extra_byte), ParquetException);
167167
}
168168

169+
TEST(TestGeometryUtil, TestWKBBounderErrorForCoordinateSequenceSizeOverflow) {
170+
WKBGeometryBounder bounder;
171+
172+
// LINESTRING with 0x10000001 XY coordinates but only one coordinate in the buffer.
173+
// On 32-bit targets, 0x10000001 * sizeof(XY) would overflow to 16 bytes.
174+
std::vector<uint8_t> wkb = {0x01, // little endian
175+
0x02, 0x00, 0x00, 0x00, // LINESTRING
176+
0x01, 0x00, 0x00, 0x10, // 0x10000001 coordinates
177+
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // x = 0
178+
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // y = 0
179+
0x00};
180+
181+
ASSERT_THROW(bounder.MergeGeometry(wkb), ParquetException);
182+
}
183+
169184
INSTANTIATE_TEST_SUITE_P(
170185
TestGeometryUtil, WKBTestFixture,
171186
::testing::Values(

0 commit comments

Comments
 (0)