GH-50051: [C++][Parquet] Avoid size overflow in WKBBuffer::ReadCoords

[jmestwa-coder]

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: [Parquet][C++] Audit WKB parsing size computations for 32-bit overflow-before-bounds-check patterns #50051

[kou]

• Could you open an issue for non MINOR change? See https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes for details.
• Could you use our PR template instead of removing it entirely?

[wgtmac]

Instead of fixing this one by one, could you figure out all similar spots with the help of coding agents?

[jmestwa-coder]

@wgtmac @kou

I opened #50051, restored the PR template, and audited the related WKB parsing paths in cpp/src/parquet/geospatial for similar overflow-before-bounds-check patterns on 32-bit targets.

During that audit, I did not identify additional high-confidence cases beyond the ReadCoords() computation addressed in this PR. I also added focused regression coverage for the malformed-input scenario exercised by this fix.

[github-actions]

⚠️ GitHub issue #50051 has been automatically assigned in GitHub to PR creator.

[wgtmac]

Thank you for creating the issue! I still have one minor question.

[wgtmac]

Should we use MultiplyWithOverflow instead? Current multiplication can still overflow in some extreme cases.

[jmestwa-coder]

Done, switched to MultiplyWithOverflow on uint64_t so the overflow path is rejected explicitly.

[Copilot]

Pull request overview

Hardens Parquet’s geospatial WKB parsing on 32-bit targets by preventing overflow in coordinate sequence size calculations prior to bounds checks, ensuring malformed WKB cannot bypass validation.

Changes:

• Widen n_coords * sizeof(Coord) computation in WKBBuffer::ReadCoords() to uint64_t before comparing against remaining buffer size.
• Add a regression test covering an overflow-sensitive coordinate count that would truncate on 32-bit builds.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
cpp/src/parquet/geospatial/util_internal.cc	Uses widened arithmetic for coordinate sequence byte-size calculation to prevent overflow-before-bounds-check.
cpp/src/parquet/geospatial/util_internal_test.cc	Adds a malformed WKB test case that exercises the overflow scenario and expects rejection.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[wgtmac]

Thank you, @jmestwa-coder!

[wgtmac]

Could you please fix the linter error?

[jmestwa-coder]

Thanks @wgtmac, done! It was a clang-format issue in the test's wkb array — reformatted and verified clang-format + cpplint pass locally now.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 4e25461.

There weren't enough matching historic benchmark results to make a call on whether there were regressions.

The full Conbench report has more details.