ext/bcmath - Fixed GH-17064: Correctly round rounding mode with zero edge case #17065
Conversation
SakiTakamachi
force-pushed
the
fix/gh17064
branch
from
cc2359a to
8d33f2b
Compare
| @@ -61,7 +96,7 @@ void bc_round(bc_num num, zend_long precision, zend_long mode, bc_num *result) | |||
| * If the result of rounding is carried over, it will be added later, so first set it to 0 here. | |||
| */ | |||
| if (rounded_len == 0) { | |||
| *result = bc_copy_num(BCG(_zero_)); | |||
| *result = bc_new_num(1, 0); | |||
There was a problem hiding this comment.
This is not related to this bug, but it was changing a global value and was dangerous, so I fixed it incidentally.
| foreach ([0, 5, -5] as $scale) { | ||
| $func_ret = bcround($number, $scale, $mode); | ||
| $method_ret = (new BcMath\Number($number))->round($scale, $mode); | ||
| if ($method_ret->compare($func_ret) !== 0) { | ||
| echo "Result is incorrect.\n"; | ||
| var_dump($number, $mode, $func_ret, $method_ret); | ||
| } |
There was a problem hiding this comment.
The indentation added with foreach, making it difficult to see the change. I increased the test cases for scale 0 to test cases for scale 0, 5, and -5.
SakiTakamachi
force-pushed
the
fix/gh17064
branch
from
8d33f2b to
8e0d7dc
Compare
SakiTakamachi
force-pushed
the
fix/gh17064
branch
from
8e0d7dc to
d552f23
Compare
ext/bcmath/libbcmath/src/round.c
Outdated
| } | ||
|
|
||
| /* If precision is -3, it becomes 1000. */ | ||
| *result = bc_new_num(-precision + 1, 0); |
There was a problem hiding this comment.
This line will break under UBSAN for the following code:
<?php
echo bcround("0.01", PHP_INT_MIN, RoundingMode::AwayFromZero);This gives:
/run/media/niels/MoreData/php-8.4/ext/bcmath/libbcmath/src/round.c:74:13: runtime error: negation of -9223372036854775808 cannot be represented in type 'zend_long' (aka 'long'); cast to an unsigned type to negate this value to itself
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /run/media/niels/MoreData/php-8.4/ext/bcmath/libbcmath/src/round.c:74:13
But since this is an extreme edge case that won't be hit in practice, I think it suffices here to just return 0 as a sentinel and throw an exception.
There was a problem hiding this comment.
@nielsdos
Nice point, thanks!
The first argument is size_t, so how about this?
if (UNEXPECTED(precision == ZEND_LONG_MIN)) {
*result = bc_new_num((size_t) ZEND_LONG_MAX + 2, 0);
}
There was a problem hiding this comment.
Sure, it doesn't matter much as it'll fail to allocate that much memory anyway but it's a simple approach.
* PHP-8.4: Correctly round rounding mode with zero edge case (#17065)
fixes #17064