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

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

Closed
wants to merge 6 commits into from

Conversation

devnexen
Copy link
Member

even without sanitizers, it is reproducible but with the following

<?php
$g = gmp_init(256);
var_dump(gmp_pow($g, PHP_INT_MAX));

we get this

AddressSanitizer:DEADLYSIGNAL
=================================================================
==286922==ERROR: AddressSanitizer: FPE on unknown address 0x03e8000460ca (pc 0x7faf6c69de5c bp 0x400000000000004 sp 0x7ffe9843c740 T0)
    #0 0x7faf6c69de5c in __pthread_kill_implementation nptl/pthread_kill.c:44
    #1 0x7faf6c649c81 in __GI_raise ../sysdeps/posix/raise.c:26
    #2 0x7faf6db9386c in __gmp_exception (/lib/x86_64-linux-gnu/libgmp.so.10+0xd86c) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    #3 0x7faf6db938d3 in __gmp_overflow_in_mpz (/lib/x86_64-linux-gnu/libgmp.so.10+0xd8d3) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    #4 0x7faf6dbac95c in __gmpz_realloc (/lib/x86_64-linux-gnu/libgmp.so.10+0x2695c) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    #5 0x7faf6dba9038 in __gmpz_n_pow_ui (/lib/x86_64-linux-gnu/libgmp.so.10+0x23038) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    #6 0x5565ae1ccd9f in zif_gmp_pow /home/dcarlier/Contribs/php-src/ext/gmp/gmp.c:1286
    #7 0x5565aee96ea9 in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:1312
    #8 0x5565af144320 in execute_ex /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:56075
    #9 0x5565af160f07 in zend_execute /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:60439
    #10 0x5565aed6fafe in zend_execute_scripts /home/dcarlier/Contribs/php-src/Zend/zend.c:1842
    #11 0x5565aeae70a8 in php_execute_script /home/dcarlier/Contribs/php-src/main/main.c:2578
    #12 0x5565af532f4e in do_cli /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:964
    #13 0x5565af535877 in main /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:1334
    #14 0x7faf6c633d67 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #15 0x7faf6c633e24 in __libc_start_main_impl ../csu/libc-start.c:360
    #16 0x5565adc04040 in _start (/home/dcarlier/Contribs/php-src/sapi/cli/php+0x2604040) (BuildId: 949049955bdf8b7197390b1978a1dfc3ef6fdf38)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: FPE nptl/pthread_kill.c:44 in __pthread_kill_implementation
==286922==ABORTING

ext/gmp/gmp.c Outdated
Comment on lines 1279 to 1307
if (Z_TYPE_P(base_arg) == IS_LONG && Z_LVAL_P(base_arg) >= 0) {
INIT_GMP_RETVAL(gmpnum_result);
if ((log10(Z_LVAL_P(base_arg)) * exp) > (double)ULONG_MAX) {
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;
unsigned long gmpnum;
FETCH_GMP_ZVAL(gmpnum_base, base_arg, temp_base, 1);
INIT_GMP_RETVAL(gmpnum_result);
gmpnum = mpz_get_ui(gmpnum_base);
if ((log10(gmpnum) * exp) > (double)ULONG_MAX) {
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);
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should instead turn the int argument into mpz_t and use mpz_powm for cases where the integer is greater than an unsigned int?

@devnexen devnexen marked this pull request as draft October 12, 2024 15:13
@devnexen devnexen marked this pull request as ready for review October 12, 2024 17:27
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Thank you, looks simpler and cleaner :D

ext/gmp/gmp.c Outdated
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);
Copy link
Member

Choose a reason for hiding this comment

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

Why compute the power modulo UINT_MAX ?

Copy link
Member Author

Choose a reason for hiding this comment

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

seems large enough and works for both 32/64. What do you have in mind ?

Copy link
Member

Choose a reason for hiding this comment

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

I just don't understand why you need to take a modulo. Would this not give the wrong result for e.g. base=2 exp=INT_MAX ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is more to put a "threshold", originally I implemented an exception.

Copy link
Member

Choose a reason for hiding this comment

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

My point is that

var_dump(gmp_pow(2, 2**31));

now outputs

object(GMP)#1 (1) {
  ["num"]=>
  string(1) "1"
}

which is wrong.

I rather have an exception than a wrong result.

Copy link
Member Author

@devnexen devnexen Oct 16, 2024

Choose a reason for hiding this comment

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

I know it s wrong :) @Girgias what do you think if I go back to my original idea ?

Copy link
Member

Choose a reason for hiding this comment

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

Right, mpz does not have a mpz_pow function, which is kinda strange. Throwing an exception is indeed better.

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.

@devnexen devnexen marked this pull request as draft October 21, 2024 14:39
@devnexen devnexen marked this pull request as ready for review October 21, 2024 16:53
@devnexen devnexen requested a review from cmb69 October 23, 2024 18:33
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Sorry that I can't be more constructive here, but I barely understand the libraries, and how we're using them.

And it still seems to me that the question should not be "how can we avoid abort when calling libgmp/mpir", but rather "how can we avoid calling libgmp/mpir". ;)

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.

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.

ext/gmp/gmp.c Outdated
mpz_clear(base_num);
} else {
mpz_ui_pow_ui(gmpnum_result, Z_LVAL_P(base_arg), exp);
if ((log10(Z_LVAL_P(base_arg)) * exp) > (double)ULONG_MAX) {
Copy link
Member

@cmb69 cmb69 Oct 23, 2024

Choose a reason for hiding this comment

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

ULONG_MAX is only 2.147.483.647 4294967295 on LLP64 (e.g. Windows 64bit). So that might constrain the range of allowed values too much. On the hand, on 32bit Windows that almost certainly can trigger OOM (e.g. 10 ** PHP_INT_MAX).

Copy link
Member Author

@devnexen devnexen Oct 24, 2024

Choose a reason for hiding this comment

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

It might be better to declare mpir_ui gmpnum, and then go from there, but that might cause further issues down the line.

Note that the mpir_ui type does not exist in gmp (mpir lib ?). unsigned long int might be appropriate however.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, of course; mpir_ui is an mpir type. unsigned long int is the same as unsigned long. We can probably use zend_ulong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry yes I missed this The problem is that mpz_get_ui() returns unsigned long long int (at least on Windows)

even without sanitizers, it is reproducible but with the following

```
<?php
$g = gmp_init(256);
var_dump(gmp_pow($g, PHP_INT_MAX));
```

we get this

```
AddressSanitizer:DEADLYSIGNAL
=================================================================
==286922==ERROR: AddressSanitizer: FPE on unknown address 0x03e8000460ca (pc 0x7faf6c69de5c bp 0x400000000000004 sp 0x7ffe9843c740 T0)
    #0 0x7faf6c69de5c in __pthread_kill_implementation nptl/pthread_kill.c:44
    #1 0x7faf6c649c81 in __GI_raise ../sysdeps/posix/raise.c:26
    php#2 0x7faf6db9386c in __gmp_exception (/lib/x86_64-linux-gnu/libgmp.so.10+0xd86c) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    php#3 0x7faf6db938d3 in __gmp_overflow_in_mpz (/lib/x86_64-linux-gnu/libgmp.so.10+0xd8d3) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    php#4 0x7faf6dbac95c in __gmpz_realloc (/lib/x86_64-linux-gnu/libgmp.so.10+0x2695c) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    php#5 0x7faf6dba9038 in __gmpz_n_pow_ui (/lib/x86_64-linux-gnu/libgmp.so.10+0x23038) (BuildId: 1af68a49fe041a5bb48a2915c3d47541f713bb38)
    php#6 0x5565ae1ccd9f in zif_gmp_pow /home/dcarlier/Contribs/php-src/ext/gmp/gmp.c:1286
    php#7 0x5565aee96ea9 in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:1312
    php#8 0x5565af144320 in execute_ex /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:56075
    php#9 0x5565af160f07 in zend_execute /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:60439
    php#10 0x5565aed6fafe in zend_execute_scripts /home/dcarlier/Contribs/php-src/Zend/zend.c:1842
    php#11 0x5565aeae70a8 in php_execute_script /home/dcarlier/Contribs/php-src/main/main.c:2578
    php#12 0x5565af532f4e in do_cli /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:964
    php#13 0x5565af535877 in main /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:1334
    php#14 0x7faf6c633d67 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    php#15 0x7faf6c633e24 in __libc_start_main_impl ../csu/libc-start.c:360
    php#16 0x5565adc04040 in _start (/home/dcarlier/Contribs/php-src/sapi/cli/php+0x2604040) (BuildId: 949049955bdf8b7197390b1978a1dfc3ef6fdf38)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: FPE nptl/pthread_kill.c:44 in __pthread_kill_implementation
==286922==ABORTING
```
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

I guess that is as good as possible.

note: using consistent "style" for test on purpose.
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.

4 participants