ext/bcmath: bounds-check $precision in bcround() and Number::round()#22182
ext/bcmath: bounds-check $precision in bcround() and Number::round()#22182edorian wants to merge 5 commits into
$precision in bcround() and Number::round()#22182Conversation
Fix ASAN issue withh entry points passed $precision to bc_round() unchecked, allowing PHP_INT_MAX / PHP_INT_MIN to trigger oversized allocations and signed overflow in libbcmath/src/round.c.
- remove the zend_always_inline - use INT_MIN instead of -INT_MAX - use % matching in the tests so they work on 32bit
|
Thank you for finding this. That said, I think it may be better to fix the code in round.c instead. My reasoning is based on the responsibility of the code and where this behavior should ideally be handled. I’d like to think through a possible solution. |
|
Sorry, but could you clarify on which specific lines in In my environment, memory is exhausted first, so I was unable to reproduce the issue. |
Sorry for not posting the error message, @SakiTakamachi Running the test:
No --asan, no env:
With
The standalone reproducer for the Asan request allocation error: Either way, this shouldn't OOM/Asan I'd say? |
|
Thank you! First, allocation failures can occur in other functions as well, so it is not immediately clear whether we should focus solely on resolving the issue in BCMath. e.g. : However, I noticed that a different issue occurs with code like the following: Since the maximum value of BCMath’s scale is defined as the maximum value of a 32-bit signed integer, this can happen. For this issue, it would probably be best to check for it in Since there is no limit on the integer part, we should check whether edit: |
I'm also ok with the resolution of this being a "not an issue / won't fix" resolution. The upside is minor and I don't want to be a burden here.
php > $a = str_repeat(rand(0, 9), PHP_INT_MAX); That for me is a regular "OOM" error while If I'm mistaken here I apologize!
I don't follow on only looking at the positive direction. Precision may be negative number according to the docs. So ensuring bounds in both directions felt correct to me. Would you suggest something liek this for round.c? - /* If precision is -3, it becomes 1000. */
- if (UNEXPECTED(precision == ZEND_LONG_MIN)) {
- *result = bc_new_num((size_t) ZEND_LONG_MAX + 2, 0);
- } else {
- *result = bc_new_num(-precision + 1, 0);
- }
+ /* If precision is -3, it becomes 1000. Negate in unsigned space so
+ * precision == ZEND_LONG_MIN doesn't overflow signed long. */
+ zend_ulong magnitude = -(zend_ulong) precision;
+ *result = bc_new_num((size_t) magnitude + 1, 0);While the tests for this run I can't really tell if that's the |
On 32-bit, SIZEOF_INT == SIZEOF_ZEND_LONG and there's no zend_long value on 32-bit that can exceed INT_MAX
edorian
force-pushed
the
bc-math-out-of-bounds-precission
branch
from
047d19b to
da80c86
Compare
From my understanding, it doesn’t look like an overflow can occur at round.c:74. Looking at the ASan output, I also don’t see anything that indicates an overflow occurred there. Is there any additional log output that hasn’t been shared yet?
Sorry, my wording was unclear. That said, my comment was based on the assumption that no overflow is actually occurring in round.c and that this is simply a regular OOM error. So I’d prefer to put this discussion on hold for now. |
Here's a reproducer that hopefully makes sense? =>
-- That said: Please let me know how you'd like to proceed.
|
3 participants
Fix the ASAN issue with entry points passing
$precisiontobc_round()unchecked, allowing PHP_INT_MAX / PHP_INT_MIN to trigger oversized allocations and signed overflow in libbcmath/src/round.c.Transparency Note: The ASAN issue was found during LLM-based scanning. Code partially generated by tooling, reviewed and tested by me.