Skip to content

MINOR: [C++] Reject out-of-range scale deltas in decimal Rescale - #55159

Closed
Nayana-Naik73 wants to merge 1 commit into
apache:mainfrom
Nayana-Naik73:decimal-rescale-scale-bound
Closed

Nayana-Naik73 wants to merge 1 commit into
apache:mainfrom
Nayana-Naik73:decimal-rescale-scale-bound

Conversation

@Nayana-Naik73

Copy link
Copy Markdown

Rationale for this change

DecimalRescale (behind Decimal{32,64,128,256}::Rescale) looks up GetScaleMultiplier(abs(new_scale - original_scale)) in the kDecimal*PowersOfTen tables, and the only bound on that index is a DCHECK. Decimal*::FromString rejects a large negative parsed scale but lets a positive one through, and the precision it reports only counts significant digits. So a CSV cell 1E-41 read as decimal128(10, 2) parses as precision 1, scale 41, passes the precision check in DecimalValueDecoder::Decode, and Rescale(41, 2) reads one entry past kDecimal128PowersOfTen (ASan reports a global-buffer-overflow READ of size 16 with basic_decimal.cc built with NDEBUG). 1E-100000 reads about 1.6 MB past the table. The JSON reader's decimal converter and the default cast(string -> decimal) take the same path, and the cast with allow_decimal_truncate=true calls ReduceScaleBy(parsed_scale - out_scale) directly with the same unbounded value. Debug builds abort on the DCHECK for all of these.

What changes are included in this PR?

  • DecimalRescale returns kRescaleDataLoss when the scale delta exceeds kMaxScale. No multiplier that large fits in the type, so any non-zero value would overflow or lose digits; zero still rescales to zero.
  • The truncating branch of StringToDecimal returns zero when more than kMaxScale digits are dropped, which is the truncated result for any value the type can hold.

The check sits in DecimalRescale rather than FromString because FromString intentionally returns scales above kMaxScale (for example 1.23E-8 as Decimal32 with scale 10 in DecimalFromStringTest.SmallValues).

Are these changes tested?

Yes. New cases in the typed Rescale test and Decimal256Test.Rescale, in Cast.StringToDecimal (safe and truncating) and in the CSV DecimalConversion.Basics test. All of them abort on the DCHECK without the fix. arrow-utility-test, arrow-compute-scalar-cast-test, arrow-csv-test and arrow-json-test pass on an ASan/UBSan debug build.

Are there any user-facing changes?

A decimal string whose scale is further than kMaxScale from the target scale now fails with the usual rescale data-loss error (or truncates to zero when allow_decimal_truncate is set) instead of reading out of bounds.

This PR contains a "Critical Fix". Untrusted CSV or JSON input, or a string column passed to cast, triggers an out-of-bounds read of a global table in release builds, and the garbage multiplier can then produce an incorrect converted value.

Was AI used for this PR?

In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.

PR code and description written by:

  • Human
  • AI

Reviewed before submission by:

  • Human
  • AI
  • Not reviewed

@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

This pull request has been automatically closed because you currently have 4 open pull requests, which is more than the limit of 3.

Due to the increase in pull requests opened by AI bots, and in order to keep the review queue manageable, Apache Arrow limits contributors without repository access to at most 3 concurrently open pull requests. This helps make sure each pull request gets the attention it needs and that work in progress does not go stale.

Once one of your other open pull requests has been merged or closed, you are welcome to reopen this one.

See also:

@github-actions github-actions Bot closed this Oct 23, 2026
@github-actions github-actions Bot added the awaiting review Awaiting review label Oct 23, 2026
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.

1 participant