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

Draft
wants to merge 1 commit 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.

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
1 participant