php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #77742 bcpow() implementation related to gcc compiler optimization
Submitted: 2019-03-14 16:10 UTC Modified: 2019-03-14 16:24 UTC
From: samding at ca dot ibm dot com Assigned:
Status: Closed Package: Unknown/Other Function
PHP Version: 7.3.2 OS: Linux
Private report: No CVE-ID: None
 [2019-03-14 16:10 UTC] samding at ca dot ibm dot com
Description:
------------
When testing case "ext/bcmath/tests/bcpow_error2.phpt", it is found that 
gcc compiler optimization ("-O2") skipped two statements in following implementation code on s390x:

46 long
 47 bc_num2long (num)
 48      bc_num num;
 49 {
 50   long val;
 51   char *nptr;
 52   int  index;
 53
 54   /* Extract the int value, ignore the fraction. */
 55   val = 0;
 56   nptr = num->n_value;
 57   for (index=num->n_len; (index>0) && (val<=(LONG_MAX/BASE)); index--)
 58     val = val*BASE + *nptr++;
 59
 60   /* Check for overflow.  If overflow, return zero. */
 61   if (index>0) val = 0;                          // when -O2 is used, these 2 lines (61 & 62) are skipped and causes  a wrong value returned.
 62   if (val < 0) val = 0;
....

Here is the comment from gcc compiler developer:

"this code appears to rely on a strictly defined signed overflow. But that's not the case with C/C++. By default GCC assumes that a signed overflow will never happen. So it will optimize away the val < 0 check if it is possible to prove that val will never be subtracted from. 

I think the reason why it hits us only on S390x is that we by default have an unsigned char. So on S/390 the loop constantly adds a positive value to val so GCC deduces that val can only be less than 0 if there was an overflow. 

But in the end the code is not correct I think. It relies on undefined behavior and therefore should be fixed. Perhaps val could be turned into an unsigned long where an overflow is well-defined?   
" 

A possible solution is to define "val" as "unsigned long" and 
change the line 62 as " if (val >=0x8000000000000000 ) val =0;", which does not
depend on the overflow but can detect if val >= 2**63

Test script:
---------------
sapi/cli/php run-tests.php -P ext/bcmath/tests/bcpow_error2.phpt

Expected result:
----------------
val == 0;

Actual result:
--------------
val != 0

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2019-03-14 16:19 UTC] nikic@php.net
-Status: Open +Status: Verified
 [2019-03-14 16:19 UTC] nikic@php.net
Indeed, this is clearly UB and should be fixed, preferably by avoiding overflow in the first place. It already checks that val*BASE doesn't overflow, might as well check that the subsequent addition doesn't,
 [2019-03-14 16:24 UTC] samding at ca dot ibm dot com
-PHP Version: 7.3.3 +PHP Version: 7.3.2
 [2019-03-14 16:24 UTC] samding at ca dot ibm dot com
PHP version 7.3.2
 [2019-03-14 16:27 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=e7d40afb7a7984174eb132a14b7a6621c8e76258
Log: Fixed bug #77742
 [2019-03-14 16:27 UTC] nikic@php.net
-Status: Verified +Status: Closed
 
PHP Copyright © 2001-2019 The PHP Group
All rights reserved.
Last updated: Fri Sep 20 22:01:27 2019 UTC