Skip to content

Fix GH-15657: Segmentation fault in ext/opcache/jit/ir/dynasm/dasm_x86.h#15819

Closed
ndossche wants to merge 1 commit intophp:masterfrom
ndossche:fix-15657
Closed

Fix GH-15657: Segmentation fault in ext/opcache/jit/ir/dynasm/dasm_x86.h#15819
ndossche wants to merge 1 commit intophp:masterfrom
ndossche:fix-15657

Conversation

@ndossche
Copy link
Member

@ndossche ndossche commented Sep 9, 2024 •
edited
Loading

The crash happens because the zend_persist.c code tries to JIT the hook's op_array while the JIT buffer memory is still protected. This happens in zend_persist_property_info called via zend_persist_class_entry through the inheritance cache. You can check that this is true by surrounding the JIT call with zend_jit_unprotect() and zend_jit_protect().

We shouldn't JIT the property hook code when persisting property info for the inheritance cache.

This is a simple workaround by temporarily disabling the JIT so that the property hook code is not JITted when persisting the property info.

An alternative solution would be to move the JITting of the property hooks to a different place in zend_persist.c by doing an additional pass over the classes.

@ndossche
Fix phpGH-15657: Segmentation fault in ext/opcache/jit/ir/dynasm/dasm…
f724216 
…_x86.h

The crash happens because the zend_persist.c code tries to JIT the hook's
op_array while the JIT buffer memory is still protected. This happens in
`zend_persist_property_info` called via `zend_persist_class_entry`
through the inheritance cache.

We shouldn't JIT the property hook code when persisting property info
for the inheritance cache.

This is a simple workaround by temporarily disabling the JIT so that the
property hook code is not JITted when persisting the property info.

An alternative solution would be to move the JITting of the property
hooks to a different place in zend_persist.c by doing an additional pass
over the classes.
@ndossche ndossche linked an issue Sep 9, 2024 that may be closed by this pull request
Segmentation fault in ext/opcache/jit/ir/dynasm/dasm_x86.h #15657
Closed
@ndossche
Copy link
Member Author

ndossche commented Sep 9, 2024 •
edited
Loading

Actually, maybe this is not complete, because if I add the following code to the test at the bottom, it crashes (both with and without this patch):

for ($i=0;$i<2;$i++)
    echo (new A)->prop;

I don't have time anymore today to analyse that crash though. It still happens even if I remove the inheritance for interface I, so maybe it's a slightly different bug.

dstogov
dstogov approved these changes Sep 10, 2024
View reviewed changes
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably right.

cc: @iluuu1994

@ndossche
Copy link
Member Author

ndossche commented Sep 10, 2024 •
edited
Loading

Actually, maybe this is not complete, because if I add the following code to the test at the bottom, it crashes (both with and without this patch):

for ($i=0;$i<2;$i++)
    echo (new A)->prop;

I don't have time anymore today to analyse that crash though. It still happens even if I remove the inheritance for interface I, so maybe it's a slightly different bug.

I analysed this and this is a different bug related to a cache slot optimization.
As far as I understand, this happens for when the cache slot satisfies the ZEND_IS_PROPERTY_HOOK_SIMPLE_GET condition. Then we set up a function call frame and re-enter the VM to execute the hook function:

php-src/Zend/zend_vm_def.h

Lines 2094 to 2126 in 7c2204c

} else if (EXPECTED(ZEND_IS_PROPERTY_HOOK_SIMPLE_GET(prop_offset))) {
zend_function *hook = prop_info->hooks[ZEND_PROPERTY_HOOK_GET];
ZEND_ASSERT(hook->type == ZEND_USER_FUNCTION);
ZEND_ASSERT(RUN_TIME_CACHE(&hook->op_array));
uint32_t call_info = ZEND_CALL_NESTED_FUNCTION | ZEND_CALL_HAS_THIS;
if (OP1_TYPE & IS_CV) {
GC_ADDREF(zobj);
}
if (OP1_TYPE & (IS_CV|IS_VAR|IS_TMP_VAR)) {
call_info |= ZEND_CALL_RELEASE_THIS;
}
zend_execute_data *call = zend_vm_stack_push_call_frame(call_info, hook, 0, zobj);
call->prev_execute_data = execute_data;
call->call = NULL;
call->return_value = EX_VAR(opline->result.var);
call->run_time_cache = RUN_TIME_CACHE(&hook->op_array);
execute_data = call;
EG(current_execute_data) = execute_data;
zend_init_cvs(0, hook->op_array.last_var EXECUTE_DATA_CC);
#if defined(ZEND_VM_IP_GLOBAL_REG) && ((ZEND_VM_KIND == ZEND_VM_KIND_CALL) || (ZEND_VM_KIND == ZEND_VM_KIND_HYBRID))
opline = hook->op_array.opcodes;
#else
EX(opline) = hook->op_array.opcodes;
#endif
LOAD_OPLINE_EX();
ZEND_OBSERVER_SAVE_OPLINE();
ZEND_OBSERVER_FCALL_BEGIN(execute_data);
ZEND_VM_ENTER_EX();
}

This seems incompatible with how the minimal JIT works, getting the property will be skipped.
Indeed if we get rid of ZEND_SET_PROPERTY_HOOK_SIMPLE_GET in zend_object_handlers.c or go to a higher optimization level the problem disappears. I'm not sure yet how to solve that.

@dstogov
Copy link
Member

dstogov commented Sep 11, 2024

I think you should commit the existent fix and open a new bug report.

@ndossche ndossche closed this in 3665ab0 Sep 11, 2024
@ndossche ndossche mentioned this pull request Sep 11, 2024
Closed
@ndossche
Copy link
Member Author

ndossche commented Sep 11, 2024

Merged and opened #15834

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dstogov dstogov dstogov approved these changes

@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

Assignees

No one assigned

Labels

Extension: opcache

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Segmentation fault in ext/opcache/jit/ir/dynasm/dasm_x86.h

2 participants

@ndossche @dstogov