Skip to content

Commit d26582e

Browse files
Arawoof06Arawoof06
authored
GH-50544: [C++][Gandiva] Fix out-of-bounds read in round with most negative precision (#50545)
### 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 Issue: #50544 Authored-by: abdul rawoof <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
1 parent 292674e commit d26582e

2 files changed

Lines changed: 20 additions & 6 deletions

File tree

‎cpp/src/gandiva/precompiled/extended_math_ops.cc‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -324,12 +324,13 @@ gdv_int32 round_int32_int32(gdv_int32 number, gdv_int32 precision) {
324324
if (precision >= 0) {
325325
return number;
326326
}
327-
gdv_int32 abs_precision = -precision;
328327
// This is to ensure that there is no overflow while calculating 10^precision, 9 is
329-
// the smallest N for which 10^N does not fit into 32 bits, so we can safely return 0
330-
if (abs_precision > 9) {
328+
// the smallest N for which 10^N does not fit into 32 bits, so we can safely return 0.
329+
// The bound is checked before negating because -INT32_MIN is still negative.
330+
if (precision < -9) {
331331
return 0;
332332
}
333+
gdv_int32 abs_precision = -precision;
333334
gdv_int32 num_sign = (number > 0) ? 1 : -1;
334335
gdv_int32 abs_number = number * num_sign;
335336
gdv_int32 power_of_10 = static_cast<gdv_int32>(get_power_of_10(abs_precision));
@@ -349,12 +350,13 @@ gdv_int64 round_int64_int32(gdv_int64 number, gdv_int32 precision) {
349350
if (precision >= 0) {
350351
return number;
351352
}
352-
gdv_int32 abs_precision = -precision;
353353
// This is to ensure that there is no overflow while calculating 10^precision, 19 is
354-
// the smallest N for which 10^N does not fit into 64 bits, so we can safely return 0
355-
if (abs_precision > 18) {
354+
// the smallest N for which 10^N does not fit into 64 bits, so we can safely return 0.
355+
// The bound is checked before negating because -INT32_MIN is still negative.
356+
if (precision < -18) {
356357
return 0;
357358
}
359+
gdv_int32 abs_precision = -precision;
358360
gdv_int32 num_sign = (number > 0) ? 1 : -1;
359361
gdv_int64 abs_number = number * num_sign;
360362
gdv_int64 power_of_10 = get_power_of_10(abs_precision);

‎cpp/src/gandiva/precompiled/extended_math_ops_test.cc‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,18 @@ TEST(TestExtendedMathOps, TestRound) {
203203
EXPECT_EQ(round_int64_int32(-23453462343, -4), -23453460000);
204204
EXPECT_EQ(round_int64_int32(-23453462343, -5), -23453500000);
205205
EXPECT_EQ(round_int64_int32(345353425343, -12), 0);
206+
207+
// The last in-range precision still rounds; one past it returns 0. These guard
208+
// against an off-by-one in the range check at both overloads' boundary.
209+
EXPECT_EQ(round_int32_int32(1234567890, -9), 1000000000);
210+
EXPECT_EQ(round_int32_int32(1234567890, -10), 0);
211+
EXPECT_EQ(round_int64_int32(1234567890123456789, -18), 1000000000000000000);
212+
EXPECT_EQ(round_int64_int32(1234567890123456789, -19), 0);
213+
214+
// The most negative precision must be rejected by the range check rather than
215+
// negated into an out-of-range table index.
216+
EXPECT_EQ(round_int32_int32(1234, std::numeric_limits<int32_t>::min()), 0);
217+
EXPECT_EQ(round_int64_int32(345353425343, std::numeric_limits<int32_t>::min()), 0);
206218
}
207219

208220
TEST(TestExtendedMathOps, TestTruncate) {

0 commit comments

Comments
 (0)