number_format: prevent integer overflow on big decimal number

[marc-mabe]

Passing big numbers as decimal places to number_format was resulting in integer overflow with unexpected results.

Unlike round, which falls back to INT_MAX/MIN, the function number_format needs to know for how many places after the decimal point the number needs to be formatted. This is why I changed the argument from int to zend_long which is an internal API BC break.

For passing big negative decimals I was able to provide a test but unfortunately passing big positive decimal places results in OOM error (as expected). On my system with memory_limit=-1 and enough free mem I was able to test it manually:

<?php

var_dump(number_format(1.2345, PHP_INT_MIN, '', ''));
$s = number_format(1.2345, 2147483648, '', '');
fwrite(STDOUT, $s, 100);
fwrite(STDOUT, "\n");

Before change:

$ php -d 'memory_limit=-1' /tmp/test.php
string(1) "1"
1

After change:

$ ./sapi/cli/php -d 'memory_limit=-1' /tmp/test.php
string(1) "0"
1234499999999999930722083263390231877565383911132812500000000000000000000000000000000000000000000000

[marc-mabe]

Internals: https://externals.io/message/120769

[iluuu1994]

Passing big decimals is dubious, but an arbitrary upper limit is probably not better.

[bukka]

LGTM

@marc-mabe Can you please resolve merge conflicts and squash it?

Should we also have maybe version for lower branches as I guess the positive overflow is already bug there, right?

[marc-mabe]

@bukka

@marc-mabe Can you please resolve merge conflicts and squash it?

done

Should we also have maybe version for lower branches as I guess the positive overflow is already bug there, right?

Not sure.

As it's dubious passing big numbers as decimals (#11649 (review)), the integer overflow does not result in crashes and the fix contains an internal BC break even though I couldn't find any usage of _php_math_number_format[_ex] it could be used in extensions.

If you prefer this to be fixed down the line I can provide a PR.

[iluuu1994]

Should we also have maybe version for lower branches as I guess the positive overflow is already bug there, right?

Switching from int to zend_long in _php_math_number_format[_ex] is an ABI break. If we want to solve this for older branches we'll need a different solution, like handling overflow directly from number_format.

[marc-mabe]

Should we also have maybe version for lower branches as I guess the positive overflow is already bug there, right?

Switching from int to zend_long in _php_math_number_format[_ex] is an ABI break. If we want to solve this for older branches we'll need a different solution, like handling overflow directly from number_format.

I can provide another PR for the 8.2 branch

[marc-mabe]

@bukka @iluuu1994 See #11714 hope that's the correct way of providing a fix for lower versions

[iluuu1994]

@marc-mabe That PR looks good. As the same issue likely applies to PHP 8.1, it should actually be backported to that branch. Can you rebase it? If you also copy the changes for the tests you made in this PR I think it should completely replace this one, as the changes are simpler.

[marc-mabe]

@iluuu1994

@marc-mabe That PR looks good. As the same issue likely applies to PHP 8.1, it should actually be backported to that branch. Can you rebase it? If you also copy the changes for the tests you made in this PR I think it should completely replace this one, as the changes are simpler.

With this PR I follow the expected behavior. Means passing e.g. 2147483648 as decimals, a number with 2147483648 decimal places will be generated. On #11714, without the BC break, the number gets truncated at 2147483647 decimal places.

Additionally on < 8.3 I'm not able to apply a test for big negative decimals because this gets supported in >= 8.3 (see #11487) and for big positive decimals I'm only be able to test manually as the test requires at least 2g of additionally free memory to work which isn't guarantied on CI - is it ?

[nielsdos]

I approved their 8.1&8.2 PR of this, and as per #11714 (review) I can handle the merge. As it's only easily testable on 8.3 I can take the test from this PR too during the merge into 8.3.
I believe that should correctly handle both PRs (and close both of them).