Skip to content

Fix zend_optimizer_replace_by_const() leaving stale OP2 uses behind - #23546

Open
Mrmaxmeier wants to merge 1 commit into
php:PHP-8.4from
Mrmaxmeier:fix-replace-by-const-op2
Open

Fix zend_optimizer_replace_by_const() leaving stale OP2 uses behind#23546
Mrmaxmeier wants to merge 1 commit into
php:PHP-8.4from
Mrmaxmeier:fix-replace-by-const-op2

Conversation

@Mrmaxmeier

Copy link
Copy Markdown
Contributor

Hi,

we ran into a failing SSA integrity check with the fuzzer-function-jit fuzzing target:

<?php
function test($v) {
    switch ($v) {
        case [$x] = (int)1.5:
            return "matched " . var_export($x, true);
        default:
            return "default";
    }
}

var_dump(test(1));
var_dump(test(2));
Assertion backtrace for reproducer
/out/php-fuzz-function-jit: Running 1 inputs 100 time(s) each.
Running: /testcase

In function ::test (before dfa):
var op2 of 3 (ZEND_IS_EQUAL) does not use/def an ssa var

test:
     ; (lines=7, args=1, vars=2, tmps=4, ssa_vars=6, no_loops)
     ; (at SSA integrity verification)
     ; /tmp/fuzzer.php:2-7
     ; return  [null] RANGE[0..0]
     ; #0.CV0($v) NOVAL [undef]
     ; #1.CV1($x) NOVAL [undef]
BB0:
     ; start lines=[0-4]
     ; to=(BB2, BB1)
     ; level=0
     ; children=(BB1, BB2)
0000 #2.CV0($v) [any] = RECV 1
0001 #3.T4 [null] = FETCH_LIST_R int(1) int(0)
0002 ASSIGN #1.CV1($x) NOVAL [undef] -> #4.CV1($x) NOVAL [null] #3.T4 [null]
0003 #5.T2 [bool] = IS_EQUAL #2.CV0($v) [any] T3
0004 JMPNZ #5.T2 [bool] BB2

BB1:
     ; follow exit lines=[5-5]
     ; from=(BB0)
     ; idom=BB0
     ; level=1
0005 RETURN null

BB2:
     ; target exit lines=[6-6]
     ; from=(BB0)
     ; idom=BB0
     ; level=1
0006 RETURN null
php-fuzz-function-jit: /src/php-src/Zend/Optimizer/ssa_integrity.c:419: void ssa_verify_integrity(zend_op_array *, zend_ssa *, const char *): Assertion `0 && "SSA integrity verification failed"' failed.
AddressSanitizer:DEADLYSIGNAL
=================================================================
==14==ERROR: AddressSanitizer: ABRT on unknown address 0x00000000000e (pc 0x7f53e68a200b bp 0x7f53e6a17588 sp 0x7fffe77bb9b0 T0)
SCARINESS: 10 (signal)
    #0 0x7f53e68a200b in raise (/lib/x86_64-linux-gnu/libc.so.6+0x4300b) (BuildId: 5792732f783158c66fb4f3756458ca24e46e827d)
    #1 0x7f53e6881858 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x22858) (BuildId: 5792732f783158c66fb4f3756458ca24e46e827d)
    #2 0x7f53e6881728  (/lib/x86_64-linux-gnu/libc.so.6+0x22728) (BuildId: 5792732f783158c66fb4f3756458ca24e46e827d)
    #3 0x7f53e6892fd5 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x33fd5) (BuildId: 5792732f783158c66fb4f3756458ca24e46e827d)
    #4 0x561f569f7398 in ssa_verify_integrity /src/php-src/Zend/Optimizer/ssa_integrity.c:419:3
    #5 0x561f569f90af in zend_dfa_optimize_op_array /src/php-src/Zend/Optimizer/dfa_pass.c:1075:3
    #6 0x561f56ac2d78 in zend_optimize_script /src/php-src/Zend/Optimizer/zend_optimizer.c:1658:5
    #7 0x561f56237b79 in cache_script_in_shared_memory /src/php-src/ext/opcache/ZendAccelerator.c:1598:2
    #8 0x561f56239b6d in persistent_compile_file /src/php-src/ext/opcache/ZendAccelerator.c:2399:24
    #9 0x561f5711538e in fuzzer_do_request_from_buffer /src/php-src/sapi/fuzzer/fuzzer-sapi.c:289:29
    #10 0x561f57113954 in LLVMFuzzerTestOneInput /src/php-src/sapi/fuzzer/fuzzer-function-jit.c:32:2
    [..]

DEDUP_TOKEN: raise--abort--
SUMMARY: AddressSanitizer: ABRT (/lib/x86_64-linux-gnu/libc.so.6+0x4300b) (BuildId: 5792732f783158c66fb4f3756458ca24e46e827d) in raise
==14==ABORTING

For opcodes that do not consume their OP1 operand (ZEND_FETCH_LIST_R and friends), the constant is propagated into every use of the temporary rather than just the first one, because the value stays alive past the first reader. That loop only ever looked at OP1 though, so a use of the same temporary as an OP2 operand was skipped and left referring to a temporary that no longer has a definition.

In the reproducer the ZEND_CAST is folded and rewritten into the ZEND_FETCH_LIST_R of the list assignment, but the comparison emitted for the case expression uses the same temporary as its OP2. That use keeps the dangling reference, and SSA construction then fails its integrity check.

The fix handles the OP2 case in the same loop. An OP2 operand is always consumed, so such a use is necessarily the last one and the loop can stop there. ZEND_FE_FETCH_R / ZEND_FE_FETCH_RW are the sole exception, where OP2 is a def rather than a use, so they are excluded.

Thanks!


Found by the CISPA Fandango team while triaging findings in oss-fuzz harnesses.

For opcodes that do not consume their OP1 operand, the constant is propagated
into every use of the temporary rather than just the first one. That loop only
ever looked at OP1, so a use of the same temporary as an OP2 operand was left
referring to a temporary that no longer has a definition:

    function test($v) {
        switch ($v) {
            case [$x] = (int)1.5:
                break;
        }
    }

The ZEND_CAST is folded and rewritten into the ZEND_FETCH_LIST_R of the list
assignment, but the comparison emitted for the case expression uses the same
temporary as its OP2 and keeps the dangling reference, which makes SSA
construction fail its integrity check.

An OP2 operand is always consumed (ZEND_FE_FETCH_R/RW, where OP2 is a def
rather than a use, is the sole exception), so such a use is the last one and
the loop can stop there.

Assisted-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant