Skip to content

ext/gmp: gmp_pow fix FPE with large values. #16384

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

ext/gmp: gmp_pow fix FPE with large values. #16384

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0c04ae6
ext/gmp: gmp_pow fix FPE with large values.
devnexen Oct 12, 2024
e2b07d7
add test
devnexen Oct 12, 2024
3534f0a
add tests
devnexen Oct 21, 2024
72b5653
change comparison limit, now new tests overflow on 64 bits.
devnexen Oct 24, 2024
7f432a1
zend_ulong
devnexen Oct 25, 2024
752627d
fix tests on 32 bits attempt.
devnexen Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 13 additions & 25 deletions ext/gmp/gmp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1282,39 +1282,27 @@ ZEND_FUNCTION(gmp_pow)
RETURN_THROWS();
}

double powmax = log((double)ZEND_LONG_MAX);

if (Z_TYPE_P(base_arg) == IS_LONG && Z_LVAL_P(base_arg) >= 0) {
INIT_GMP_RETVAL(gmpnum_result);
if (exp >= INT_MAX) {
mpz_t base_num, exp_num, mod;
mpz_init(base_num);
mpz_init(exp_num);
mpz_init(mod);
mpz_set_si(base_num, Z_LVAL_P(base_arg));
mpz_set_si(exp_num, exp);
mpz_set_ui(mod, UINT_MAX);
mpz_powm(gmpnum_result, base_num, exp_num, mod);
mpz_clear(mod);
mpz_clear(exp_num);
mpz_clear(base_num);
} else {
mpz_ui_pow_ui(gmpnum_result, Z_LVAL_P(base_arg), exp);
if ((log(Z_LVAL_P(base_arg)) * exp) > powmax) {
zend_value_error("base and exponent overflow");
RETURN_THROWS();
}
mpz_ui_pow_ui(gmpnum_result, Z_LVAL_P(base_arg), exp);
} else {
mpz_ptr gmpnum_base;
zend_ulong gmpnum;
FETCH_GMP_ZVAL(gmpnum_base, base_arg, temp_base, 1);
INIT_GMP_RETVAL(gmpnum_result);
if (exp >= INT_MAX) {
mpz_t exp_num, mod;
mpz_init(exp_num);
mpz_init(mod);
mpz_set_si(exp_num, exp);
mpz_set_ui(mod, UINT_MAX);
mpz_powm(gmpnum_result, gmpnum_base, exp_num, mod);
mpz_clear(mod);
mpz_clear(exp_num);
} else {
mpz_pow_ui(gmpnum_result, gmpnum_base, exp);
gmpnum = mpz_get_ui(gmpnum_base);
Copy link
Member

Choose a reason for hiding this comment

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

This can overflow; e.g. gmp_pow(gmp_init(PHP_INT_MAX), 256) is sufficient for overflow on x64 Windows; on LP64, gmp_pow(gmp_add(gmp_mul(gmp_init(PHP_INT_MAX), gmp_init(PHP_INT_MAX)), 3), 256) is likely to overflow here.

It seems to me that the libgmp/mpir developers where so focused to make it fast, that they forgot to make it right.

Copy link
Member Author

Choose a reason for hiding this comment

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

I ll add those two cases, the latter does not crash on linux/64 locally.

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that mpz_get_ui() returns unsigned long long int (at least on Windows) which is an unsigned 64bit value. If you assign that to an unsigned long, the value will be truncated (well-defined behavior, but likely not desired here). It might be better to declare mpir_ui gmpnum, and then go from there, but that might cause further issues down the line.

if ((log(gmpnum) * exp) > powmax) {
FREE_GMP_TEMP(temp_base);
zend_value_error("base and exponent overflow");
RETURN_THROWS();
}
mpz_pow_ui(gmpnum_result, gmpnum_base, exp);
FREE_GMP_TEMP(temp_base);
}
}
Expand Down
35 changes: 25 additions & 10 deletions ext/gmp/tests/gmp_pow_fpe.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,30 @@ gmp
<?php
$g = gmp_init(256);

var_dump(gmp_pow($g, PHP_INT_MAX));
var_dump(gmp_pow(256, PHP_INT_MAX));
?>
--EXPECTF--
object(GMP)#2 (1) {
["num"]=>
string(%d) "%s"
try {
gmp_pow($g, PHP_INT_MAX);
} catch (\ValueError $e) {
echo $e->getMessage() . PHP_EOL;
}
object(GMP)#2 (1) {
["num"]=>
string(%d) "%s"
try {
gmp_pow(256, PHP_INT_MAX);
} catch (\ValueError $e) {
echo $e->getMessage() . PHP_EOL;
}

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();
}
?>
--EXPECTF--
base and exponent overflow
base and exponent overflow
base and exponent overflow
base and exponent overflow
Loading