Skip to content

GH-49817: [C++] Reject overflowing decimal strings - #51169

Open
1fanwang wants to merge 1 commit into
apache:mainfrom
1fanwang:1fannnw/decimal-shift-overflow
Open

GH-49817: [C++] Reject overflowing decimal strings#51169
1fanwang wants to merge 1 commit into
apache:mainfrom
1fanwang:1fannnw/decimal-shift-overflow

Conversation

@1fanwang

@1fanwang 1fanwang commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

Parsing an oversized decimal string can return OK with a wrapped value instead of rejecting the input. For the 51-digit input in #49817, the returned integer is the input modulo 2^128 instead of the parsed value.

What changes are included in this PR?

The digit accumulator now reports carry beyond the destination limbs. The parser also rejects magnitudes outside the signed range before constructing the decimal value. Decimal32 and Decimal64 use the same carry check.

This leaves precision, scale, and Gandiva rounding policy unchanged. It only prevents integer wrap from being reported as a successful parse.

Closes #49817

Are these changes tested?

Scenario Before After
Oversized Decimal128 input Returns OK with wrapped data Returns Invalid
Oversized Decimal256 input Returns OK with wrapped data Returns Invalid
Existing FromString coverage Not applicable Passes
Raw logs
$ cpp/build/debug/arrow-utility-test \
    --gtest_filter='Decimal128Test.FromStringLimits:Decimal256Test.FromStringLimits'

Before:
Expected Decimal128::FromString(...) to fail with Invalid, but got OK
Expected Decimal256::FromString(...) to fail with Invalid, but got OK
2 FAILED TESTS

$ cpp/build/debug/arrow-utility-test --gtest_filter='*FromString*'

After:
[==========] Running 48 tests from 7 test suites.
[  PASSED  ] 48 tests.

Are there any user-facing changes?

Yes. Decimal strings that exceed the target integer range now return Invalid instead of a corrupted value.

This PR contains a "Critical Fix". It prevents the decimal parser from returning incorrect data after integer overflow.

Generated-by: GitHub Copilot CLI (GPT-5.6 Sol)
Signed-off-by: 1fanwang <1fannnw@gmail.com>
@1fanwang
1fanwang requested a review from pitrou as a code owner September 6, 2026 15:00
Copilot AI lite review requested due to automatic review settings September 6, 2026 15:00
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Decimal32/Decimal64 parsing still incorrectly rejects the minimum negative representable value due to a value > max() magnitude check that doesn’t allow -2^(N-1).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR addresses integer overflow during C++ decimal string parsing so that oversized inputs no longer wrap modulo the destination bit width while still returning Status::OK(). It does so by adding overflow detection to the digit-accumulation path and rejecting magnitudes outside the signed integer range before constructing Decimal128/Decimal256 values.

Changes:

  • Add carry/overflow reporting to the digit accumulator used by FromString so overflow is detected instead of silently dropped.
  • Reject parsed magnitudes outside the signed representable range (e.g., > 2^(N-1)-1, or < -2^(N-1)) for wide decimals prior to constructing the decimal value.
  • Extend Decimal128Test/Decimal256Test limit coverage with overflow-focused cases matching the reported issue.
File summaries
File Description
cpp/src/arrow/util/decimal.cc Introduces overflow-aware accumulation and signed-range magnitude rejection during decimal parsing.
cpp/src/arrow/util/decimal_test.cc Adds regression tests ensuring oversized Decimal128/Decimal256 strings now return Invalid.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +988 to 992
if (ShiftAndAddWithOverflow(dec.whole_digits, &value, 1) ||
ShiftAndAddWithOverflow(dec.fractional_digits, &value, 1) ||
value > static_cast<uint64_t>(
std::numeric_limits<typename DecimalClass::ValueType>::max())) {
return Status::Invalid("The string '", s, "' cannot be represented as ", type_name);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++] arrow::Decimal128::FromString silently truncates when the input string has more than 38 significant digits

2 participants