GH-47184: [Parquet][C++] Avoid multiplication overflow in FixedSizeBinaryBuilder::Reserve by mapleFU · Pull Request #47185 · apache/arrow

mapleFU · 2025-07-24T12:25:42Z

Rationale for this change

Fix issue found by OSS-Fuzz: https://oss-fuzz.com/testcase?key=6634425377161216

What changes are included in this PR?

This patch add more checkings directly in builder.

Are these changes tested?

By apache/arrow-testing#110

Are there any user-facing changes?

no

GitHub Issue: [C++][Parquet] Fuzz: Integer or heap overflow when reading bad file #47184

github-actions · 2025-07-24T12:26:06Z

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

kou

+1

kou · 2025-07-25T00:46:04Z

cpp/src/arrow/array/builder_binary.cc

It may be better that we also show the requested capacity like

arrow/cpp/src/arrow/array/builder_base.h

Lines 286 to 294 in 25f8f00

if (ARROW_PREDICT_FALSE(new_capacity < 0)) {

return Status::Invalid(

"Resize capacity must be positive (requested: ", new_capacity, ")");

}

if (ARROW_PREDICT_FALSE(new_capacity < length_)) {

return Status::Invalid("Resize cannot downsize (requested: ", new_capacity,

", current length: ", length_, ")");

}

does.

pitrou

@mapleFU You updated the parquet-testing submodule, but you should update arrow-testing instead, no?

pitrou · 2025-08-19T15:45:55Z

cpp/src/arrow/array/builder_binary.cc

We have Status::CapacityError for such errors.

mapleFU · 2025-08-22T23:22:07Z

Don't know why cannot reproduce test failed on MacOS M1...🤔

mapleFU · 2025-08-23T00:26:33Z

cpp/src/parquet/decoder.cc

I don't know can we decrease overhead for this

Did the inequality happen in the fuzz test?

See https://github.com/apache/arrow/actions/runs/17131673477/job/48597348959

The fuzzer will generate random underlying data from seeds, and this caused a heap buffer overflow when type mismatches. Maybe a boundary check on buffer is more lightweight, which only prevent from heap buffer overflow.

Size check would enforce underlying data to be same sized. However it introduce a size checking.

cpp/src/parquet/decoder.cc

wgtmac · 2025-08-23T07:39:26Z

cpp/src/parquet/decoder.cc

Did the inequality happen in the fuzz test?

cpp/src/arrow/array/builder_binary.cc

pitrou · 2025-08-25T09:44:23Z

Can you run the Parquet benchmarks to see if there any regressions?

mapleFU · 2025-08-25T14:55:01Z

I've change the checking from Builder ( which is commonly used ) to only DeltaByteArrayDecoder ( which is only place can cause FLBA to have this problem)

pitrou · 2025-08-25T15:52:14Z

I've change the checking from Builder ( which is commonly used ) to only DeltaByteArrayDecoder ( which is only place can cause FLBA to have this problem)

Did you forget to push your changes?
(also, please rebase :-))

pitrou · 2025-08-25T16:37:41Z

cpp/src/arrow/array/builder_binary.cc

Do we still need this change?

I don't know, I'll remove this and have a try

I suggest we need this change, I add cout and run fuzzing and found there're so many error message now:

Status FixedSizeBinaryBuilder::Resize(int64_t capacity) { RETURN_NOT_OK(CheckCapacity(capacity)); int64_t dest_capacity_bytes; if (ARROW_PREDICT_FALSE( MultiplyWithOverflow(capacity, byte_width_, &dest_capacity_bytes))) { std::cout << "Resize: capacity overflows (requested: " << capacity << ", byte_width: " << byte_width_ << std::endl; return Status::CapacityError("Resize: capacity overflows (requested: ", capacity, ", byte_width: ", byte_width_, ")"); } RETURN_NOT_OK(byte_builder_.Resize(dest_capacity_bytes)); return ArrayBuilder::Resize(capacity); }

Perhaps the problem here is:

Status ReadColumn(int i, const std::vector<int>& row_groups, ColumnReader* reader, std::shared_ptr<ChunkedArray>* out) { BEGIN_PARQUET_CATCH_EXCEPTIONS // TODO(wesm): This calculation doesn't make much sense when we have repeated // schema nodes int64_t records_to_read = 0; for (auto row_group : row_groups) { // Can throw exception std::cout << "Numvalues:" << reader_->metadata()->RowGroup(row_group)->ColumnChunk(i)->num_values() << '\n'; records_to_read += reader_->metadata()->RowGroup(row_group)->ColumnChunk(i)->num_values(); }

Num values is int64_t::max()

Generally, builder will throw exception. For fixed sized type, the value would exceeds the bounds

Perhaps for builder, the FLBA column is just so huge, and causes the issue. The integer and float types might also has this problem.

Oh, it's because ReserveValues is called above this:

void ReserveValues(int64_t extra_values) override { ARROW_DCHECK(!uses_values_); TypedRecordReader::ReserveValues(extra_values); PARQUET_THROW_NOT_OK(array_builder_.Reserve(extra_values)); }

!uses_values_ is only called by ByteArray and FLBA, so other types doesn't touches this case... And ByteArray would not have a multiplier, which is less possible to touch this

@pitrou gentle ping for idea here, should I prevent overflow from ReserveValues here?

Yes, I think this change is ok. Sorry for the delay.

cpp/src/parquet/decoder.cc

pitrou · 2025-08-25T16:40:29Z

cpp/src/parquet/decoder.cc

Should we do this in GetInternal instead? It will then benefit the Decode method as well.

Would try to put in the end of GetInternal

mapleFU · 2025-09-09T14:28:44Z

gentle ping @pitrou

pitrou

Thanks a lot @mapleFU . Can you just rebase so that we get updated CI results?

… using FixedSizedBuilder

This reverts commit 11c7ad250a610fdcd054341efb7662467f3558a1.

pitrou · 2025-09-16T16:26:34Z

@github-actions crossbow submit -g cpp

pitrou · 2025-09-16T16:35:29Z

@github-actions crossbow submit -g cpp

github-actions · 2025-09-16T16:39:01Z

Revision: 08b020b

Submitted crossbow builds: ursacomputing/crossbow @ actions-f7e0ba7b53

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-42-cpp
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

pitrou · 2025-09-16T17:12:53Z

Thanks again for fixing this @mapleFU !

conbench-apache-arrow · 2025-09-16T23:49:04Z

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

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

The full Conbench report has more details.

…SizeBinaryBuilder::Reserve (apache#47185) ### Rationale for this change Fix issue found by OSS-Fuzz: https://oss-fuzz.com/testcase?key=6634425377161216 ### What changes are included in this PR? This patch add more checkings directly in builder. ### Are these changes tested? By apache/arrow-testing#110 ### Are there any user-facing changes? no * GitHub Issue: apache#47184 Lead-authored-by: mwish <maplewish117@gmail.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>

github-actions bot added Component: C++ awaiting review Awaiting review labels Jul 24, 2025

kou approved these changes Jul 25, 2025

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Jul 25, 2025

mapleFU requested a review from pitrou August 19, 2025 15:24

pitrou requested changes Aug 19, 2025

View reviewed changes

cpp/src/arrow/array/builder_binary.cc Outdated

Copy link

Member

pitrou Aug 19, 2025

Choose a reason for hiding this comment

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

We have Status::CapacityError for such errors.

pitrou changed the title ~~GH-47184: [Parquet][C++] Try prevent from too large buffer when using FixedSizeBuilder~~ GH-47184: [Parquet][C++] Avoid multiplication overflow in FixedSizeBuilder::Reserve Aug 19, 2025

pitrou changed the title ~~GH-47184: [Parquet][C++] Avoid multiplication overflow in FixedSizeBuilder::Reserve~~ GH-47184: [Parquet][C++] Avoid multiplication overflow in FixedSizeBinaryBuilder::Reserve Aug 19, 2025

mapleFU requested a review from wgtmac as a code owner August 22, 2025 23:34

github-actions bot added the Component: Parquet label Aug 22, 2025

mapleFU commented Aug 23, 2025

View reviewed changes

wgtmac reviewed Aug 23, 2025

View reviewed changes

pitrou reviewed Aug 25, 2025

View reviewed changes

cpp/src/parquet/decoder.cc Outdated Show resolved Hide resolved

pitrou reviewed Aug 25, 2025

View reviewed changes

pitrou approved these changes Sep 15, 2025

View reviewed changes

mapleFU added 7 commits September 16, 2025 18:20

apacheGH-47184: [Parquet][C++] Try prevent from too large buffer when…

0a4333a

… using FixedSizedBuilder

bump parquet-testing

8af6819

update error msg

b0d0330

update testing

bfcfa8f

update builder_binary

266f918

Revert "bump parquet-testing"

efb33e3

This reverts commit 11c7ad250a610fdcd054341efb7662467f3558a1.

Try to add some checking

2d64ced

mapleFU added 4 commits September 16, 2025 18:20

Update: change checks from Builder to decoder

4416ccd

Add: change the checking to GetInternal

86bab12

fix fmt

337605c

Update code

078b574

pitrou force-pushed the GH-47184 branch from 4ed39b4 to 078b574 Compare September 16, 2025 16:20

This comment was marked as outdated.

Sign in to view

Bump subproject

08b020b

pitrou merged commit 37dcfcc into apache:main Sep 16, 2025
37 of 39 checks passed

pitrou removed the awaiting merge Awaiting merge label Sep 16, 2025

pitrou mentioned this pull request Sep 16, 2025

[C++][Parquet] Fuzz: Integer or heap overflow when reading bad file #47184

Closed

	if (ARROW_PREDICT_FALSE(new_capacity < 0)) {
	return Status::Invalid(
	"Resize capacity must be positive (requested: ", new_capacity, ")");
	}

	if (ARROW_PREDICT_FALSE(new_capacity < length_)) {
	return Status::Invalid("Resize cannot downsize (requested: ", new_capacity,
	", current length: ", length_, ")");
	}

Conversation

mapleFU commented Jul 24, 2025 • edited by pitrou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Jul 24, 2025

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Aug 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented Aug 25, 2025

Uh oh!

mapleFU commented Aug 25, 2025

Uh oh!

pitrou commented Aug 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Sep 9, 2025

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Sep 16, 2025

Uh oh!

This comment was marked as outdated.

pitrou commented Sep 16, 2025

Uh oh!

github-actions bot commented Sep 16, 2025

Uh oh!

Uh oh!

pitrou commented Sep 16, 2025

Uh oh!

conbench-apache-arrow bot commented Sep 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

mapleFU commented Jul 24, 2025 •

edited by pitrou

Loading

mapleFU Aug 26, 2025 •

edited

Loading