php / php-src

The PHP Interpreter
https://www.php.net
Other
37.95k stars 7.73k forks source link

Segmentation fault in Zend/zend_vm_execute.h #15658

Closed YuanchengJiang closed 6 days ago

YuanchengJiang commented 2 weeks ago

Description

The following code:

<?php
var_dump(match ($undefinedVariable) {
1, 2, 3, 4, 5 => 'foo',
default => 'bar',
});
?>

Resulted in this output:

Zend/zend_vm_execute.h:6015:2: runtime error: member access within misaligned address 0x7ff9c500501f for type 'zval' (aka 'struct _zval_struct'), which requires 8 byte alignment
0x7ff9c500501f: note: pointer points here
 00 01 00 00 00  00 a8 07 7c f9 7f 00 00  a0 50 00 c5 f9 7f 00 00  00 00 00 00 00 00 00 00  c0 93 06
             ^ 
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior Zend/zend_vm_execute.h:6015:2

To reproduce:

opcache.enable=1
opcache.enable_cli=1
opcache.jit_buffer_size=1024M
opcache.jit=0101

PHP Version

PHP 8.4.0-dev

Operating System

ubuntu 22.04

iluuu1994 commented 2 weeks ago

It seems setting the optimization level to 1 breaks many tests. E.g. php-dev run-tests.php -d opcache.enable_cli=1 -d opcache.jit=1201. I can also reproduce this on 8.2. /cc @dstogov

nielsdos commented 2 weeks ago

If I reduce it to this:

<?php
match (random_int(1, 2)) {
    1, 2 => 'foo',
};

Then the ZEND_MATCH opcode is never called. Instead, after the DO_ICALL and exception check, the assembly does a call to the ZEND_MATCH_ERROR handler and then tailcalls to the opline handler. So no wonder we get weird results if the wrong op is executed.

JIT$/run/media/niels/MoreData/php-src/y.php: ; after folding
{
    uintptr_t c_1 = 0;
    bool c_2 = 0;
    bool c_3 = 1;
    uintptr_t c_4 = func *0x60ced2f0fa3e(): int32_t;
    uintptr_t c_5 = func *0x60ced2f1f601(): int32_t;
    uintptr_t c_6 = func *0x60ced2ef526e(): int32_t;
    uintptr_t c_7 = 0x7d4f30600010;
    uintptr_t c_8 = 0x60ced470fc88;
    uintptr_t c_9 = func *0x60ced2f530bc(): int32_t;
    uintptr_t c_10 = func *0x60ced3056f6f(): int32_t;
    l_1 = START(l_15);
    l_2 = CALL(l_1, c_4);
    l_3 = CALL(l_2, c_5);
    l_4 = CALL(l_3, c_5);
    l_5 = CALL(l_4, c_6);
    uintptr_t d_6, l_6 = LOAD(l_5, c_8);
    l_7 = GUARD_NOT(l_6, d_6, c_7);
    l_8 = CALL(l_7, c_9);
    uintptr_t d_9, l_9 = RLOAD(l_8, 15);
    uintptr_t d_10, l_10 = LOAD(l_9, d_9);
    l_11 = TAILCALL(l_10, d_10);
    l_12 = UNREACHABLE(l_11);
    l_13 = BEGIN;
    l_14 = TAILCALL(l_13, c_10);
    l_15 = UNREACHABLE(l_14, null, l_12);
}
JIT$/run/media/niels/MoreData/php-src/y.php:
    subq $0x30, %rsp
    movabsq $0x60ced2f0fa3e, %rax
    callq *%rax
    movabsq $0x60ced2f1f601, %rax
    callq *%rax
    movabsq $0x60ced2f1f601, %rax
    callq *%rax
    movabsq $0x60ced2ef526e, %rax
    callq *%rax
    movabsq $EG(exception), %rax
    movq (%rax), %rax
    testq %rax, %rax
    jne JIT$$exception_handler
    movabsq $0x60ced2f530bc, %rax
    callq *%rax
    addq $0x30, %rsp
    jmpq *(%r15)

I do see that there is no special handling, but perhaps this is intended to be generated somewhere else? https://github.com/php/php-src/blob/558ccf73622a8c6993cb5d623fb7cccde9486c3d/ext/opcache/jit/zend_jit.c#L2542-L2543

iluuu1994 commented 2 weeks ago

@nielsdos From the documentation, this jit mode is intended to only generate code that calls the vm handlers. So, I believe that not generating match code is expected.

nielsdos commented 2 weeks ago

@iluuu1994 Yes, but I'm not seeing a call to the MATCH VM handler, only one to the MATCH_ERROR.

iluuu1994 commented 2 weeks ago

Ah, ok. Sorry.

dstogov commented 1 week ago

JIT doesn't properly implement support for MATCH instruction when opcache.jit=1 (function jit without optimization). This is a historical limitation (MATCH implementation didn't provide JIT support).

It would be good to fix this. 1) It's possible to call MATCH handler and then generate JIT code for N-way switch based on the value of opline register. 2) It's possible to call MATCH handler as a tail handler and pass the following control to VM interpreter.

nielsdos commented 1 week ago

Because function jit without optimization is probably not used a lot, I think fix number 2 is the most sensible when regarding the trade-off complexity vs worth.