Repository navigation
MINOR: [C++] Reject out-of-range scale deltas in decimal Rescale - #55159
Closed
Nayana-Naik73 wants to merge 1 commit into
Closed
Nayana-Naik73 wants to merge 1 commit into
Nayana-Naik73 wants to merge 1 commit into
Conversation
|
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: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
DecimalRescale(behindDecimal{32,64,128,256}::Rescale) looks upGetScaleMultiplier(abs(new_scale - original_scale))in thekDecimal*PowersOfTentables, and the only bound on that index is aDCHECK.Decimal*::FromStringrejects a large negative parsed scale but lets a positive one through, and the precision it reports only counts significant digits. So a CSV cell1E-41read asdecimal128(10, 2)parses as precision 1, scale 41, passes the precision check inDecimalValueDecoder::Decode, andRescale(41, 2)reads one entry pastkDecimal128PowersOfTen(ASan reports a global-buffer-overflow READ of size 16 withbasic_decimal.ccbuilt withNDEBUG).1E-100000reads about 1.6 MB past the table. The JSON reader's decimal converter and the defaultcast(string -> decimal)take the same path, and the cast withallow_decimal_truncate=truecallsReduceScaleBy(parsed_scale - out_scale)directly with the same unbounded value. Debug builds abort on theDCHECKfor all of these.What changes are included in this PR?
DecimalRescalereturnskRescaleDataLosswhen the scale delta exceedskMaxScale. No multiplier that large fits in the type, so any non-zero value would overflow or lose digits; zero still rescales to zero.StringToDecimalreturns zero when more thankMaxScaledigits are dropped, which is the truncated result for any value the type can hold.The check sits in
DecimalRescalerather thanFromStringbecauseFromStringintentionally returns scales abovekMaxScale(for example1.23E-8as Decimal32 with scale 10 inDecimalFromStringTest.SmallValues).Are these changes tested?
Yes. New cases in the typed
Rescaletest andDecimal256Test.Rescale, inCast.StringToDecimal(safe and truncating) and in the CSVDecimalConversion.Basicstest. All of them abort on theDCHECKwithout the fix.arrow-utility-test,arrow-compute-scalar-cast-test,arrow-csv-testandarrow-json-testpass on an ASan/UBSan debug build.Are there any user-facing changes?
A decimal string whose scale is further than
kMaxScalefrom the target scale now fails with the usual rescale data-loss error (or truncates to zero whenallow_decimal_truncateis 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:
Reviewed before submission by: