Fix GH-15834: Segfault with hook "simple get" cache slot and minimal JIT #17909

nielsdos · 2025-02-23T21:37:07Z

The FETCH_OBJ_R VM handler has an optimization that directly enters into a hook if it is a simpler getter hook. This is not compatible with the minimal JIT because the minimal JIT will try to continue executing the opcodes after the FETCH_OBJ_R.
To solve this, we check whether the opcode is still the expected one after the execution of the VM handler. If it is not, we know that we are going to execute a simple hook. In that case, exit to the VM.

Normally, I wouldn't care too much about this issue, because it is in minimal JIT. However, someone had this cause a crash to them by accident: #17767

iluuu1994

Thank you Niels! It seems I have missed the original bug report. Dmitry should judge whether this is the correct fix.

iluuu1994 · 2025-02-24T09:33:21Z

ext/opcache/jit/zend_jit.c

+						goto jit_failure;
+					}
+
+					/* If a simple hook is called, exit to the VM. */


Technically this could be omitted for certain property fetches, i.e. the ones that are known not to be polymorphic & don't contain hooks. More concretely, private and final properties.

dstogov · 2025-02-24T12:28:33Z

ext/opcache/jit/zend_jit.c

+					ir_ref if_hook_enter = ir_IF(jit_CMP_IP(jit, IR_EQ, opline + 1));
+					ir_IF_FALSE(if_hook_enter);
+					if (GCC_GLOBAL_REGS) {
+						ir_TAILCALL(IR_VOID, ir_LOAD_A(jit_IP(jit)));
+					} else {
+						ir_RETURN(ir_CONST_I32(1)); /* ZEND_VM_ENTER */
+					}
+					ir_IF_TRUE(if_hook_enter);
+


Checking for a possible hook call after each property read - looks stupid...
Previously we had magic __get and __set, but we didn't optimize their calls through trampolines...
I understand, this is targeted for "-O1", but this also will affect any non-constant property access.
Please check how this affect the resulting code size on typical opcache.jit=1205 and 1254 (on Wordpress and/or Symfony Demo).

Shouldn't you also do the same for FETCH_OBJ_W and maybe others?

Tracing JIT should be also affected by this problem.

Previously we had magic __get and __set, but we didn't optimize their calls through trampolines...

Indeed, but this optimization makes get hooks as fast as getter functions. While the vast majority of public properties will not have hooks, and thus still improve performance, it was still important to us that hooks are not significantly slower than a getter function.

Shouldn't you also do the same for FETCH_OBJ_W and maybe others?

This optimization only exists for property read.

Checking for a possible hook call after each property read - looks stupid...

Maybe we can avoid this by creating a custom VM handler that is only used here, that omits the "simple get" optimization.

Please check how do this work with non-constant property reads in tracing JIT. (I mean $this->{$name} that leads to read property hook).
I guess, tracing JIT also may need to be fixed.

I won't have time anymore to check this today. I kinda missed that this codegen path may still be taken a lot of times.
For call VM we can improve the codegen to check the return value of the VM handler directly instead of comparing IP, that should make more compact machine code.
A custom VM specialization may help, the question would be if the trade-off seems reasonable.
I'll think a bit more about possible other solutions.

I rebased and I pushed a little optimization.

Unfortunately, I get an assertion failure on Symfony with jit=1205. This already happens without this patch. No influence yet on jit=1254 because I didn't yet change tracing JIT code (and didn't have time to do so yet). To reproduce, just run php bin/console cache:clear then php bin/console cache:warmup and then php -d opcache.jit=1205 public/index.php

For WordPress on jit=1205 I get an increase from 35164971 to 35166107 in JIT memory usage. An increase of 1136 bytes, or 1.000032305x increase. So quite a few checks added, although < 0.01% increase.

…al JIT The FETCH_OBJ_R VM handler has an optimization that directly enters into a hook if it is a simpler getter hook. This is not compatible with the minimal JIT because the minimal JIT will try to continue executing the opcodes after the FETCH_OBJ_R. To solve this, we check whether the opcode is still the expected one after the execution of the VM handler. If it is not, we know that we are going to execute a simple hook. In that case, exit to the VM.

dstogov · 2025-03-03T06:46:36Z

ext/opcache/jit/zend_jit.c

+						if (opline->op2_type != IS_CONST
+						 || Z_TYPE_P(RT_CONSTANT(opline, opline->op2)) != IS_STRING
+						 || Z_STRVAL_P(RT_CONSTANT(opline, opline->op2))[0] == '\0') {
+							break;
+						}


Why did you move this down?
I suppose, you relay on ce selected above in non-optimized path.
This leads to false positive build failures, because of possibly uninitialized ce. This should be fixed.

I'm not completely sure if this code is going to be right for properties overridden with hooked ones in children.
Let we generate the code for parent class context where ce->num_hooked_props is zero. Then we execute it in context of child that overrides the accessed property...@iluuu1994 please check this

Why did you move this down? I suppose, you relay on ce selected above in non-optimized path. This leads to false positive build failures, because of possibly uninitialized ce. This should be fixed.

Indees that is the reason for moving down. I can fix build failure.

I'm not completely sure if this code is going to be right for properties overridden with hooked ones in children. Let we generate the code for parent class context where ce->num_hooked_props is zero. Then we execute it in context of child that overrides the accessed property...@iluuu1994 please check this

I only skip the check for final classes without hooks, so this should not be a problem.

I only skip the check for final classes without hooks, so this should not be a problem.

Right. Then this shouldn't be a problem.

dstogov · 2025-03-03T06:59:26Z

did you check, if the same problem affects the tracing JIT?

nielsdos · 2025-03-03T07:19:05Z

did you check, if the same problem affects the tracing JIT?

I had little time this weekend and didn't have time to check this yet. I will try to check this evening as I'm leaving for work soon. I also did not yet have time to investigate the assertion failure I mentioned in #17909 (comment)

dstogov · 2025-03-03T07:27:02Z

I had little time this weekend and didn't have time to check this yet. I will try to check this evening as I'm leaving for work soon. I also did not yet have time to investigate the assertion failure I mentioned in #17909 (comment)

OK. Just let me know when this is ready for the final review. The code looks right and I don't have objections.

nielsdos · 2025-03-04T22:18:42Z

Please check how do this work with non-constant property reads in tracing JIT. (I mean $this->{$name} that leads to read property hook).

The bug does not affect this case. It is because for non-constant names cache_slot is NULL, so ZEND_SET_PROPERTY_HOOK_SIMPLE_GET(cache_slot) in zend_read_property will not do anything.
I confirmed this manually by printing out when the slow path / fast path is taken, and it always takes the slow path.
So this also means that we can skip the opline comparison for non-constant accesses.
I will modify the patch to take this into account.

did you check, if the same problem affects the tracing JIT?

When tracing JIT starts execution, it removes the ZEND_PROPERTY_HOOK_SIMPLE_GET_BIT flag that enables the fast path here:

php-src/ext/opcache/jit/zend_jit_vm_helpers.c

Lines 863 to 865 in cd65240

    
           if (IS_HOOKED_PROPERTY_OFFSET(prop_offset)) { 
        
           	CACHE_PTR_EX(cache_slot + 1, (void*)((uintptr_t)CACHED_PTR_EX(cache_slot + 1) & ~ZEND_PROPERTY_HOOK_SIMPLE_GET_BIT)); \ 
        
           }

Because the cache slot is not reset to NULLs, only the flag is cleared, the read_property handler will not set the flag again.

So to me it seems tracing JIT is not affected. I also tested this manually and it always takes the slow path.

I made new memory measurements with the latest patch, and with my IR fix applied so function JIT works on Symfony:

	before	PR without IS_CONST check	PR with IS_CONST check
Symfony (demo)	19,928,312	19,928,456	19,928,312
WordPress (demo install)	32,027,112	32,027,912	32,027,112

So with the new IS_CONST check added, there is no extra overhead for the Symfony demo or WordPress homepage.

Note though that in function JIT it may still generate the check, for example for a constant property name that's either not a string or starts with a '\0' byte.

nielsdos · 2025-03-05T08:00:59Z

@dstogov this is ready for review please

github-actions bot added the Extension: opcache label Feb 23, 2025

nielsdos linked an issue Feb 23, 2025 that may be closed by this pull request

Segfault with hook "simple get" cache slot and minimal JIT #15834

Open

nielsdos marked this pull request as ready for review February 23, 2025 23:02

nielsdos requested a review from dstogov as a code owner February 23, 2025 23:02

nielsdos requested a review from iluuu1994 February 23, 2025 23:03

iluuu1994 approved these changes Feb 24, 2025

View reviewed changes

dstogov reviewed Feb 24, 2025

View reviewed changes

nielsdos added 2 commits February 27, 2025 20:56

Simplify check

a590953

nielsdos force-pushed the fix-15834 branch from 3a97e8f to d66f9f5 Compare February 27, 2025 21:31

Small optimization

d66f9f5

dstogov reviewed Mar 3, 2025

View reviewed changes

Fix false positive build warning

065961b

nielsdos mentioned this pull request Mar 3, 2025

Fix php/php-src#17966: Symfony JIT 1205 assertion failure dstogov/ir#109

Open

Optimize

19f7441

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GH-15834: Segfault with hook "simple get" cache slot and minimal JIT #17909

Fix GH-15834: Segfault with hook "simple get" cache slot and minimal JIT #17909

nielsdos commented Feb 23, 2025 •

edited

Loading

iluuu1994 left a comment

iluuu1994 Feb 24, 2025

dstogov Feb 24, 2025

iluuu1994 Feb 24, 2025

dstogov Feb 24, 2025

nielsdos Feb 24, 2025 •

edited

Loading

nielsdos Feb 27, 2025 •

edited

Loading

dstogov Mar 3, 2025

nielsdos Mar 3, 2025 •

edited

Loading

dstogov Mar 3, 2025

dstogov commented Mar 3, 2025

nielsdos commented Mar 3, 2025

dstogov commented Mar 3, 2025

nielsdos commented Mar 4, 2025 •

edited

Loading

nielsdos commented Mar 5, 2025

Fix GH-15834: Segfault with hook "simple get" cache slot and minimal JIT #17909

Are you sure you want to change the base?

Fix GH-15834: Segfault with hook "simple get" cache slot and minimal JIT #17909

Conversation

nielsdos commented Feb 23, 2025 • edited Loading

iluuu1994 left a comment

Choose a reason for hiding this comment

iluuu1994 Feb 24, 2025

Choose a reason for hiding this comment

dstogov Feb 24, 2025

Choose a reason for hiding this comment

iluuu1994 Feb 24, 2025

Choose a reason for hiding this comment

dstogov Feb 24, 2025

Choose a reason for hiding this comment

nielsdos Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

nielsdos Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

dstogov Mar 3, 2025

Choose a reason for hiding this comment

nielsdos Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

dstogov Mar 3, 2025

Choose a reason for hiding this comment

dstogov commented Mar 3, 2025

nielsdos commented Mar 3, 2025

dstogov commented Mar 3, 2025

nielsdos commented Mar 4, 2025 • edited Loading

nielsdos commented Mar 5, 2025

nielsdos commented Feb 23, 2025 •

edited

Loading

nielsdos Feb 24, 2025 •

edited

Loading

nielsdos Feb 27, 2025 •

edited

Loading

nielsdos Mar 3, 2025 •

edited

Loading

nielsdos commented Mar 4, 2025 •

edited

Loading