Fix GH-13834: Applying non-zero offset 36 to null pointer in zend_jit.c #13846

nielsdos · 2024-03-30T23:47:36Z

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:

Add ADVANCE_SSA_OP() to safely add an offset to ssa_op in zend_jit.c
For inference, add an extra offset argument to the helper functions.
Replace open-coded _ssa_op1_info with 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.

…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).

iluuu1994 · 2024-03-31T00:18:24Z

Nice! I'm in favor of fixing this. Can you remove the -fno-sanitize=pointer-overflow flag from .github/workflows/push.yml?

nielsdos · 2024-03-31T01:01:49Z

Current remaining failures are ~~all~~ mostly related to the stack map being NULL. I'll give it a go tomorrow.
EDIT: ASAN job now passes, macOS job has a failure but I have seen that one before on the master branch too.

dstogov · 2024-04-01T07:13:36Z

Zend/Optimizer/zend_inference.h

-	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) \


I would prefer not to change the "public" API prototypes.
Let me think a bit...

dstogov

Please check my comments. The changes to zend_jit_trace.c also look unrelated.
This probably should be targeted to PHP-8.2

dstogov · 2024-04-01T07:34:56Z

Zend/Optimizer/zend_inference.h

-#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)))


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.

dstogov · 2024-04-01T07:38:44Z

ext/opcache/jit/zend_jit_ir.c

-		zend_jit_trace_stack *parent_stack =
+		zend_jit_trace_stack *parent_stack = parent_vars_count == 0 ? NULL :


What do you fix by this? (this is not related).

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.

dstogov · 2024-04-01T20:55:37Z

Hi @nielsdos, it looks like this patch made "slowdown" on bench.php with tracing JIT. see https://nielsdos.github.io/php-benchmark-visualisation/
Checking the recent changes I see a difference in type inference in traces forsieve() function.

 ---- 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 range().
Could you please analyse this?

nielsdos · 2024-04-01T20:56:48Z

I suspect, the problem is in changes of type inference for range().
Could you please analyse this?

Yes, I have also noticed this and analysed this already. I had committed eb1cdb5 to fix this.

dstogov · 2024-04-01T20:59:58Z

I had committed eb1cdb5 to fix this.

Thanks. This seems fixed the problem.

nielsdos requested review from dstogov and iluuu1994 as code owners March 30, 2024 23:47

nielsdos linked an issue Mar 30, 2024 that may be closed by this pull request

Applying non-zero offset 36 to null pointer in zend_jit.c #13834

Closed

github-actions bot added Extension: opcache Category: Optimizer labels Mar 30, 2024

Remove -fno-sanitize=pointer-overflow flag from CI

befd724

nielsdos requested a review from TimWolla as a code owner March 31, 2024 00:26

TimWolla removed their request for review March 31, 2024 09:03

nielsdos added 3 commits March 31, 2024 16:57

Fix NULL pointer offsets added to the stack_map

09be586

Fix an offset add on a potentially NULL ssa->ops

1e7a540

Fix NULL pointer arithmetic in zend_range_info()

f7c9a5c

dstogov reviewed Apr 1, 2024

View reviewed changes

dstogov requested changes Apr 1, 2024

View reviewed changes

Address review comments

a37ee2f

dstogov approved these changes Apr 1, 2024

View reviewed changes

nielsdos merged commit 00c6d53 into php:master Apr 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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 #13846

Uh oh!

nielsdos commented Mar 30, 2024

Uh oh!

iluuu1994 commented Mar 31, 2024

Uh oh!

nielsdos commented Mar 31, 2024 •

edited

Loading

Uh oh!

dstogov Apr 1, 2024

Uh oh!

dstogov left a comment

Uh oh!

dstogov Apr 1, 2024

Uh oh!

nielsdos Apr 1, 2024

Uh oh!

dstogov Apr 1, 2024

Uh oh!

nielsdos Apr 1, 2024

Uh oh!

dstogov commented Apr 1, 2024

Uh oh!

nielsdos commented Apr 1, 2024

Uh oh!

dstogov commented Apr 1, 2024 •

edited

Loading

Uh oh!

Uh oh!

		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) \

		#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)))

		zend_jit_trace_stack *parent_stack =
		zend_jit_trace_stack *parent_stack = parent_vars_count == 0 ? NULL :

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 #13846

Uh oh!

Conversation

nielsdos commented Mar 30, 2024

Uh oh!

iluuu1994 commented Mar 31, 2024

Uh oh!

nielsdos commented Mar 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dstogov Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

dstogov Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

nielsdos Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

dstogov Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

nielsdos Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

dstogov commented Apr 1, 2024

Uh oh!

nielsdos commented Apr 1, 2024

Uh oh!

dstogov commented Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nielsdos commented Mar 31, 2024 •

edited

Loading

dstogov commented Apr 1, 2024 •

edited

Loading