Skip to content

Fix GH-16878: gmp_fact overflow on memory allocation attempt. #16880

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

Open
wants to merge 2 commits into
base: PHP-8.2
Choose a base branch
from
Open

Fix GH-16878: gmp_fact overflow on memory allocation attempt. #16880

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10660d8
Fix GH-16878: gmp_fact overflow on memory allocation attempt.
devnexen Nov 21, 2024
ac20c82
changes from feedback
devnexen Nov 21, 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
16 changes: 12 additions & 4 deletions ext/gmp/gmp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1281,20 +1281,28 @@ ZEND_FUNCTION(gmp_fact)
RETURN_THROWS();
}

#if SIZEOF_SIZE_T == 4
const zend_long maxbits = ULONG_MAX / GMP_NUMB_BITS;
#else
const zend_long maxbits = INT_MAX;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

This should be moved to the top-level, since we're already using it in gmp_random_bits(), and may want to re-use it elsewhere. (Could also be a macro; doesn't really matter, I think.)



if (Z_TYPE_P(a_arg) == IS_LONG) {
if (Z_LVAL_P(a_arg) < 0) {
zend_argument_value_error(1, "must be greater than or equal to 0");
if (Z_LVAL_P(a_arg) < 0 || Z_LVAL_P(a_arg) > maxbits) {
zend_argument_value_error(1, "must be between 0 and " ZEND_LONG_FMT, maxbits);
RETURN_THROWS();
}
} else {
mpz_ptr gmpnum;
gmp_temp_t temp_a;

FETCH_GMP_ZVAL(gmpnum, a_arg, temp_a, 1);
long r = mpz_get_si(gmpnum);
Copy link
Member

Choose a reason for hiding this comment

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

Please replace the call to zval_get_long(a_arg) below to use the new variable.

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 use mpz_get_si() here in the first place, since it may overflow for large gmpnum. Possibly use mpz_sizeinbase(gmpnum, 2) instead; that might need some fine-tuning of maxbits, though.

FREE_GMP_TEMP(temp_a);

if (mpz_sgn(gmpnum) < 0) {
zend_argument_value_error(1, "must be greater than or equal to 0");
if (r < 0 || r > maxbits) {
zend_argument_value_error(1, "must be between 0 and " ZEND_LONG_FMT, maxbits);
RETURN_THROWS();
}
}
Expand Down
21 changes: 21 additions & 0 deletions ext/gmp/tests/gh16878.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
--TEST--
GH-16878 (gmp_fact overflow)
--EXTENSIONS--
gmp
--FILE--
<?php
try {
gmp_fact(PHP_INT_MAX);
} catch (\ValueError $e) {
echo $e->getMessage() . PHP_EOL;
}

try {
gmp_fact(gmp_init(PHP_INT_MAX));
} catch (\ValueError $e) {
echo $e->getMessage();
}
?>
--EXPECTF--
gmp_fact(): Argument #1 ($num) must be between 0 and %d
gmp_fact(): Argument #1 ($num) must be between 0 and %d
8 changes: 4 additions & 4 deletions ext/gmp/tests/gmp_fact.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,17 @@ try {

echo "Done\n";
?>
--EXPECT--
--EXPECTF--
string(1) "1"
gmp_fact(): Argument #1 ($num) is not an integer string
string(1) "1"
gmp_fact(): Argument #1 ($num) must be greater than or equal to 0
gmp_fact(): Argument #1 ($num) must be greater than or equal to 0
gmp_fact(): Argument #1 ($num) must be between 0 and %d
gmp_fact(): Argument #1 ($num) must be between 0 and %d
string(19) "2432902008176640000"
string(65) "30414093201713378043612608166064768844377641568960512000000000000"
string(7) "3628800"
string(1) "1"
string(9) "479001600"
gmp_fact(): Argument #1 ($num) must be greater than or equal to 0
gmp_fact(): Argument #1 ($num) must be between 0 and %d
gmp_fact(): Argument #1 ($num) must be of type GMP|string|int, array given
Done
Loading