Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

array_sum() with GMP can loose precision (LLP64) #16890

Open
cmb69 opened this issue Nov 21, 2024 · 1 comment · May be fixed by #16891
Open

array_sum() with GMP can loose precision (LLP64) #16890

cmb69 opened this issue Nov 21, 2024 · 1 comment · May be fixed by #16891

Comments

@cmb69
Copy link
Member

cmb69 commented Nov 21, 2024

Description

The following code:

<?php
var_dump(array_sum([new GMP("9223372036854775806"), 1]));

Resulted in this output:

float(9.223372036854775E+18)

But I expected this output instead:

int(9223372036854775807)

This is caused by an erroneous assumption in gmp_cast_object() for _IS_NUMBER casts (as such it can affect other functions as well), namely that ZEND_LONG would be a signed long, but that is not true for LLP64 data models where it is signed long long. On such platforms, values outside of range [INT_MIN, INT_MAX] are converted to float.

PHP Version

PHP-8.2

Operating System

Windows 64bit

@cmb69
Copy link
Member Author

cmb69 commented Nov 21, 2024

Oops. In the meantime I've noticed that array_sum() in PHP-8.2 is not affected by this (since objects are skipped); so the given reproducer does not apply. Still, the issue should be fixed in PHP-8.2, too, since it may affect external extensions. I don't think the bug could be triggered using SimpleXML, which is the only other extension using convert_scalar_to_number() in php-src.

cmb69 added a commit to cmb69/php-src that referenced this issue Nov 21, 2024
We must use `mpz_fits_si_p()` instead of `mpz_fits_slong_p()` since the
latter is not suitable for LLP64 data models.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants