Fix GH-17577: JIT packed type guard crash #17584

nielsdos · 2025-01-26T12:35:13Z

When a guard check is created for a variable to check if it's a packed array, it is possible that there was no prior type check for that variable. This happens in the global scope for example when the variable aliases. In the test, this causes a dereference of address 8 because the integer element in $a is interpreted as an array address and zend_array.u.flags is at offset 8.

This patch adds a check to see if the guard is handled.

If we were not able to determine or guard the type is array, then we also cannot know the array is packed.
An alternative I thought about was emitting a type check in this branch, but that can cause incorrect guard motion in case of aliasing. In such case we also shouldn't clear the MAY_BE_PACKED_GUARD like it does now.

When a guard check is created for a variable to check if it's a packed array, it is possible that there was no prior type check for that variable. This happens in the global scope for example when the variable aliases. In the test, this causes a dereference of address 8 because the integer element in `$a` is interpreted as an array address. This patch adds a check to see if the guard is handled. If we were not able to determine or guard the type then we also cannot know the array is packed.

dstogov

The patch fixes the problem, but I think it might be better not to add MAY_BE_PACKED_GUARD in the first place. This may be done by checking op1_info & (MAY_BE_ANY|MAY_BE_UNDEF) == MY_BE_ARRAY.

What do you think? may be I miss something.

nielsdos · 2025-01-27T20:55:34Z

The particular code responsible for setting MAY_BE_PACKED_GUARD is in the ZEND_FETCH_DIM_R case. Of course this is not the only place that would need modification, but just to show:

diff --git a/ext/opcache/jit/zend_jit_trace.c b/ext/opcache/jit/zend_jit_trace.c
index c138a5bcb0a..8fd0d1323d0 100644
--- a/ext/opcache/jit/zend_jit_trace.c
+++ b/ext/opcache/jit/zend_jit_trace.c
@@ -1935,7 +1935,8 @@ static zend_ssa *zend_jit_trace_build_tssa(zend_jit_trace_rec *trace_buffer, uin
 
 						zend_ssa_var_info *info = &tssa->var_info[tssa->ops[idx].op1_use];
 
-						if (MAY_BE_PACKED(info->type) && MAY_BE_HASH(info->type)) {
+						if (MAY_BE_PACKED(info->type) && MAY_BE_HASH(info->type)
+						 && (info->type & (MAY_BE_ANY|MAY_BE_UNDEF)) == MAY_BE_ARRAY) {
 							info->type |= MAY_BE_PACKED_GUARD;
 							if (orig_op1_type & IS_TRACE_PACKED) {
 								info->type &= ~(MAY_BE_ARRAY_NUMERIC_HASH|MAY_BE_ARRAY_STRING_HASH);
@@ -4162,7 +4163,6 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par
 			}
 
 			if ((info & MAY_BE_PACKED_GUARD) != 0
-			 && (info & MAY_BE_GUARD) == 0
 			 && (trace_buffer->stop == ZEND_JIT_TRACE_STOP_LOOP
 			  || trace_buffer->stop == ZEND_JIT_TRACE_STOP_RECURSIVE_CALL
 			  || trace_buffer->stop == ZEND_JIT_TRACE_STOP_RECURSIVE_RET)

If I do this, and keep the assertion for the stack type to be IS_ARRAY, then the test added in this PR succeeds but bug80447.phpt gets an assertion failure.
This happens because #0.CV0($treeNode) has stack type IS_UNKNOWN because it has a MAY_BE_GUARD flag that cannot be motioned, so the stack type does not become IS_ARRAY. So I believe in this case we could still have an issue because we didn't ensure the variable is of type array.

dstogov · 2025-01-30T23:48:16Z

If I do this, and keep the assertion for the stack type to be IS_ARRAY, then the test added in this PR succeeds but bug80447.phpt gets an assertion failure.
This happens because #0.CV0($treeNode) has stack type IS_UNKNOWN because it has a MAY_BE_GUARD flag that cannot be motioned, so the stack type does not become IS_ARRAY. So I believe in this case we could still have an issue because we didn't ensure the variable is of type array.

Actually, the check for PACKED_GUARD is useless in bug80447.phpt.
The code must be changed consistently with type GUARD.

                        if ((info & MAY_BE_PACKED_GUARD) != 0
                         && (trace_buffer->stop == ZEND_JIT_TRACE_STOP_LOOP
                          || trace_buffer->stop == ZEND_JIT_TRACE_STOP_RECURSIVE_CALL
-                         || trace_buffer->stop == ZEND_JIT_TRACE_STOP_RECURSIVE_RET)
+                         || (trace_buffer->stop == ZEND_JIT_TRACE_STOP_RECURSIVE_RET)
+                          && EX_VAR_TO_NUM((opline-1)->result.var) == i))
                         && (ssa->vars[i].use_chain != -1
                          || (ssa->vars[i].phi_use_chain
                           && !(ssa->var_info[ssa->vars[i].phi_use_chain->ssa_var].type & MAY_BE_PACKED_GUARD)))) {

dstogov

Looks fine, except the IS_VAR condition.
I would prefer to keep the conditions for type guard and packed guard the same.

dstogov · 2025-02-03T08:26:53Z

ext/opcache/jit/zend_jit_trace.c

@@ -4164,10 +4169,14 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par
 			if ((info & MAY_BE_PACKED_GUARD) != 0
 			 && (trace_buffer->stop == ZEND_JIT_TRACE_STOP_LOOP
 			  || trace_buffer->stop == ZEND_JIT_TRACE_STOP_RECURSIVE_CALL
-			  || trace_buffer->stop == ZEND_JIT_TRACE_STOP_RECURSIVE_RET)
+			  || (trace_buffer->stop == ZEND_JIT_TRACE_STOP_RECURSIVE_RET
+			   && (opline-1)->result_type == IS_VAR


It's better to keep the conditions exactly the same. We don't have IS_VAR check above (when we generate type guard). In general, Optimizer may fuse DO_FCALL with the following ASSIGN to IS_CV (I'm not sure if this is enabled or not).

Maybe I miss something, but I got this from here:

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

Lines 4148 to 4150 in a99025c

|| (trace_buffer->stop == ZEND_JIT_TRACE_STOP_RECURSIVE_RET

&& (opline-1)->result_type == IS_VAR

&& EX_VAR_TO_NUM((opline-1)->result.var) == i))

So I'm not sure I understand why the check for IS_VAR is fine there for the type guard, but not for the packed type guard.

I see. I looked into the master branch code.
So keep IS_VAR in PHP-8.3 and remove it in PHP-8.4 and master.

nielsdos linked an issue Jan 26, 2025 that may be closed by this pull request

JIT packed type guard crash #17577

Closed

github-actions bot added the Extension: opcache label Jan 26, 2025

nielsdos force-pushed the fix-17577 branch 3 times, most recently from ab89b9b to b477345 Compare January 26, 2025 13:57

nielsdos marked this pull request as ready for review January 26, 2025 15:22

nielsdos requested a review from dstogov as a code owner January 26, 2025 15:22

nielsdos force-pushed the fix-17577 branch from b477345 to f7b7472 Compare January 26, 2025 18:54

dstogov reviewed Jan 27, 2025

View reviewed changes

Review

a99025c

dstogov approved these changes Feb 3, 2025

View reviewed changes

nielsdos closed this in 0c3cf1f Feb 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GH-17577: JIT packed type guard crash #17584

Fix GH-17577: JIT packed type guard crash #17584

nielsdos commented Jan 26, 2025 •

edited

Loading

dstogov left a comment

nielsdos commented Jan 27, 2025

dstogov commented Jan 30, 2025

dstogov left a comment

dstogov Feb 3, 2025

nielsdos Feb 3, 2025

dstogov Feb 3, 2025

	\|\| (trace_buffer->stop == ZEND_JIT_TRACE_STOP_RECURSIVE_RET
	&& (opline-1)->result_type == IS_VAR
	&& EX_VAR_TO_NUM((opline-1)->result.var) == i))

Fix GH-17577: JIT packed type guard crash #17584

Fix GH-17577: JIT packed type guard crash #17584

Conversation

nielsdos commented Jan 26, 2025 • edited Loading

dstogov left a comment

Choose a reason for hiding this comment

nielsdos commented Jan 27, 2025

dstogov commented Jan 30, 2025

dstogov left a comment

Choose a reason for hiding this comment

dstogov Feb 3, 2025

Choose a reason for hiding this comment

nielsdos Feb 3, 2025

Choose a reason for hiding this comment

dstogov Feb 3, 2025

Choose a reason for hiding this comment

nielsdos commented Jan 26, 2025 •

edited

Loading