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

Fix GH-16870: gmp_pow(64, 11) throws overflow exception #16884

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Nov 21, 2024

The current guard to prevent FPEs is way too restrictive; 64 ** 11 is a perfectly reasonable operation. Instead, we now estimate the number of bytes of the resulting GMP (assuming that numbers are stored base 256 encoded), and fail if that exceeds a given threshold. The chosen threshold is somewhat arbitrary.

We also ensure that we do not prematurely convert a given non int base to an int to avoid overflow which could circumvent our early check.


Note that the threshold of 2000 seems way too small. To avoid triggering overflow handling during allocations it should rather be close to SIZE_MAX (or whatever libgmp/mpir assume there), but even 10,000 would cause

try {
gmp_pow(gmp_add(gmp_mul(gmp_init(PHP_INT_MAX), gmp_init(PHP_INT_MAX)), 3), 256);
} catch (\ValueError $e) {
echo $e->getMessage() . PHP_EOL;
}
try {
gmp_pow(gmp_init(PHP_INT_MAX), 256);
} catch (\ValueError $e) {
echo $e->getMessage();
}

to no longer throw, but instead yield reasonable results: numbers with a bit less than 10,000 and 5,000 digits, respectively. It's hard for me to tell whether these examples would actually cause libgmp's overflow check to trigger, since mpir doesn't even have these overflow checks.

In any way, it might not be the best idea to push the limits of our early overflow check just to avoid overflow checks to be triggered by libgmp, since we're likely hitting OOM, which causes abort.

The current guard to prevent FPEs is way too restrictive; `64 ** 11` is
a perfectly reasonable operation.  Instead, we now estimate the number
of bytes of the resulting GMP (assuming that numbers are stored base
256 encoded), and fail if that exceeds a given threshold.  The chosen
threshold is somewhat arbitrary.

We also ensure that we do not prematurely convert a given non int base
to an int to avoid overflow which could circumvent our early check.
@cmb69 cmb69 linked an issue Nov 21, 2024 that may be closed by this pull request
@cmb69
Copy link
Member Author

cmb69 commented Nov 21, 2024

The threshold logic should be unified with #16880 and other places in gmp.c.

ext/gmp/gmp.c Outdated

if (Z_TYPE_P(base_arg) == IS_LONG && Z_LVAL_P(base_arg) >= 0) {
INIT_GMP_RETVAL(gmpnum_result);
if ((log(Z_LVAL_P(base_arg)) * exp) > powmax) {
if ((log(Z_LVAL_P(base_arg)) / log(256) * exp) > powmax) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we just care if it can be "allocated reasonably" ?
Which is actually somewhat described in section 5.14 of the GMP manual (mpz_export)
image

Wonder if we shouldn't use a heuristic based on that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically what is already happening in this patch. The difference is that I'm counting bytes, not bits. The logic is to estimate the number of bytes the result would have, and if this is greater than some threshold (powmax is badly named; should be rather max_size_in_bytes or so), trigger a value error.

mpz_sizeinbase() is only used in the else branch, because if the input is a zend_long, that wouldn't work. So I'm calculating log256(base_arg) * exp instead.

I'll update to counting bytes.

@cmb69
Copy link
Member Author

cmb69 commented Nov 24, 2024

Oh, I had only checked with 64bit builds, but now noticed that two test cases are failing on 32bit. I could further reduce max_bits, but I think gmp_pow(20,10) should actually succeed (result is a number with only 14 decimal digits; even 20**10 works); same for the others that are now failing. Maybe we can agree on a threshold (max_bits) first; I will then adapt test cases as needed (and also rebase, if we do not want to target any stable branches).

@cmb69 cmb69 marked this pull request as ready for review November 24, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gmp_pow(64, 11) throws overflow exception
2 participants