php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #70173 ZVAL_COPY_VALUE_EX broken for 32bit Solaris Sparc
Submitted: 2015-07-31 05:53 UTC Modified: 2015-08-09 14:28 UTC
From: rainer dot jung at kippdata dot de Assigned:
Status: Closed Package: Scripting Engine problem
PHP Version: 7.0.0beta3 OS: Solaris 10 Sparc
Private report: No CVE-ID:
 [2015-07-31 05:53 UTC] rainer dot jung at kippdata dot de
Description:
------------
A precision problem in float makes a huge number of tests fail. It could well be, that this is a platform specific failure (Solaris 10 Sparc, 32 Bit build).

Below you will find a test case with correct result returned from version 5.6.11 and wrong result from 7.0.0 Beta 2.


Test script:
---------------
<?php
$var = 29000000;
var_dump($var);
$var = 290000000;
var_dump($var);
$var = 2900000000;
var_dump($var);
$var = 29000000000;
var_dump($var);
$var = 290000000000;
var_dump($var);
$var = 2900000000000;
var_dump($var);
$var = 29000000000000;
var_dump($var);
$var = 290000000000000;
var_dump($var);
$var = 2900000000000000;
var_dump($var);
?>

Expected result:
----------------
int(29000000)
int(290000000)
float(2900000000)
float(29000000000)
float(290000000000)
float(2900000000000)
float(29000000000000)
float(2.9E+14)
float(2.9E+15)


Actual result:
--------------
int(29000000)
int(290000000)
float(2899998720)
float(28999991296)
float(289999945728)
float(2899998408704)
float(28999988281344)
float(2.899999499223E+14)
float(2.8999984254812E+15)


Patches

70173_Standalone_Reproduction.c (last revision 2015-08-08 21:55 UTC) by rainer dot jung at kippdata dot de)

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-08-03 13:06 UTC] cmb@php.net
That might be related to the update of Zend/zend_strtod*[1].

[1] <https://github.com/php/php-src/commit/5d4616e2b08f20ce6a582587cf9d3e34775a43a4>
 [2015-08-08 11:06 UTC] rainer dot jung at kippdata dot de
Problem still exists for 7.0.0 Beta 3.

Linux x86_64 builds are fine (as expected).
 [2015-08-08 12:41 UTC] rainer dot jung at kippdata dot de
Concerning zend_strtod():

- I first tried the original library available under www.netlib.org/fp. It didn't show the problem.

- I then tried to shrink the delta between the PHP copy and the original and ended up with a test case, that doesn't show the problem even with PHP zend_strtod(). So it seems it is something in PHP before the calls to zend_strtod(), or the problem is in the output rendering of the float.

Here's my test:

1) Create a file dtoa-main.c containing:

#include <stdio.h>
#include <zend_strtod.h>

int main() {
    fprintf(stdout, "%f\n", zend_strtod("29000000", NULL));
    fprintf(stdout, "%f\n", zend_strtod("290000000", NULL));
    fprintf(stdout, "%f\n", zend_strtod("2900000000", NULL));
    fprintf(stdout, "%f\n", zend_strtod("29000000000", NULL));
    fprintf(stdout, "%f\n", zend_strtod("290000000000", NULL));
    fprintf(stdout, "%f\n", zend_strtod("2900000000000", NULL));
    fprintf(stdout, "%f\n", zend_strtod("29000000000000", NULL));
    fprintf(stdout, "%f\n", zend_strtod("290000000000000", NULL));
    fprintf(stdout, "%f\n", zend_strtod("2900000000000000", NULL));
}

The numbers are the same as in the test.php I originally use to produce the problem.

Now compile this snippet against the original zend_strtod.o object file:

solaris10.sparc apache% gcc -I /path/to/php/bldir/Zend -I /path/to/php/bldir/TSRM -I /path/to/php/bldir -Wl,-z -Wl,nodefs -o dtoa-test dtoa-main.c /path/to/php/bldir/Zend/.libs/zend_strtod.o

The "-z nodefs" linker argument is only needed, because the object file references the zend_error_noreturn symbol, which is not used in this test case. I didn't want to soak in many more object files.

Now run the binary:

./dtoa-test
29000000.000000
290000000.000000
2900000000.000000
29000000000.000000
290000000000.000000
2900000000000.000000
29000000000000.000000
290000000000000.000000
2900000000000000.000000

So the float seems to be OK.
 [2015-08-08 12:53 UTC] rainer dot jung at kippdata dot de
I added debug statements to zend_strtod(). The call to it gets the correct input string, and if I print the result before returning with fprintf(stderr, "%f") it is also correct. Only the var_dump() output float(...) is wrong. So it seems the problem is more about the formatting of float in var_dump().
 [2015-08-08 14:32 UTC] rainer dot jung at kippdata dot de
OK, another step forward, but I gues that's how far I get without any hints:

Simplest test script:

<?php
$var = 2900000000;
var_dump($var);
?>

Expected result:
----------------
float(2900000000)

Actual result:
--------------
float(2899998720)

It works for PHP 5.6, fails for 7.

- The result of zend_strtod() when printed with fprintf and %f format is "2900000000.000000". So this is OK.

- The double assignment in Zend/zend_language_scanner.c in line 2757 assigns the correct value (verified with fprintf and %f). If I call php_var_dump(zendlval, 0) there it writes the correct string "float(2900000000)".

- In the var_dump() call of next script line, the float argument is already wrong. If checked via fprointf and %f it is "2899998720.000000". The address of struc in php_var_dump() has also changed from the previous address of zendlval in zend_language_scanner.c.

I don't know where the change happens, I could just narrow it down so far.

Just to make sure: I don't think this is an expected float precision problem. The test works for 5.6 and the observed precision is 7 instead of 14. It makes a lot of standard test suite tests fail.
 [2015-08-08 20:53 UTC] rainer dot jung at kippdata dot de
OK, I used the debugger. The problem is inside ZVAL_COPY_VALUE_EX. Only half of the double is being copied, only 32 bits of the 64 bits. ZVAL_COPY_VALUE_EX is meant to fix this by using the "#if SIZEOF_SIZE_T == 4" definition of it it additionally copies value.ww.w2. But on Solaris Sparc, value.ww.w2 has already been copied and what is missing is value.ww.w1 ! For some reason the struct layout is not the expected one, w1 and w2 have the wrong order. It seems the ZEND_ENDIAN_LOHI() logic for w1, w2 resp. the refcount field which is used in the basic copying doesn't work on sparc. It is a big endian platform and WORDS_BIGENDIAN is defined, but is nevertheless doesn't work.

I hope that's enough debugging for you guys to be able to go the last mile. I can test on the target platform whatever change you want me to.
 [2015-08-08 20:57 UTC] rainer dot jung at kippdata dot de
-Summary: Precision problem in float +Summary: ZVAL_COPY_VALUE_EX broken for 32bit Solaris Sparc -PHP Version: 7.0.0beta2 +PHP Version: 7.0.0beta3
 [2015-08-08 20:57 UTC] rainer dot jung at kippdata dot de
Changed Summary, because I narrowed down the root cause.
Unfortunately there is no "Zend" in the issue tracker "Package" drop down.
 [2015-08-08 21:53 UTC] rainer dot jung at kippdata dot de
I extracted a C standalone reproduction case from the PHP files.

I will attach it.

The zval.value layout on this playform is such, that lval and dval both start at the lower address but dval extends above lval.

Now because of ZEND_ENDIAN_LOHI in the definition of ww, the address of w2 is the low one (same as lval and dval), and the address of w1 is the high one.

Copying in ZVAL_COPY_VALUE copies the 32 bit contents of the low address, and then additionally z->value.ww.w2 = _w2, which is again the low hald of dval.

So the high half stays uninitialized resp. 0.

So for this platform the use of ZEND_ENDIAN_LOHI doesn't work as expected.

I will attach a small C program to show the problem. It can be compiled standalone on Solaris Sparc (I used gcc), no dependencies needed. I will attach it as a patch, because I don't see any other way of attaching source files.
 [2015-08-09 04:04 UTC] rainer dot jung at kippdata dot de
I removed the ZEND_ENDIAN_LOHI in front of w1, w2 and now the test suite results are in line with the 5.6 ones.

I really don't see a reason, why the ZEND_ENDIAN_LOHI should be there. I always expect the struct layout to have lval and dval start at the lower address, as well as w1 when ZEND_ENDIAN_LOHI is removed. So for a 32 Bit build one then would always need to copy w2 in addition, which is what the code currently does. Whether w1 resp. w2 contain least significant bits or most significant bits soesn't matter, als long as bot are copied (and not used to interprete the two halves of the 64 bits individually).

If you really think you need the ZEND_ENDIAN_LOHI, then you also need to switch the additional copying of w2 to w1 instead for big endian systems.
 [2015-08-09 13:15 UTC] cmb@php.net
-Status: Open +Status: Analyzed -Package: *General Issues +Package: Scripting Engine problem
 [2015-08-09 13:15 UTC] cmb@php.net
Thanks, Rainer, for the thorough analysis. AIUI the portable data
layout (ZEND_ENDIAN_LOHI) allows for more efficient operations[1].

PR #1464 is supposed to solve the issue. I do not have a
big-endian machine at hand, so I can't test it, though.

[1] <https://github.com/php/php-src/commit/d8099d0468426dbee59f540048376653535270ce>
 [2015-08-09 14:23 UTC] rainer dot jung at kippdata dot de
Where do I find "PR #1464"? Is there a typo (the number is very small).
I can test any suggested change.
 [2015-08-09 14:26 UTC] rainer dot jung at kippdata dot de
Ah, OK, found it at Github. I will add the change to my build and restest. Building and testing will take a few hours. Thanks for the fast reaction!
 [2015-08-09 14:28 UTC] cmb@php.net
You can find it on <https://github.com/php/php-src/pull/1464>.
(It's also linked further above in this ticket in the "Pull
Requests" section.)
 [2015-08-09 18:10 UTC] rainer dot jung at kippdata dot de
PR tested. Build succeeds, running test suite shows good results including the new test. Looks good to me for merging. Thanks a bunch.
 [2015-08-10 15:31 UTC] laruence@php.net
Automatic comment on behalf of laruence
Revision: http://git.php.net/?p=php-src.git;a=commit;h=89940993f0f60415e539b05dbec2c24fa79bca81
Log: Fixed Bug #70173 (ZVAL_COPY_VALUE_EX broken for 32bit Solaris Sparc)
 [2015-08-10 15:31 UTC] laruence@php.net
-Status: Analyzed +Status: Closed
 [2015-08-10 15:31 UTC] laruence@php.net
Automatic comment on behalf of cmb
Revision: http://git.php.net/?p=php-src.git;a=commit;h=79f64e5e673e15c1512eebfd9ce2df719b4a3036
Log: Fix #70173: ZVAL_COPY_VALUE_EX broken for 32bit Solaris Sparc
 [2015-08-18 16:24 UTC] ab@php.net
Automatic comment on behalf of laruence
Revision: http://git.php.net/?p=php-src.git;a=commit;h=89940993f0f60415e539b05dbec2c24fa79bca81
Log: Fixed Bug #70173 (ZVAL_COPY_VALUE_EX broken for 32bit Solaris Sparc)
 [2015-08-18 16:24 UTC] ab@php.net
Automatic comment on behalf of cmb
Revision: http://git.php.net/?p=php-src.git;a=commit;h=79f64e5e673e15c1512eebfd9ce2df719b4a3036
Log: Fix #70173: ZVAL_COPY_VALUE_EX broken for 32bit Solaris Sparc
 [2016-07-20 11:37 UTC] davey@php.net
Automatic comment on behalf of laruence
Revision: http://git.php.net/?p=php-src.git;a=commit;h=89940993f0f60415e539b05dbec2c24fa79bca81
Log: Fixed Bug #70173 (ZVAL_COPY_VALUE_EX broken for 32bit Solaris Sparc)
 [2016-07-20 11:37 UTC] davey@php.net
Automatic comment on behalf of cmb
Revision: http://git.php.net/?p=php-src.git;a=commit;h=79f64e5e673e15c1512eebfd9ce2df719b4a3036
Log: Fix #70173: ZVAL_COPY_VALUE_EX broken for 32bit Solaris Sparc
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Sat Apr 29 07:01:45 2017 UTC