fix segfault in `ZEND_BIND_STATIC` #12758

realFlowControl · 2023-11-23T15:46:09Z

In case a ZEND_BIND_STATIC is being executed, while the current chunk is full, the zend_array_dup() call will trigger a OOM in ZendMM which will crash, as the opline might be a dangling pointer.

I am currently working on a stable test for this, but it is reproducible with the following PHP code:

function &ref() {
    static $a = 5;
    return $a;
}

class Foo {
    public static int $i;
    public static string $s = "x";
}

var_dump(Foo::$i = "1");
var_dump(Foo::$s, Foo::$i);

$a = range(0, 20000);

gc_mem_caches();

for ($i = 0; $i < 10364; ++$i) {
	$a[$i] = "abcdefghijklmnopqrstuvwxyz" . ($i % 2);
}

var_dump(ref());

The 10364 in the for loop is the "magic" number to fill up all the bins in the current chunk (this might vary).
Now in the call to ref() PHP will execute the ZEND_BIND_STATIC opcode and choose the path via zend_array_dub() in https://github.com/php/php-src/blob/PHP-8.1/Zend/zend_vm_def.h#L8793 which will try to allocate a new chunk, failing todo so as the memory limit is reached and then crash while trying to follow the opline pointer in zend_get_executed_lineno() while trying to create the "Allowed memory size of ..."-Error message.

$ php -n -dmemory_limit=2M x.php 
int(1)
string(1) "x"
int(1)
Segmentation fault (core dumped)

$ gdb php core
...
Core was generated by `php -n -dmemory_limit=2M x.php'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  zend_get_executed_lineno () at /usr/local/src/php/Zend/zend_execute_API.c:654
654			return ex->opline->lineno;
(gdb) bt
#0  zend_get_executed_lineno () at /usr/local/src/php/Zend/zend_execute_API.c:654
#1  0x0000004000582b42 in zend_get_executed_lineno () at /usr/local/src/php/Zend/zend_execute_API.c:636
#2  0x000000400018a2a7 in get_filename_lineno (type=type@entry=1, filename=filename@entry=0x40028f4540, lineno=lineno@entry=0x40028f453c) at /usr/local/src/php/Zend/zend.c:1548
#3  0x000000400018ac1d in zend_error_noreturn (type=type@entry=1, format=format@entry=0x4000f12460 "Allowed memory size of %zu bytes exhausted (tried to allocate %zu bytes)")
    at /usr/local/src/php/Zend/zend.c:1623
#4  0x0000004000187f34 in zend_mm_safe_error (format=format@entry=0x4000f12460 "Allowed memory size of %zu bytes exhausted (tried to allocate %zu bytes)", limit=2097152, size=size@entry=4096, 
    heap=<optimized out>) at /usr/local/src/php/Zend/zend_alloc.c:383
#5  0x0000004000188004 in zend_mm_alloc_pages (heap=<optimized out>, pages_count=<optimized out>) at /usr/local/src/php/Zend/zend_alloc.c:1016
#6  0x0000004000567a62 in zend_mm_alloc_small_slow (heap=0x4006600040, bin_num=6) at /usr/local/src/php/Zend/zend_alloc.c:1254
#7  0x00000040005a46e2 in zend_array_dup (source=0x40066571c0) at /usr/local/src/php/Zend/zend_hash.c:2330
#8  0x00000040005ba71e in ZEND_BIND_STATIC_SPEC_CV_UNUSED_HANDLER () at /usr/local/src/php/Zend/zend_vm_execute.h:48686
#9  0x00000040005f5958 in execute_ex (ex=0x1) at /usr/local/src/php/Zend/zend_vm_execute.h:60095
#10 0x00000040005fd530 in zend_execute (op_array=0x400667e100, return_value=0x0) at /usr/local/src/php/Zend/zend_vm_execute.h:60408
#11 0x000000400059202b in zend_execute_scripts (type=type@entry=8, retval=retval@entry=0x0, file_count=file_count@entry=3) at /usr/local/src/php/Zend/zend.c:1827
#12 0x000000400052e056 in php_execute_script (primary_file=<optimized out>) at /usr/local/src/php/main/main.c:2542
#13 0x000000400066e012 in do_cli (argc=4, argv=0x40011258d0) at /usr/local/src/php/sapi/cli/php_cli.c:964
#14 0x00000040001a6225 in main (argc=4, argv=0x40011258d0) at /usr/local/src/php/sapi/cli/php_cli.c:1333

I am currently working on a test, to reproduce this in a stable manner and not using a magic number that might change.

In case a `ZEND_BIND_STATIC` is being executed, while the current chunk is full, the `zend_array_dup()` call will trigger a OOM in ZendMM which will crash, as the opline might be a dangling pointer.

realFlowControl · 2023-11-23T17:39:39Z

I added the missing test 🎉

nielsdos

Tests fail, and I have remarks and questions. VM change itself looks good and should not impact performance as it simply moves a SAVE_OPLINE we already did anyway.
cc @iluuu1994

ext/zend_test/test.c

iluuu1994 · 2023-11-23T20:39:53Z

This is once again similar to #12648. This change is harmless, because (as Niels pointed out) the opline is saved anyway. I wonder if an alternative approach for Levis PR might be to initialize EX(opline) in i_init_func_execute_data even when ZEND_VM_IP_GLOBAL_REG is used. This seems to be the root cause of the issue, if the first instruction of a function invocation fails, before any instruction has written to opline. I previously thought this was always the case, but apparently not.

php-src/Zend/zend_execute.c

Lines 3998 to 4002 in a4e80b0

    
           #if defined(ZEND_VM_IP_GLOBAL_REG) && ((ZEND_VM_KIND == ZEND_VM_KIND_CALL) || (ZEND_VM_KIND == ZEND_VM_KIND_HYBRID)) 
        
           	opline = op_array->opcodes; 
        
           #else 
        
           	EX(opline) = op_array->opcodes; 
        
           #endif

This might overall be cheaper than initializing all oplines that may OOM.

nielsdos · 2023-11-23T21:04:00Z

This might overall be cheaper than initializing all oplines that may OOM.

Perhaps yeah, although we do a lot of SAVE_OPLINE anyway because a lot of instructions can throw, in which case it wouldn't save anything I think (unless we can delay the SAVE_OPLINE then...). Of course, this is speculation and overhead should be measured.
I would only try such a thing on master though.

iluuu1994 · 2023-11-23T21:09:36Z

@nielsdos I agree, my comment was somewhat speculative and this should be measured. In theory, one handler execution per function that skips SAVE_OPLINE could offset saving the opline when beginning function execution. The benefit of saving opline in i_init_func_execute_data is that it would solve all cases without having to manually examine all handlers. This is really easy to miss.

Edit: Also, execute_data is definitely in CPU cache in i_init_func_execute_data. I'm not sure if that's the case for every affected handler. (Although I suppose for a write it might not actually matter?)

bwoebi · 2023-11-23T21:32:07Z

It was in i_init_func_execute_data in the past. It's been probably removed from there as it's technically not necessary and we want it in the opline handlers for being accurate.

I'm curious whether it would be instead possible to make IP and FP truly global across all of php-src and extensions? Avoiding the need to store it at all (short of switching frames) and universally accessible. This would obviously reduce some register availability, but I'd be interested in performance characteristics of that - and it would make things simpler as to not having to store IP at all. The only thing which would then need handling are extensions which execute callbacks in response to a C callback from an external library.

iluuu1994 · 2023-11-23T21:58:07Z

@bwoebi It would indeed be nice to avoid the whole SAVE_OPLINE/CHECK_EXCEPTION dance. I can think of two problems:

If the VM assumes that the global register is set but the extension is built without ZEND_VM_IP_GLOBAL_REG, it will incorrectly access EX(opline). Similar issue in the opposite case. This means there's one more way for extensions/core to be incompatible.
If a library calls into the extension code (e.g. through a callback), we have this issue:

https://gcc.gnu.org/onlinedocs/gcc/Global-Register-Variables.html

If the register is a call-saved register, call ABI is affected: the register will not be restored in function epilogue sequences after the variable has been assigned. Therefore, functions cannot safely return to callers that assume standard ABI.

External libraries that call into extension code might clobber the IP register. Similarly, the extension might modify the IP register and not restore the register for the external library (which it should according to ABI).

I'm not very familiar with this topic, maybe there are nice solutions to this.

…timize

bwoebi · 2023-11-24T10:21:24Z

This means there's one more way for extensions/core to be incompatible.

Yes, it would require support at the extension coupling point. Like instead of just NTS vs ZTS and DEBUG vs NDEBUG, we'd now also need REG / NOREG. But in practice whole platforms are using the the same. GCC and clang both support global registers

If the register is a call-saved register, call ABI is affected [...] Therefore, functions cannot safely return to callers that assume standard ABI

Yes, that means, if you wish to access IP / FP in callbacks from an external non-extension code, you need to manually store IP/FP into thread local state before the external function call, and backup the register contents in a var and restore it in callback start, then set it back to backup var. Macros could help with that.

But it's a relatively rare case that you need to access IP/FP from external callbacks.

…places as well

realFlowControl · 2023-11-24T11:51:09Z

@nielsdos / @iluuu1994 thanks for your feedback so far. I'll try and see if I can make reproducer for the other cases @morrisonlevi found in his PR and try to make other PR's to PHP 8.1 today

iluuu1994 · 2023-11-24T13:54:58Z

But it's a relatively rare case that you need to access IP/FP from external callbacks.

Actually, I don't think it is, because the original issue here is OOM. We often use emalloc for external libraries. This means essentially any call to such a library would require backing up the register. I'm not sure we can avoid this without dropping the line number in OOM messages.

iluuu1994 · 2023-11-24T17:51:26Z

@bwoebi Just FYI, I'm experimenting with this. I think the idea is sound, apart from OOM. I'll post a PoC tomorrow or so.

bwoebi · 2023-11-24T17:58:47Z

@iluuu1994 for the edge case of the fatal error, we could possibly handle it in zend_catch, i.e. after register restore, that we just compute the error (sprintf), store it and then handle afterwards, enriching with the now available registers.

bwoebi · 2023-11-24T18:02:02Z

And also, often. I think it's PCRE, xml and tidy. And that's it for php-src, which use custom allocators within external library code here.

bwoebi · 2023-11-25T00:03:56Z

Merged together with #12768.

Florian Engelhardt and others added 2 commits November 23, 2023 16:34

fix segfault in ZEND_BIND_STATIC

3056f4c

In case a `ZEND_BIND_STATIC` is being executed, while the current chunk is full, the `zend_array_dup()` call will trigger a OOM in ZendMM which will crash, as the opline might be a dangling pointer.

add missing test

0fcb4e4

devnexen requested a review from nielsdos November 23, 2023 17:48

nielsdos reviewed Nov 23, 2023

View reviewed changes

ext/zend_test/test.c Outdated Show resolved Hide resolved

ext/zend_test/test.c Outdated Show resolved Hide resolved

ext/zend_test/test.c Outdated Show resolved Hide resolved

ext/zend_test/test.c Show resolved Hide resolved

assert()ing seems easier than trying to make the compiler to not op…

0e40a22

…timize

realFlowControl force-pushed the fix-opline branch 4 times, most recently from ccf4148 to 5acd417 Compare November 24, 2023 10:16

moved from function call to INI setting, so we can use this in other …

2449f9e

…places as well

realFlowControl force-pushed the fix-opline branch from 5acd417 to 2449f9e Compare November 24, 2023 11:07

realFlowControl added 2 commits November 24, 2023 12:15

make assert() work no NDEBUG builds

c559eae

document magic number

516ea04

realFlowControl mentioned this pull request Nov 24, 2023

fix segfault in ZEND_FUNC_GET_ARGS #12768

Merged

bwoebi closed this Nov 25, 2023

realFlowControl deleted the fix-opline branch November 27, 2023 08:03

realFlowControl mentioned this pull request Nov 27, 2023

Support neighbouring extension in the ZendMM custom handlers hook in zend_test #12792

Open

iluuu1994 mentioned this pull request Dec 4, 2023

Universal global regs 3 iluuu1994/php-src#113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix segfault in `ZEND_BIND_STATIC` #12758

fix segfault in `ZEND_BIND_STATIC` #12758

realFlowControl commented Nov 23, 2023 •

edited

realFlowControl commented Nov 23, 2023

nielsdos left a comment

iluuu1994 commented Nov 23, 2023

nielsdos commented Nov 23, 2023

iluuu1994 commented Nov 23, 2023 •

edited

bwoebi commented Nov 23, 2023

iluuu1994 commented Nov 23, 2023

bwoebi commented Nov 24, 2023 •

edited

realFlowControl commented Nov 24, 2023

iluuu1994 commented Nov 24, 2023

iluuu1994 commented Nov 24, 2023

bwoebi commented Nov 24, 2023 •

edited

bwoebi commented Nov 24, 2023

bwoebi commented Nov 25, 2023

fix segfault in ZEND_BIND_STATIC #12758

fix segfault in ZEND_BIND_STATIC #12758

Conversation

realFlowControl commented Nov 23, 2023 • edited

realFlowControl commented Nov 23, 2023

nielsdos left a comment

Choose a reason for hiding this comment

iluuu1994 commented Nov 23, 2023

nielsdos commented Nov 23, 2023

iluuu1994 commented Nov 23, 2023 • edited

bwoebi commented Nov 23, 2023

iluuu1994 commented Nov 23, 2023

bwoebi commented Nov 24, 2023 • edited

realFlowControl commented Nov 24, 2023

iluuu1994 commented Nov 24, 2023

iluuu1994 commented Nov 24, 2023

bwoebi commented Nov 24, 2023 • edited

bwoebi commented Nov 24, 2023

bwoebi commented Nov 25, 2023

fix segfault in `ZEND_BIND_STATIC` #12758

fix segfault in `ZEND_BIND_STATIC` #12758

realFlowControl commented Nov 23, 2023 •

edited

iluuu1994 commented Nov 23, 2023 •

edited

bwoebi commented Nov 24, 2023 •

edited

bwoebi commented Nov 24, 2023 •

edited