php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #79595 zend_init_fpu() alters FPU precision
Submitted: 2020-05-13 23:48 UTC Modified: 2021-06-10 09:33 UTC
From: v-yitam at microsoft dot com Assigned: cmb (profile)
Status: Closed Package: Math related
PHP Version: 7.4.5 OS: Alpine Linux
Private report: No CVE-ID: None
 [2020-05-13 23:48 UTC] v-yitam at microsoft dot com
Description:
------------
A PHP extension which depends on the FPU precision being the default of the ABI (80 bit) will produce subtly incorrect results with floating point number. 

In particular, it breaks any musl libc-linked binaries that use floating point strtod(). In Alpine Linux, musl's strtod (https://git.musl-libc.org/cgit/musl/tree/src/internal/floatscan.c) depends on long double, which requires FPU set to 80-bit precision to actually work

To illustrate, I wrote two simple test scripts (one cpp and one php), using the example provided in a similar issue (https://github.com/microsoft/WSL/issues/830):

The output from the test program: 1.00000000000000000e-05
The output from running php:      1.00000000000000008e-5

Please see below.

Test script:
---------------
alpine:~$ more test_print.cpp
#include <stdio.h>

int main()
{
  long double n = 1;
  printf("%.17Le\n", n / 100000);
}
alpine:~$ g++ -std=c++11 -static -o test_print test_print.cpp
alpine:~$ ./test_print
1.00000000000000000e-05

alpine:~$ more test_print.cpp
#include <stdio.h>

int main()
{
  long double n = 1;
  printf("%.17Le\n", n / 100000);
}
alpine:~$ g++ -std=c++11 -static -o test_print test_print.cpp
alpine:~$ ./test_print
1.00000000000000000e-05
alpine:~$ more test_print.php
<?php

$n = 1;
printf("%.17e\n", 1/100000);

?>

alpine:~$ php test_print.php
1.00000000000000008e-5



Expected result:
----------------
FPU precision should not be changed

Actual result:
--------------
Have to save/restore FPU state and force it back to 80-bit 

Patches

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2020-05-14 17:40 UTC] v-yitam at microsoft dot com
I wanted to modify my original bug report but it didn't seem possible. Anyway, below is a more accurate example to illustrate the issue. It was copied when running the debugger and stepping through zend_init_fpu() in Zend/zend_float.c:


24	{
(gdb) s
28		if (!EG(saved_fpu_cw_ptr)) {
(gdb) n
29			EG(saved_fpu_cw_ptr) = (void*)&EG(saved_fpu_cw);
(gdb) n
31		XPFPA_STORE_CW(EG(saved_fpu_cw_ptr));
(gdb) n
32	long double n = 1;                                                           
(gdb) n
33	printf("Before %.17Le\n", n/100000);
(gdb) n
Before 1.00000000000000000e-05
35		XPFPA_SWITCH_DOUBLE();
(gdb) n
36	printf("After %.17Le\n", n/100000);
(gdb) n
After 1.00000000000000008e-05
40	}
(gdb)
 [2020-05-18 07:26 UTC] cmb@php.net
From a comment in zend_float.h:

| This header file defines several platform-dependent macros that
| ensure equal and deterministic floating point behaviour across
| several platforms, compilers and architectures.

So, apparently, this is done deliberately.  I'm not sure that this
is a good idea, though.
 [2020-05-19 17:20 UTC] v-yitam at microsoft dot com
Thanks for getting back to us. Indeed, I suspect so:

/* NOTE: This only sets internal precision. MSVC does NOT support double-
   extended precision! */
# define XPFPA_SWITCH_DOUBLE_EXTENDED()

If I changed the macro XPFPA_SWITCH_DOUBLE() to XPFPA_SWITCH_DOUBLE_EXTENDED(), the problem was resolved in Alpine Linux.

Hence, please investigate further why using XPFPA_SWITCH_DOUBLE() is necessary. On Linux the FPU should be left at 80-bit precision, especially Alpine, as musl libc's strtod() requires it to work properly, not to mention it also breaks any functions that expect the standard ABI on Linux.
 [2020-05-20 11:09 UTC] nikic@php.net
Are you interested in this for 32-bit or 64-bit builds?

I think we can safely disable this code if compiling under __SSE__, because we know that PHP itself will not use x87 FPU in that case.

For 32-bit builds targeting x87 FPU, I don't think we want to change this, as we do want normal floating point operations to be based on double precision.
 [2020-05-20 15:20 UTC] v-yitam at microsoft dot com
This floating point calculation issue happens in Alpine Linux only, so we are targeting 64-bit builds for Linux only. Thanks for considering!
 [2020-05-20 15:50 UTC] cmb@php.net
A possible fix might be <https://github.com/php/php-src/pull/5602>.
Could you please try that, @yitam?
 [2020-05-20 15:56 UTC] v-yitam at microsoft dot com
My pleasure! Will get back to you later.
 [2020-05-20 16:53 UTC] v-yitam at microsoft dot com
The patch seems to fix the issue after the preliminary round of testing in Alpine Linux with PHP 7.4.5. Congratulations! Please let us know when this patch will be merged to a stable release.
 [2020-05-22 07:38 UTC] cmb@php.net
-Assigned To: +Assigned To: cmb
 [2020-05-22 07:39 UTC] cmb@php.net
The following pull request has been associated:

Patch Name: Fix #79595: zend_init_fpu() alters FPU precision
On GitHub:  https://github.com/php/php-src/pull/5602
Patch:      https://github.com/php/php-src/pull/5602.patch
 [2020-05-22 13:48 UTC] cmb@php.net
Automatic comment on behalf of cmbecker69@gmx.de
Revision: http://git.php.net/?p=php-src.git;a=commit;h=88dfc475c5937822399843e8aed9b98a36a01813
Log: Fix #79595: zend_init_fpu() alters FPU precision
 [2020-05-22 13:48 UTC] cmb@php.net
-Status: Assigned +Status: Closed
 [2020-05-22 13:50 UTC] cmb@php.net
The fix is scheduled to be released with PHP 7.4.7.
 [2020-05-22 14:57 UTC] nikic@php.net
Automatic comment on behalf of nikita.ppv@gmail.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=10eb842a6434f7b05dbb8676da57ada445f4434f
Log: Revert &quot;Fix #79595: zend_init_fpu() alters FPU precision&quot;
 [2020-05-25 08:40 UTC] cmb@php.net
The following pull request has been associated:

Patch Name: Fix #79595: zend_init_fpu() alters FPU precision
On GitHub:  https://github.com/php/php-src/pull/5621
Patch:      https://github.com/php/php-src/pull/5621.patch
 [2020-05-25 08:41 UTC] cmb@php.net
-Status: Closed +Status: Re-Opened
 [2020-05-25 08:41 UTC] cmb@php.net
The fix had to be reverted due to unforeseen issues on
i386.  PR #5621 is hopefully a better fix.
 [2020-05-25 17:15 UTC] v-yitam at microsoft dot com
Thank you all for working hard on fixing this issue. I just tested the new proposed fix but unfortunately it didn't work in Alpine Linux. I added the following 'debugging' lines in zend_float.c:

ZEND_API void zend_init_fpu(void) /* {{{ */                                
{                                                                          
int x = -1;                                                                
#if defined(HAVE__CONTROLFP_S) && !defined(__x86_64__)                     
    x = 1;                                                                 
#elif defined(HAVE__CONTROLFP) && !defined(__x86_64__)                     
    x = 2;                                                                 
#elif defined(HAVE__FPU_SETCW)  && !defined(__x86_64__)                    
    x = 3;                                                                 
#elif defined(HAVE_FPSETPREC)  && !defined(__x86_64__)                     
    x = 4;                                                                 
#elif defined(HAVE_FPU_INLINE_ASM_X86)                                     
    x = 5;                                                                 
#else                                                  
    x = 0;                                             
#endif         


Then I ran the debugger:


Breakpoint 1, zend_init_fpu ()
    at /home/jennyt/php-7.4.6/Zend/zend_float.c:24
24	{
(gdb) s
25	int x = -1;
(gdb) s
36	    x = 5;
(gdb)



Hope this helps!
 [2020-05-26 07:03 UTC] cmb@php.net
Thanks for checking, and yes, this was helpful!  I had forgotten
to add the guard to that #elif[1].

[1] <https://github.com/php/php-src/pull/5621/commits/c3fe3f7424b7e9a793ef4b55f197de9e2b6593af>
 [2020-05-26 15:22 UTC] cmb@php.net
Automatic comment on behalf of cmbecker69@gmx.de
Revision: http://git.php.net/?p=php-src.git;a=commit;h=879004dae325e50d3e1a8f9477e66cdfeef0d366
Log: Fix #79595: zend_init_fpu() alters FPU precision
 [2020-05-26 15:22 UTC] cmb@php.net
-Status: Re-Opened +Status: Closed
 [2020-05-26 15:24 UTC] cmb@php.net
Note that the fix missed the deadline for PHP 7.4.7, so will have
to wait for PHP 7.4.8 (roughly mid July).
 [2020-05-26 16:23 UTC] v-yitam at microsoft dot com
> Note that the fix missed the deadline for PHP 7.4.7, so will have
to wait for PHP 7.4.8 (roughly mid July).

No worries. We will jot it down. A brief testing shows that the new fix works. Thank you all!
 [2021-06-10 01:42 UTC] bugdal at aerifal dot cx
The fix is not a fix, or at least not a complete one; it leaves at least 32-bit x86 broken.

If php/zend is really depending on having the fpu in a nonstandard precision mode, some logic to switch in and out of this mode when making external calls is needed; you can't just call external code with a state that violates the ABI that code is expecting.

Also, if you're using the "64-bit" x87 precision mode and assuming it yields behavior equivalent to IEEE double arithmetic, this is not correct. In this mode, computations and x87 registers have a 53-bit significand like IEEE double, but still have a 15-bit exponent like Intel 80-bit extended. This means that you can end up with values which are larger in magnitude than what double can represent, or that have more precision than they should due to being in the denormal range as double but normal with the 15-bit exponent. Such values change in unstable ways when the compiler spills and reloads them, leading to all sorts of catastrophic dangerous bugs. These go back to GCC issue https://gcc.gnu.org/bugzilla/show_bug.cgi?id=323 where many of them are linked, but there are a lot of newer, more specific bug reports as well, including one whereby *integer* code can be transformed incorrectly by the optimizer.

In short, not only is it unsafe to use the nonstandard x87 precision modes; it's also unsafe to do floating point without -fexcess-precision=standard (implied by -std=c* but not by -std=gnu*) on targets with excess precision (32-bit x86 and m68k).
 [2021-06-10 09:33 UTC] nikic@php.net
> If php/zend is really depending on having the fpu in a nonstandard precision mode, some logic to switch in and out of this mode when making external calls is needed; you can't just call external code with a state that violates the ABI that code is expecting.

I don't think that's feasible, there are too many external calls. I would expect it to work the other way around, i.e. only switch the precision mode for code segments that need it. I'm not sure what those are, but I expect at least the rounding implementation.

I'd be willing to review a PR in that direction, but given the irrelevance of i386 *without* sse2 as a contemporary architecture, I have no interest in pursuing this myself.
 
PHP Copyright © 2001-2021 The PHP Group
All rights reserved.
Last updated: Mon Oct 25 11:03:35 2021 UTC