Skip to content

GH-50275: [C++][CSV] avoid int32 overflow in block parser value counts#50074

Merged
pitrou merged 3 commits into
apache:mainfrom
metsw24-max:csv-block-parser-int32-overflow
Jun 29, 2026
Merged

GH-50275: [C++][CSV] avoid int32 overflow in block parser value counts#50074
pitrou merged 3 commits into
apache:mainfrom
metsw24-max:csv-block-parser-int32-overflow

Conversation

@metsw24-max

@metsw24-max metsw24-max commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

The CSV block parser sizes its per-chunk value array from num_cols, the column count inferred from the first line of the input, times the rows-in-chunk count. PresizedValueDescWriter computes 2 + num_rows * num_cols, and ParseSpecialized computes num_cols_ * (num_rows_ - start) * 10, both in int32_t. A CSV whose first line carries a few million fields pushes these products past INT32_MAX, which is signed-integer-overflow UB (UBSan flags both expressions).

What changes are included in this PR?

Raise an error if the num_cols would prevent the block size from fitting in a 32 bits integer.

Are these changes tested?

With a new unit test.

Are there any user-facing changes?

No.

@metsw24-max
metsw24-max requested a review from pitrou as a code owner June 23, 2026 11:42
@metsw24-max

Copy link
Copy Markdown
Contributor Author

Pushed a clang-format fix for the C++ Format lint failure. The macOS Python job looks like an unrelated teardown issue, not from this change.

@pitrou

pitrou commented Jun 23, 2026

Copy link
Copy Markdown
Member

ParsedValueDesc stores a 32-bit offset, so we'll be having a problem anyway, right?
How about we simply error out when the product of rows and columns is greater than 2**31?

@metsw24-max

Copy link
Copy Markdown
Contributor Author

Good point, the offset is only 31 bits so widening the products doesn't actually save us once the value count passes int32. I've switched to erroring out before presizing when rows x cols would exceed INT32_MAX, and added a test that drives num_cols high enough to trip it. Kept the two multiplications in int64 as well, since the capacity's +2 and the bulk-filter threshold's * 10 can still overflow within that limit.

@pitrou

pitrou commented Jun 25, 2026

Copy link
Copy Markdown
Member

@metsw24-max Thanks for the update! This is looking good, but can you open an issue for this? This is not a MINOR issue.

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 25, 2026
@metsw24-max metsw24-max changed the title MINOR: [C++][CSV] avoid int32 overflow in block parser value counts GH-50275: [C++][CSV] avoid int32 overflow in block parser value counts Jun 29, 2026
@github-actions

Copy link
Copy Markdown

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

@metsw24-max

Copy link
Copy Markdown
Contributor Author

Opened #50275 and re-titled the PR to reference it. Cheers.

@pitrou

pitrou commented Jun 29, 2026

Copy link
Copy Markdown
Member

CI failures are unrelated, I'll merge.

@pitrou
pitrou merged commit d8890e0 into apache:main Jun 29, 2026
57 of 61 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Jun 29, 2026
@conbench-apache-arrow

Copy link
Copy Markdown

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

There was 1 benchmark result indicating a performance regression:

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants