Bug #76466 [Ana]: Loop variable confusion

Edit report at https://bugs.php.net/bug.php?id=76466&edit=1 ID: 76466 Updated by: laruence@php.net Reported by: cmb@php.net Summary: Loop variable confusion Status: Analyzed Type: Bug Package: opcache Operating System: Linux PHP Version: 7.3.0alpha1 Assigned To: laruence Block user comment: N Private report: N New Comment: @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 . Previous Comments: ------------------------------------------------------------------------ [2018-06-14 09:46:55] 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 09:26:54] 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-13 13:17:41] 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-13 10:38:20] cmb@php.net Related To: Bug #76446 ------------------------------------------------------------------------ [2018-06-13 10:36:10] 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) } } ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=76466&edit=1