Skip to content

GH-50964: [C++] fix out-of-bounds read in DecimalRescale for out-of-range scale - #50965

Open
Arawoof06 wants to merge 1 commit into
apache:mainfrom
Arawoof06:decimal-rescale-scale-bound
Open

GH-50964: [C++] fix out-of-bounds read in DecimalRescale for out-of-range scale#50965
Arawoof06 wants to merge 1 commit into
apache:mainfrom
Arawoof06:decimal-rescale-scale-bound

Conversation

@Arawoof06

Copy link
Copy Markdown
Contributor

Rationale for this change

DecimalRescale indexes the scale-multiplier table through GetScaleMultiplier(abs_delta_scale), but that table only holds kMaxScale + 1 entries and is guarded by a DCHECK that disappears under NDEBUG. Decimal::FromString clamps a negative parsed scale but leaves a positive one unbounded, so a literal like 1E-100 parses to precision 1 and scale 100. In the CSV decimal converter that passes the precision check and then calls Rescale(100, 0), reading kDecimal128PowersOfTen[100] well past the end of the 39-entry table. The same path is reachable from the public Decimal32/64/128/256::FromString and Rescale APIs. UBSan flags it as index 100 out of bounds for type 'const BasicDecimal128[39]'.

What changes are included in this PR?

Reject a scale delta whose magnitude exceeds kMaxScale in DecimalRescale before the lookup, returning kRescaleDataLoss. A change that large can never be represented without overflow or truncation anyway. The check lives in the one templated function so it covers all four decimal widths, and testing the delta before std::abs also keeps std::abs(INT32_MIN) out of reach.

Are these changes tested?

Yes. The typed Rescale test now asserts a delta past kMaxScale fails in both directions (a delta of exactly kMaxScale is still exercised by the existing loops). A new Decimal128 case parses 1E-100, confirms scale 100, and checks the following Rescale returns Invalid instead of reading past the table. Built with -fsanitize=bounds, the prior code aborts at GetScaleMultiplier; with the fix it returns cleanly.

Are there any user-facing changes?

A decimal string whose scale exceeds the type maximum now returns an Invalid status from Rescale rather than reading out of bounds. Valid inputs are unchanged.

This PR contains a "Critical Fix". An out-of-bounds read of a static table, reachable from untrusted decimal strings (for example CSV values), that the DCHECK bound does not catch in release builds.

@Arawoof06
Arawoof06 requested a review from pitrou as a code owner August 24, 2026 09:08
@github-actions github-actions Bot added the awaiting review Awaiting review label Aug 24, 2026
@github-actions

Copy link
Copy Markdown

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

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