Fix GH-13834: Applying non-zero offset 36 to null pointer in zend_jit.c#13846
Fix GH-13834: Applying non-zero offset 36 to null pointer in zend_jit.c#13846ndossche merged 6 commits intophp:masterfrom
Conversation
…jit.c ssa_op can be NULL in function JIT. Doing pointer arithmetic on a NULL pointer is undefined behaviour. Undefined behaviour can be dangerous because the optimizer may assume then that the variable is not actually NULL. To solve this: 1. Add ADVANCE_SSA_OP() to safely add an offset to ssa_op in zend_jit.c 2. For inference, add an extra offset argument to the helper functions. To reproduce this, use Clang (not GCC) on a test like sapi/cli/tests/gh12363.phpt (or other tests also work).
|
Nice! I'm in favor of fixing this. Can you remove the |
|
Current remaining failures are |
Zend/Optimizer/zend_inference.h
Outdated
|
|
||
| #define DEFINE_SSA_OP_INFO(opN) \ | ||
| static zend_always_inline uint32_t _ssa_##opN##_info(const zend_op_array *op_array, const zend_ssa *ssa, const zend_op *opline, const zend_ssa_op *ssa_op) \ | ||
| static zend_always_inline uint32_t _ssa_##opN##_info(const zend_op_array *op_array, const zend_ssa *ssa, const zend_op *opline, const zend_ssa_op *ssa_op, size_t offset) \ |
There was a problem hiding this comment.
I would prefer not to change the "public" API prototypes.
Let me think a bit...
dstogov
left a comment
dstogov
left a comment
There was a problem hiding this comment.
Please check my comments. The changes to zend_jit_trace.c also look unrelated.
This probably should be targeted to PHP-8.2
| #define OP1_DATA_INFO() (_ssa_op1_info(op_array, ssa, (opline+1), (ssa_op+1))) | ||
| #define OP2_DATA_INFO() (_ssa_op2_info(op_array, ssa, (opline+1), (ssa_op+1))) |
There was a problem hiding this comment.
Can you change this into
-#define OP1_DATA_INFO() (_ssa_op1_info(op_array, ssa, (opline+1), (ssa_op+1)))
-#define OP2_DATA_INFO() (_ssa_op2_info(op_array, ssa, (opline+1), (ssa_op+1)))
+#define OP1_DATA_INFO() (_ssa_op1_info(op_array, ssa, (opline+1), ssa_op ? (ssa_op+1) : NULL))
+#define OP2_DATA_INFO() (_ssa_op2_info(op_array, ssa, (opline+1), ssa_op ? (ssa_op+1) : NULL))I think this should lead to a simple diff.
| int i; | ||
| int parent_vars_count = parent->exit_info[exit_num].stack_size; | ||
| zend_jit_trace_stack *parent_stack = | ||
| zend_jit_trace_stack *parent_stack = parent_vars_count == 0 ? NULL : |
There was a problem hiding this comment.
What do you fix by this? (this is not related).
There was a problem hiding this comment.
After removing -fno-sanitize=pointer-overflow from CI, more pointer arithmetic on NULL was found. This was one of the places that the sanitizer complained about. parent->stack_map may be NULL when parent_vars_count == 0, so I added this check to fix that.
|
Hi @nielsdos, it looks like this patch made "slowdown" on bench.php with tracing JIT. see https://nielsdos.github.io/php-benchmark-visualisation/ ---- TRACE 61 stop (loop)
---- TRACE 61 TSSA start (loop) sieve() /home/dmitry/php/php-master/Zend/bench.php:325
- ;#2.CV2($flags) [rc1, [packed, hash] array [long] of [long]]
+ ;#2.CV2($flags) [rc1, [packed, hash] array [long] of [long, double, string]]
;#3.CV3($i) [long] RANGE[2..8192]
;#4.CV4($k) [long] RANGE[4..16384]
LOOP:
- ;#8.CV2($flags) [rc1, [packed, hash] array [long] of [long]] = Phi(#2.CV2($flags) [rc1, [packed, hash] array [long] of [long]], #11.CV2($flags) [rc1, [p
acked, hash] array [long] of [long]])
+ ;#8.CV2($flags) [rc1, [packed, hash] array [long] of [long, double, string]] = Phi(#2.CV2($flags) [rc1, [packed, hash] array [long] of [long, double, st
ring]], #11.CV2($flags) [rc1, [packed, hash] array [long] of [long, double, string]])
;#9.CV4($k) [long] RANGE[4..16384] = Phi(#4.CV4($k) [long] RANGE[4..16384], #12.CV4($k) [long] RANGE[6..16384])
0019 #10.T5 [bool] = IS_SMALLER_OR_EQUAL #9.CV4($k) [long] RANGE[4..16384] int(8192) ; op1(int)
0020 ;JMPNZ #10.T5 [bool] 0016
-0016 ASSIGN_DIM #8.CV2($flags) [rc1, [packed, hash] array [long] of [long]] -> #11.CV2($flags) [rc1, [packed, hash] array [long] of [long]] #9.CV4($k) [long]
RANGE[4..16384] ; op1(packed array) op2(int) val(int)
+0016 ASSIGN_DIM #8.CV2($flags) [rc1, [packed, hash] array [long] of [long, double, string]] -> #11.CV2($flags) [rc1, [packed, hash] array [long] of [long, do
uble, string]] #9.CV4($k) [long] RANGE[4..16384] ; op1(packed array) op2(int) val(int)
0017 ;OP_DATA int(0)
0018 ADD #9.CV4($k) [long] RANGE[4..16384] #3.CV3($i) [long] RANGE[2..8192] #9.CV4($k) [long] RANGE[4..16384] -> #12.CV4($k) [long] RANGE[6..16384] ; op1(int
) op2(int)
---- TRACE 61 TSSA stop (loop)This later causes generation of more expensive code. I suspect, the problem is in changes of type inference for |
Yes, I have also noticed this and analysed this already. I had committed eb1cdb5 to fix this. |
Thanks. This seems fixed the problem. |
3 participants
ssa_op can be NULL in function JIT. Doing pointer arithmetic on a NULL pointer is undefined behaviour. Undefined behaviour can be dangerous because the optimizer may assume then that the variable is not actually NULL.
To solve this:
_ssa_op1_infowith helper macros because we would have to change those lines anyway for the extra argument.To reproduce this, use Clang (not GCC) on a test like sapi/cli/tests/gh12363.phpt (or other tests also work).
Targeting master, if no problems we can backport.