From b666dc97887c8d5648ab435656d61ec41300dd63 Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Sat, 21 Dec 2024 19:05:18 +0100
Subject: [PATCH 1/4] Fix GH-15833: Segmentation fault (access null pointer) in
ext/spl/spl_array.c
We're accessing the object properties table directly in spl, but we're
not accounting for lazy objects. Upon accessing we should trigger the
initialization as spl is doing direct manipulations on the object
property table and expects a real object.
Closes GH-17235.
---
NEWS | 4 ++++
ext/spl/spl_array.c | 25 ++++++++++++++++++++++
ext/spl/tests/gh15833_1.phpt | 22 +++++++++++++++++++
ext/spl/tests/gh15833_2.phpt | 41 ++++++++++++++++++++++++++++++++++++
4 files changed, 92 insertions(+)
create mode 100644 ext/spl/tests/gh15833_1.phpt
create mode 100644 ext/spl/tests/gh15833_2.phpt
diff --git a/NEWS b/NEWS
index cc0824a952a..4e734a6e9c0 100644
--- a/NEWS
+++ b/NEWS
@@ -46,6 +46,10 @@ PHP NEWS
. Fixed bug GH-17330 (SNMP::setSecurity segfault on closed session).
(David Carlier)
+- SPL:
+ . Fixed bug GH-15833 (Segmentation fault (access null pointer) in
+ ext/spl/spl_array.c). (nielsdos)
+
02 Jan 2025, PHP 8.4.3
- BcMath:
diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c
index 0acbc986119..db7f4fff9bb 100644
--- a/ext/spl/spl_array.c
+++ b/ext/spl/spl_array.c
@@ -40,6 +40,7 @@ PHPAPI zend_class_entry *spl_ce_ArrayObject;
typedef struct _spl_array_object {
zval array;
+ HashTable *sentinel_array;
uint32_t ht_iter;
int ar_flags;
unsigned char nApplyCount;
@@ -74,6 +75,19 @@ static inline HashTable **spl_array_get_hash_table_ptr(spl_array_object* intern)
return &Z_ARRVAL(intern->array);
} else {
zend_object *obj = Z_OBJ(intern->array);
+ /* Since we're directly playing with the properties table, we shall initialize the lazy object directly.
+ * If we don't, it's possible to continue working with the wrong object in case we're using a proxy. */
+ if (UNEXPECTED(zend_lazy_object_must_init(obj))) {
+ obj = zend_lazy_object_init(obj);
+ if (UNEXPECTED(!obj)) {
+ if (!intern->sentinel_array) {
+ intern->sentinel_array = zend_new_array(0);
+ }
+ return &intern->sentinel_array;
+ }
+ }
+ /* should no longer be lazy */
+ ZEND_ASSERT(!zend_lazy_object_must_init(obj));
/* rebuild properties */
zend_std_get_properties_ex(obj);
if (GC_REFCOUNT(obj->properties) > 1) {
@@ -129,6 +143,10 @@ static void spl_array_object_free_storage(zend_object *object)
zend_hash_iterator_del(intern->ht_iter);
}
+ if (UNEXPECTED(intern->sentinel_array)) {
+ zend_array_release(intern->sentinel_array);
+ }
+
zend_object_std_dtor(&intern->std);
zval_ptr_dtor(&intern->array);
@@ -489,6 +507,9 @@ static void spl_array_write_dimension_ex(int check_inherited, zend_object *objec
uint32_t refcount = 0;
if (!offset || Z_TYPE_P(offset) == IS_NULL) {
ht = spl_array_get_hash_table(intern);
+ if (UNEXPECTED(ht == intern->sentinel_array)) {
+ return;
+ }
refcount = spl_array_set_refcount(intern->is_child, ht, 1);
zend_hash_next_index_insert(ht, value);
@@ -505,6 +526,10 @@ static void spl_array_write_dimension_ex(int check_inherited, zend_object *objec
}
ht = spl_array_get_hash_table(intern);
+ if (UNEXPECTED(ht == intern->sentinel_array)) {
+ spl_hash_key_release(&key);
+ return;
+ }
refcount = spl_array_set_refcount(intern->is_child, ht, 1);
if (key.key) {
zend_hash_update_ind(ht, key.key, value);
diff --git a/ext/spl/tests/gh15833_1.phpt b/ext/spl/tests/gh15833_1.phpt
new file mode 100644
index 00000000000..d658a770e80
--- /dev/null
+++ b/ext/spl/tests/gh15833_1.phpt
@@ -0,0 +1,22 @@
+--TEST--
+GH-15833 (Segmentation fault (access null pointer) in ext/spl/spl_array.c)
+--CREDITS--
+YuanchengJiang
+--FILE--
+newLazyProxy(function ($obj) {
+ $obj = new C();
+ return $obj;
+});
+$recursiveArrayIterator = new RecursiveArrayIterator($obj);
+var_dump($recursiveArrayIterator->current());
+$recursiveArrayIterator->next();
+var_dump($recursiveArrayIterator->current());
+?>
+--EXPECT--
+int(1)
+NULL
diff --git a/ext/spl/tests/gh15833_2.phpt b/ext/spl/tests/gh15833_2.phpt
new file mode 100644
index 00000000000..8f30921741f
--- /dev/null
+++ b/ext/spl/tests/gh15833_2.phpt
@@ -0,0 +1,41 @@
+--TEST--
+GH-15833 (Segmentation fault (access null pointer) in ext/spl/spl_array.c)
+--CREDITS--
+YuanchengJiang
+--FILE--
+newLazyProxy(function ($obj) {
+ static $counter = 0;
+ throw new Error('nope ' . ($counter++));
+});
+$recursiveArrayIterator = new RecursiveArrayIterator($obj);
+try {
+ var_dump($recursiveArrayIterator->current());
+} catch (Error $e) {
+ echo $e->getMessage(), "\n";
+}
+try {
+ var_dump($recursiveArrayIterator->current());
+} catch (Error $e) {
+ echo $e->getMessage(), "\n";
+}
+try {
+ $recursiveArrayIterator->next();
+} catch (Error $e) {
+ echo $e->getMessage(), "\n";
+}
+try {
+ var_dump($recursiveArrayIterator->current());
+} catch (Error $e) {
+ echo $e->getMessage(), "\n";
+}
+?>
+--EXPECT--
+nope 0
+nope 1
+nope 2
+nope 3
From 72184abd2f47e6768bb7741647472acc00f8fa3c Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Thu, 2 Jan 2025 23:48:47 +0100
Subject: [PATCH 2/4] Fix GH-15981: Segfault with frameless jumps and minimal
JIT
Minimal JIT shouldn't generate a call to the complex handler, but
instead rely on the VM and then check for a two-way jump.
This moves the frameless codegen under the check `JIT_G(opt_level) >=
ZEND_JIT_LEVEL_INLINE`.
---
NEWS | 4 ++++
ext/opcache/jit/zend_jit.c | 11 ++++++-----
ext/opcache/tests/jit/gh15981.phpt | 24 ++++++++++++++++++++++++
3 files changed, 34 insertions(+), 5 deletions(-)
create mode 100644 ext/opcache/tests/jit/gh15981.phpt
diff --git a/NEWS b/NEWS
index 4e734a6e9c0..6c54aa4c998 100644
--- a/NEWS
+++ b/NEWS
@@ -36,6 +36,10 @@ PHP NEWS
- Intl:
. Fixed bug GH-11874 (intl causing segfault in docker images). (nielsdos)
+- Opcache:
+ . Fixed bug GH-15981 (Segfault with frameless jumps and minimal JIT).
+ (nielsdos)
+
- PHPDBG:
. Fix crashes in function registration + test. (nielsdos, Girgias)
diff --git a/ext/opcache/jit/zend_jit.c b/ext/opcache/jit/zend_jit.c
index 21870089e48..3c0bb0ee555 100644
--- a/ext/opcache/jit/zend_jit.c
+++ b/ext/opcache/jit/zend_jit.c
@@ -2489,6 +2489,11 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
goto jit_failure;
}
goto done;
+ case ZEND_JMP_FRAMELESS:
+ if (!zend_jit_jmp_frameless(&ctx, opline, /* exit_addr */ NULL, /* guard */ 0)) {
+ goto jit_failure;
+ }
+ goto done;
case ZEND_INIT_METHOD_CALL:
if (opline->op2_type != IS_CONST
|| Z_TYPE_P(RT_CONSTANT(opline, opline->op2)) != IS_STRING) {
@@ -2644,17 +2649,13 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
case ZEND_FE_FETCH_R:
case ZEND_FE_FETCH_RW:
case ZEND_BIND_INIT_STATIC_OR_JMP:
+ case ZEND_JMP_FRAMELESS:
if (!zend_jit_handler(&ctx, opline,
zend_may_throw(opline, ssa_op, op_array, ssa)) ||
!zend_jit_cond_jmp(&ctx, opline + 1, ssa->cfg.blocks[b].successors[0])) {
goto jit_failure;
}
break;
- case ZEND_JMP_FRAMELESS:
- if (!zend_jit_jmp_frameless(&ctx, opline, /* exit_addr */ NULL, /* guard */ 0)) {
- goto jit_failure;
- }
- break;
case ZEND_NEW:
if (!zend_jit_handler(&ctx, opline, 1)) {
return 0;
diff --git a/ext/opcache/tests/jit/gh15981.phpt b/ext/opcache/tests/jit/gh15981.phpt
new file mode 100644
index 00000000000..823b6122dc1
--- /dev/null
+++ b/ext/opcache/tests/jit/gh15981.phpt
@@ -0,0 +1,24 @@
+--TEST--
+GH-15981 (Segfault with frameless jumps and minimal JIT)
+--EXTENSIONS--
+opcache
+--INI--
+opcache.jit=1111
+--FILE--
+
+--EXPECTF--
+string(%d) "%s"
From c790c5b2e722c271e50e2aa89b5faacd6e0bf619 Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Mon, 6 Jan 2025 20:52:30 +0100
Subject: [PATCH 3/4] Generate inline frameless icall handlers only if the
optimization level is set to inline
---
ext/opcache/jit/zend_jit.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/ext/opcache/jit/zend_jit.c b/ext/opcache/jit/zend_jit.c
index 3c0bb0ee555..83837bb3454 100644
--- a/ext/opcache/jit/zend_jit.c
+++ b/ext/opcache/jit/zend_jit.c
@@ -2547,6 +2547,23 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
goto jit_failure;
}
goto done;
+ case ZEND_FRAMELESS_ICALL_0:
+ jit_frameless_icall0(jit, opline);
+ goto done;
+ case ZEND_FRAMELESS_ICALL_1:
+ op1_info = OP1_INFO();
+ jit_frameless_icall1(jit, opline, op1_info);
+ goto done;
+ case ZEND_FRAMELESS_ICALL_2:
+ op1_info = OP1_INFO();
+ op2_info = OP2_INFO();
+ jit_frameless_icall2(jit, opline, op1_info, op2_info);
+ goto done;
+ case ZEND_FRAMELESS_ICALL_3:
+ op1_info = OP1_INFO();
+ op2_info = OP2_INFO();
+ jit_frameless_icall3(jit, opline, op1_info, op2_info, OP1_DATA_INFO());
+ goto done;
default:
break;
}
@@ -2693,23 +2710,6 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
call_level--;
}
break;
- case ZEND_FRAMELESS_ICALL_0:
- jit_frameless_icall0(jit, opline);
- goto done;
- case ZEND_FRAMELESS_ICALL_1:
- op1_info = OP1_INFO();
- jit_frameless_icall1(jit, opline, op1_info);
- goto done;
- case ZEND_FRAMELESS_ICALL_2:
- op1_info = OP1_INFO();
- op2_info = OP2_INFO();
- jit_frameless_icall2(jit, opline, op1_info, op2_info);
- goto done;
- case ZEND_FRAMELESS_ICALL_3:
- op1_info = OP1_INFO();
- op2_info = OP2_INFO();
- jit_frameless_icall3(jit, opline, op1_info, op2_info, OP1_DATA_INFO());
- goto done;
default:
if (!zend_jit_handler(&ctx, opline,
zend_may_throw(opline, ssa_op, op_array, ssa))) {
From 28b448ac200ccbeafdb6927f263d08b22e643711 Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Wed, 1 Jan 2025 22:00:21 +0100
Subject: [PATCH 4/4] Fix GH-17307: Internal closure causes JIT failure
`bcadd(...)` is a closure for an internal function, and
`zend_jit_push_call_frame` takes into account both last_var and the
difference in argument numbers not only for user code but also for
internal code. However, this is inconsistent with
`zend_vm_calc_used_stack`, causing argument corruption.
Making this consistent fixes the issue.
I could only reproduce the assertion failure when using Valgrind.
Closes GH-17319.
---
NEWS | 1 +
ext/opcache/jit/zend_jit_ir.c | 22 ++++++++------------
ext/opcache/tests/jit/gh17307.phpt | 32 ++++++++++++++++++++++++++++++
3 files changed, 41 insertions(+), 14 deletions(-)
create mode 100644 ext/opcache/tests/jit/gh17307.phpt
diff --git a/NEWS b/NEWS
index 6c54aa4c998..f73cbf7b5c3 100644
--- a/NEWS
+++ b/NEWS
@@ -39,6 +39,7 @@ PHP NEWS
- Opcache:
. Fixed bug GH-15981 (Segfault with frameless jumps and minimal JIT).
(nielsdos)
+ . Fixed bug GH-17307 (Internal closure causes JIT failure). (nielsdos)
- PHPDBG:
. Fix crashes in function registration + test. (nielsdos, Girgias)
diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c
index e14345215f6..0aea4eab2bf 100644
--- a/ext/opcache/jit/zend_jit_ir.c
+++ b/ext/opcache/jit/zend_jit_ir.c
@@ -8503,18 +8503,16 @@ static int zend_jit_push_call_frame(zend_jit_ctx *jit, const zend_op *opline, co
} else {
ir_ref num_args_ref;
ir_ref if_internal_func = IR_UNUSED;
+ const size_t func_type_offset = is_closure ? offsetof(zend_closure, func.type) : offsetof(zend_function, type);
used_stack = (ZEND_CALL_FRAME_SLOT + opline->extended_value + ZEND_OBSERVER_ENABLED) * sizeof(zval);
used_stack_ref = ir_CONST_ADDR(used_stack);
+ used_stack_ref = ir_HARD_COPY_A(used_stack_ref); /* load constant once */
- if (!is_closure) {
- used_stack_ref = ir_HARD_COPY_A(used_stack_ref); /* load constant once */
-
- // JIT: if (EXPECTED(ZEND_USER_CODE(func->type))) {
- ir_ref tmp = ir_LOAD_U8(ir_ADD_OFFSET(func_ref, offsetof(zend_function, type)));
- if_internal_func = ir_IF(ir_AND_U8(tmp, ir_CONST_U8(1)));
- ir_IF_FALSE(if_internal_func);
- }
+ // JIT: if (EXPECTED(ZEND_USER_CODE(func->type))) {
+ ir_ref tmp = ir_LOAD_U8(ir_ADD_OFFSET(func_ref, func_type_offset));
+ if_internal_func = ir_IF(ir_AND_U8(tmp, ir_CONST_U8(1)));
+ ir_IF_FALSE(if_internal_func);
// JIT: used_stack += (func->op_array.last_var + func->op_array.T - MIN(func->op_array.num_args, num_args)) * sizeof(zval);
num_args_ref = ir_CONST_U32(opline->extended_value);
@@ -8541,12 +8539,8 @@ static int zend_jit_push_call_frame(zend_jit_ctx *jit, const zend_op *opline, co
}
ref = ir_SUB_A(used_stack_ref, ref);
- if (is_closure) {
- used_stack_ref = ref;
- } else {
- ir_MERGE_WITH_EMPTY_TRUE(if_internal_func);
- used_stack_ref = ir_PHI_2(IR_ADDR, ref, used_stack_ref);
- }
+ ir_MERGE_WITH_EMPTY_TRUE(if_internal_func);
+ used_stack_ref = ir_PHI_2(IR_ADDR, ref, used_stack_ref);
}
zend_jit_start_reuse_ip(jit);
diff --git a/ext/opcache/tests/jit/gh17307.phpt b/ext/opcache/tests/jit/gh17307.phpt
new file mode 100644
index 00000000000..292d695963c
--- /dev/null
+++ b/ext/opcache/tests/jit/gh17307.phpt
@@ -0,0 +1,32 @@
+--TEST--
+GH-17307 (Internal closure causes JIT failure)
+--EXTENSIONS--
+opcache
+simplexml
+bcmath
+--INI--
+opcache.jit=1254
+opcache.jit_hot_func=1
+opcache.jit_buffer_size=32M
+--FILE--
+");
+
+function run_loop($firstTerms, $closure) {
+ foreach ($firstTerms as $firstTerm) {
+ \debug_zval_dump($firstTerm);
+ $closure($firstTerm, "10");
+ }
+}
+
+run_loop($simple, bcadd(...));
+echo "Done\n";
+
+?>
+--EXPECTF--
+object(SimpleXMLElement)#%d (0) refcount(3){
+}
+object(SimpleXMLElement)#%d (0) refcount(3){
+}
+Done