Fix GH-20733: heap buffer overflow in optimizer#20736
Fix GH-20733: heap buffer overflow in optimizer#20736ndossche wants to merge 2 commits intophp:PHP-8.3from
Conversation
Some (conditional) jump instructions can be the last one in the op_array, because they can jump to themselves. In those cases `i + 1` in the CFG builder can point to outside the op_array because `i` is already the last opline. To solve this we need to check against the end and prevent setting the successor out of bounds.
| if (j + 1 < blocks_count) { | ||
| block->successors[1] = j + 1; | ||
| } else { | ||
| block->successors[1] = j; /* last instruction and its own target */ |
There was a problem hiding this comment.
You make a block with 2 equal successors.
Actually we should have block with single successor or we should introduce some kind of never-reachable or a fake successor.
| block->successors_count = 2; | ||
| block->successors[0] = block_map[OP_JMP_ADDR(opline, opline->op2) - op_array->opcodes]; | ||
| if (j + 1 < blocks_count) { | ||
| block->successors[1] = j + 1; |
There was a problem hiding this comment.
In case SCCP removes block(s) but doesn't change block terminating instructions, this successor assignment may lead to wrong CFG.
For example :
<?php
function test($a)
{
$arr1 = ['line-break-chars' => 'php://temp','line-length' => $undef];
$arr2 = [];
$and = $arr1 and $arr2;
if ($a) goto l;
while ($and) {
}
echo "1\n";
l:
echo "2\n";
}
echo "Done";There was a problem hiding this comment.
SCCP removes echo "1\n"; and "connects" while loop with echo "2\n";
dstogov
left a comment
dstogov
left a comment
There was a problem hiding this comment.
Wow!
I'm surprised SCCP doesn't change conditional branch instructions into unconditional.
In general SCCP should do this. This is definitely possible for JMPZ|JMPNZ, but probably not always possible for all other instructions.
I think this fix is not complete.
2 participants
In this example, the SCCP pass causes the JMPNZ instruction to be the last instruction in the op_array, and always jump to itself. In those cases
i + 1in the CFG builder can point to outside the op_array becauseiis already the last opline.To solve this we need to check against the end and prevent setting the successor out of bounds.
The proposed solution is not really pretty. Ideally SCCP can replace the JMPNZ opcode with a JMP opcode, but that's more risky to commit to a stable branch.