php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

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: cmb@php.net Assigned: laruence (profile)
Status: Closed Package: opcache
PHP Version: 7.3.0alpha1 OS: Linux
Private report: No CVE-ID: None
View Developer Edit
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: cmb@php.net
New email:
PHP Version: OS:

 

 [2018-06-13 10:36 UTC] cmb@php.net 
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 10:38 UTC] cmb@php.net 
Related To: Bug #76446
 [2018-06-13 13:17 UTC] cmb@php.net
-PHP Version: master-Git-2018-06-13 (Git) +PHP Version: 7.3.0alpha1
 [2018-06-13 13:17 UTC] cmb@php.net 
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] laruence@php.net
-Status: Open +Status: Analyzed -Assigned To: +Assigned To: laruence
 [2018-06-14 09:26 UTC] laruence@php.net 
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] nikic@php.net 
@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] laruence@php.net 
@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] dmitry@php.net 
@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] nikic@php.net 
@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] dmitry@php.net 
Automatic comment on behalf of dmitry@zend.com
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] dmitry@php.net
-Status: Analyzed +Status: Closed
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved. 		Last updated: Sat Nov 23 07:01:29 2024 UTC