Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fdcfc0a
Fix GH-15834: Segfault with hook "simple get" cache slot and minimal JIT
nielsdos Feb 23, 2025
a590953
Simplify check
nielsdos Feb 23, 2025
d66f9f5
Small optimization
nielsdos Feb 27, 2025
065961b
Fix false positive build warning
nielsdos Mar 3, 2025
19f7441
Optimize
nielsdos Mar 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 36 additions & 6 deletions ext/opcache/jit/zend_jit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1316,7 +1316,7 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
uint32_t target_label, target_label2;
uint32_t op1_info, op1_def_info, op2_info, res_info, res_use_info, op1_mem_info;
zend_jit_addr op1_addr, op1_def_addr, op2_addr, op2_def_addr, res_addr;
zend_class_entry *ce;
zend_class_entry *ce = NULL;
bool ce_is_instanceof;
bool on_this;

Expand Down Expand Up @@ -2335,11 +2335,6 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
case ZEND_FETCH_OBJ_R:
case ZEND_FETCH_OBJ_IS:
case ZEND_FETCH_OBJ_W:
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;
}
ce = NULL;
ce_is_instanceof = 0;
on_this = 0;
Expand Down Expand Up @@ -2369,6 +2364,11 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
}
}
}
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;
}
Comment on lines +2367 to +2371
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@nielsdos nielsdos Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

if (!zend_jit_fetch_obj(&ctx, opline, op_array, ssa, ssa_op,
op1_info, op1_addr, 0, ce, ce_is_instanceof, on_this, 0, 0, NULL,
RES_REG_ADDR(), IS_UNKNOWN,
Expand Down Expand Up @@ -2709,6 +2709,36 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
/* We skip over the DO_FCALL, so decrement call_level ourselves. */
call_level--;
}
break;
case ZEND_FETCH_OBJ_R:
if (!zend_jit_handler(&ctx, opline,
zend_may_throw(opline, ssa_op, op_array, ssa))) {
goto jit_failure;
}

/* Cache slot is only used for IS_CONST op2, so only that can result in hook fast path. */
if (opline->op2_type == IS_CONST) {
if (JIT_G(opt_level) < ZEND_JIT_LEVEL_INLINE) {
if (opline->op1_type == IS_UNUSED) {
ce = op_array->scope;
} else {
ce = NULL;
}
}

if (!ce || !(ce->ce_flags & ZEND_ACC_FINAL) || ce->num_hooked_props > 0) {
/* If a simple hook is called, exit to the VM. */
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);
}
}

break;
default:
if (!zend_jit_handler(&ctx, opline,
Expand Down
23 changes: 23 additions & 0 deletions ext/opcache/tests/jit/gh15834.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
--TEST--
GH-15834 (Segfault with hook "simple get" cache slot and minimal JIT)
--EXTENSIONS--
opcache
--INI--
opcache.jit=1111
--FILE--
<?php
class A {
public $_prop = 1;
public $prop {
get => $this->_prop;
}
}

$a = new A;
for ($i=0;$i<5;$i++) {
echo $a->prop;
$a->_prop++;
}
?>
--EXPECT--
12345
Loading