Skip to content

GH-50544: [C++][Gandiva] fix out-of-bounds read in round with most negative precision#50545

Open
Arawoof06 wants to merge 1 commit into
apache:mainfrom
Arawoof06:round-precision-negate-bound
Open

GH-50544: [C++][Gandiva] fix out-of-bounds read in round with most negative precision#50545
Arawoof06 wants to merge 1 commit into
apache:mainfrom
Arawoof06:round-precision-negate-bound

Conversation

@Arawoof06

@Arawoof06 Arawoof06 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

round_int32_int32 and round_int64_int32 negate the caller-supplied precision into abs_precision before range-checking it, so the check abs_precision > 9 (and > 18 for the int64 overload) never fires for precision == INT32_MIN, where the negation leaves the value negative. That value reaches get_power_of_10, which indexes a 19-element static table behind DCHECK_GE/DCHECK_LE only. The DCHECKs compile out in release builds, so round(int_col, -2147483648) reads far outside the table and either faults or returns arbitrary memory as the rounding multiplier. Both overloads are registered in function_registry_arithmetic.cc, so the precision comes straight from user SQL.

truncate_int64_int32 in the same file already compares the signed scale directly (out_scale < -38) and is unaffected; the two round overloads are the only sites that negate first.

What changes are included in this PR?

Check the bound against the signed precision before negating, at both sites. Every precision that was in range before still takes the same path, and the negation now only runs on values known to be within 9 (or 18) of zero.

Are these changes tested?

Yes. Added two cases to TestExtendedMathOps.TestRound covering std::numeric_limits<int32_t>::min() for both overloads, matching the existing truncate_int64_int32 case in the same file. On the unpatched tree the debug build aborts with extended_math_ops.cc:372: Check failed: (exp) >= (0); with the fix the full gandiva-precompiled-test suite passes (134 tests).

Are there any user-facing changes?

No.

This PR contains a "Critical Fix". It fixes an out-of-bounds read reachable from a user-supplied round precision argument.

@github-actions

Copy link
Copy Markdown

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

@github-actions

Copy link
Copy Markdown

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

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.

1 participant