Skip to content

GH-50057: [C++] Avoid signed overflow in Decimal FromString exponent#50058

Merged
pitrou merged 2 commits into
apache:mainfrom
metsw24-max:decimal-fromstring-exponent-overflow
Jun 25, 2026
Merged

GH-50057: [C++] Avoid signed overflow in Decimal FromString exponent#50058
pitrou merged 2 commits into
apache:mainfrom
metsw24-max:decimal-fromstring-exponent-overflow

Conversation

@metsw24-max

@metsw24-max metsw24-max commented May 27, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

Decimal{32,64,128,256}::FromString parse the exponent as int32_t, so a string like 0E-2147483648 reaches DecimalFromString/SimpleDecimalFromString with dec.exponent == INT32_MIN and negating it is UB:

decimal.cc: runtime error: negation of -2147483648 cannot be represented in type 'int32_t' (aka 'int')

A near-INT32_MIN exponent overflows the -exponent + fractional_digits addition the same way. Both helpers are reachable from the CSV/JSON readers parsing decimal columns.

What changes are included in this PR?

Compute the scale via internal::SubtractWithOverflow and reject the unrepresentable exponents with Status::Invalid, in both helpers.

Are these changes tested?

Yes, 0E-2147483648 and 1.0E-2147483647 added to DecimalFromStringTest.InvalidInput (runs for Decimal32/64/128/256).

Are there any user-facing changes?

No.

@github-actions github-actions Bot added the awaiting review Awaiting review label May 27, 2026
@github-actions

Copy link
Copy Markdown

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

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50057 has no components, please add labels for components.

Signed-off-by: Sayed Kaif <metsw24@gmail.com>
@metsw24-max metsw24-max requested a review from pitrou as a code owner June 23, 2026 16:54
@metsw24-max

Copy link
Copy Markdown
Contributor Author

Pushed a clang-format fixup for the C++ Format lint failure, rest of the change is unchanged.

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this @metsw24-max , let's just wait for CI

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 25, 2026
@pitrou pitrou merged commit b43a417 into apache:main Jun 25, 2026
53 of 55 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Jun 25, 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 b43a417.

There was 1 benchmark result indicating a performance regression:

The full Conbench report has more details. It also includes information about 15 possible false positives for unstable benchmarks that are known to sometimes produce them.

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