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.

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

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

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 all commits
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
31 changes: 18 additions & 13 deletions ext/gmp/gmp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@
#define GMP_BIG_ENDIAN (1 << 3)
#define GMP_NATIVE_ENDIAN (1 << 4)

#if SIZEOF_SIZE_T == 4
#define GMP_ALLOC_MAXBITS (ULONG_MAX / GMP_NUMB_BITS)
#else
#define GMP_ALLOC_MAXBITS INT_MAX
#endif
Comment on lines +50 to +54
Copy link
Member

Choose a reason for hiding this comment

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

The logic to avoid overflow in _mpz_realloc() (libgmp 6.3.0) is:

  if (sizeof (mp_size_t) == sizeof (int))
    {
      if (UNLIKELY (new_alloc > ULONG_MAX / GMP_NUMB_BITS))
	MPZ_OVERFLOW;
    }
  else
    {
      if (UNLIKELY (new_alloc > INT_MAX))
	MPZ_OVERFLOW;
    }

Assuming that mp_size_t is actually size_t, the simplification seems to be correct (we're assuming sizeof(int) == 4 anyway). However, I do not understand how ULONG_MAX fits into the mix. That only works under the assumption that ULONG_MAX == UINT_MAX for 32bit platforms (which is what php-src assumes anyway), but why don't they use UINT_MAX in the first place? I think we should.


#include "gmp_arginfo.h"

ZEND_DECLARE_MODULE_GLOBALS(gmp)
Expand Down Expand Up @@ -1276,14 +1282,17 @@ ZEND_FUNCTION(gmp_fact)
{
zval *a_arg;
mpz_ptr gmpnum_result;
zend_long val;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "z", &a_arg) == FAILURE){
RETURN_THROWS();
}


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");
val = Z_LVAL_P(a_arg);
if (val < 0 || val > GMP_ALLOC_MAXBITS) {
zend_argument_value_error(1, "must be between 0 and " ZEND_LONG_FMT, GMP_ALLOC_MAXBITS);
RETURN_THROWS();
}
} else {
Expand All @@ -1293,14 +1302,16 @@ ZEND_FUNCTION(gmp_fact)
FETCH_GMP_ZVAL(gmpnum, a_arg, temp_a, 1);
FREE_GMP_TEMP(temp_a);

if (mpz_sgn(gmpnum) < 0) {
zend_argument_value_error(1, "must be greater than or equal to 0");
(void)gmpnum;
val = zval_get_long(a_arg);
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand how zval_get_long() fits into the mix here. At least, its usage makes FETCH_GMP__ZVAL() above superflous, and we wouldn't even need the special casing for Z_TYPE_P(a_arg) == IS_LONG (since this is already catered to by zval_get_long().

What happens if one calls gmp_fact("18446744073709551617")? Wouldn't that raise a deprecation notice ('Implicit conversion from float-string "18446744073709551617" to int loses precision') instead of throwing an ValueError.

Copy link
Member Author

Choose a reason for hiding this comment

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

What happens if one calls gmp_fact("18446744073709551617")? Wouldn't that raise a deprecation notice ('Implicit conversion from float-string "18446744073709551617" to int loses precision') instead of throwing an ValueError.

Nope I get the exception in this case too. I ll see the rest later.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, indeed! That is because the conversion happens in coercive mode. And that also shows that we can't easily drop the seemingly superfluous FETCH_GMP_ZVAL() since only that will report issues regarding strict typing mode (e.g. passing a float). So zval_get_long() is basically correct here; the saturating behavior is fine since we cannot compute the factorial of PHP_INT_MAX anyway.

Still, there is an issue unrelated to this PR

E.g. for gmp_fact(new GMP("18446744073709551617")) zval_get_long() returns 1 which passes the overflow check, and evaluates to GMP(1). That is because the mpz_get_si() in gmp_cast_object() overflows, while I would expect saturating behavior. This is not related to this PR, though, but rather a general issue with ext/gmp. From the mpz_get_si() documentation:

If op fits into a signed long int return the value of op. Otherwise return the least significant part of op, with the same sign as op.

If op is too big to fit in a signed long int, the returned result is probably not very useful. To find out if the value will fit, use the function mpz_fits_slong_p.

So apparently, there is an easy fix for gmp_cast_object(). However, on LLP64 mpir 3.0.0 mpz_fits_slong_p() returns false for 2147483648. ;)

Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand how zval_get_long() fits into the mix here. At least, its usage makes FETCH_GMP__ZVAL() above superflous, and we wouldn't even need the special casing for Z_TYPE_P(a_arg) == IS_LONG (since this is already catered to by zval_get_long().

What happens if one calls gmp_fact("18446744073709551617")? Wouldn't that raise a deprecation notice ('Implicit conversion from float-string "18446744073709551617" to int loses precision') instead of throwing an ValueError.

The zval_get_long() is to convert the GMP object back to an int...

Copy link
Member

Choose a reason for hiding this comment

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

The zval_get_long() is to convert the GMP object back to an int...

Yeah, I figured that much, but I wondered about values outside of [ZEND_LONG_MIN, ZEND_LONG_MAX]. However, I found out. :)

if (val < 0 || val > GMP_ALLOC_MAXBITS) {
zend_argument_value_error(1, "must be between 0 and " ZEND_LONG_FMT, GMP_ALLOC_MAXBITS);
RETURN_THROWS();
}
}

INIT_GMP_RETVAL(gmpnum_result);
mpz_fac_ui(gmpnum_result, zval_get_long(a_arg));
mpz_fac_ui(gmpnum_result, val);
}
/* }}} */

Expand Down Expand Up @@ -1859,14 +1870,8 @@ ZEND_FUNCTION(gmp_random_bits)
RETURN_THROWS();
}

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

if (bits <= 0 || bits > maxbits) {
zend_argument_value_error(1, "must be between 1 and " ZEND_LONG_FMT, maxbits);
if (bits <= 0 || bits > GMP_ALLOC_MAXBITS) {
zend_argument_value_error(1, "must be between 1 and " ZEND_LONG_FMT, GMP_ALLOC_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