php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #76466 Loop variable confusion
Submitted: 2018-06-13 10:36 UTC Modified: 2018-06-15 16:53 UTC
Votes:1
Avg. Score:3.0 ± 0.0
Reproduced:1 of 1 (100.0%)
Same Version:1 (100.0%)
Same OS:0 (0.0%)
From: [email protected] Assigned: laruence (profile)
Status: Closed Package: opcache
PHP Version: 7.3.0alpha1 OS: Linux
Private report: No CVE-ID: None
 [2018-06-13 10:36 UTC] [email protected]
Description:
------------
Running the test script without Opcache works fine, but with
Opcache enabled it results in erroneous behavior (at least after a
few tries).

PHP-7.2 does not exhibit this bug.


Test script:
---------------
<?php  

function foo() {
    for ($i = 1; $i <= 2; $i++) {
        if ($i == 2) {
            $type = 'text';
        } else {
            $type = 'varchar';
        }
        $field_array[] = ['name' => "pal_field{$i}", 'type' => $type, 'length' => 255, 'unsigned' => 1];
    }
    var_dump($field_array);
}
foo();


Expected result:
----------------
array(2) {
  [0]=>
  array(4) {
    ["name"]=>
    string(10) "pal_field1"
    ["type"]=>
    string(7) "varchar"
    ["length"]=>
    int(255)
    ["unsigned"]=>
    int(1)
  }
  [1]=>
  array(4) {
    ["name"]=>
    string(10) "pal_field2"
    ["type"]=>
    string(4) "text"
    ["length"]=>
    int(255)
    ["unsigned"]=>
    int(1)
  }
}

Actual result:
--------------
array(2) {
  [0]=>
  array(4) {
    ["name"]=>
    string(10) "pal_field1"
    ["type"]=>
    string(7) "varchar"
    ["length"]=>
    int(255)
    ["unsigned"]=>
    int(1)
  }
  [1]=>
  array(4) {
    ["name"]=>
    string(10) "pal_field1"
    ["type"]=>
    string(7) "varchar"
    ["length"]=>
    int(255)
    ["unsigned"]=>
    int(1)
  }
}

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2018-06-13 13:17 UTC] [email protected]
-PHP Version: master-Git-2018-06-13 (Git) +PHP Version: 7.3.0alpha1
 [2018-06-13 13:17 UTC] [email protected]
Looks like the SCCP is too aggressive as of commit ce3dbb5[1].
For what it's worth, removing the if statement from the test
script gives expected results.

[1] <https://github.com/php/php-src/commit/ce3dbb53a7dcef314bba1d355f049ddb2252fad1>
 [2018-06-14 09:26 UTC] [email protected]
-Status: Open +Status: Analyzed -Assigned To: +Assigned To: laruence
 [2018-06-14 09:26 UTC] [email protected]
the problem is that we may visit one instruction multiply times, depends on new infos comes... let me try to explain:

  let say we have two ops 
  #1 INIT_ARRAY
  #2 ADD_ARRAY_ELEMENT

 when we first visit INIT_ARRAY, we have op1 is an const value,  then we set INIT_ARRAY’s result as a const array.

then we visist #2,  we get the result value from #1,  and set the #1 result to NULL, then make #2 result a const array.

but later,  INIT_ARRAY op1’s value become BOT… and #1 is added into work instr again… 

but this time, we visist #1,  we get INIT_ARRAY->Result.def is IS_NULL,  then we simply return, then we leave #2’s result as an incorrect value type…. (it should be BOT, but it’s array)

fix could be something like https://gist.github.com/laruence/4068c564e38e8e0f84148cc1b3f977cf
 [2018-06-14 09:46 UTC] [email protected]
@laruence: Yes, this is clearly wrong. Without the partial array propagation this would still be fine if we made sure to propagate a BOT op1/op2 before the check (which we didn't do either...), as we know that these are the only possible lowerings and both result in a BOT. However, with partial arrays this is no longer possible, so I'd say we should remove this (you'll also have to modify the if (result) code below) and instead add a check for the array size and return BOT if the array is larger than say 16 elements, to avoid quadratic blowup.
 [2018-06-14 14:47 UTC] [email protected]
@nikic probably you saw the old patch? the result parts are already changed :)

about the array size limit, yeah, it's good idea... as after this change the array numbers will increased visibly.

I've asked dmitry to also have a look.. let's see what's his opinion .
 [2018-06-15 09:59 UTC] [email protected]
@laruence @nikic

I propose another approach - make BOT all the chain of ADD_ARRAY_ELEMENT opcodes, when some variable changes.

Please review https://gist.github.com/dstogov/142223b0c42c49a523f72a814db8b4f0
 [2018-06-15 16:53 UTC] [email protected]
@dmitry: It should not be necessary to manually BOT out the chain. If you only set the current result to BOT, the value will be propagated.

Of course returning BOT here will prevent us from catching certain patterns, though probably they aren't particularly important to us.
 [2018-06-18 08:21 UTC] [email protected]
Automatic comment on behalf of [email protected]
Revision: http://git.php.net/?p=php-src.git;a=commit;h=84d7d4e1cc49e657068ddd6f2c57a93165ffb1a5
Log: Fixed bug #76466 (Loop variable confusion)
 [2018-06-18 08:21 UTC] [email protected]
-Status: Analyzed +Status: Closed
 
PHP Copyright © 2001-2026 The PHP Group
All rights reserved.
Last updated: Tue Mar 17 11:00:01 2026 UTC