Skip to content

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

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

Merged
merged 6 commits into from
Apr 1, 2024
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7b6523c
Fix GH-13834: Applying non-zero offset 36 to null pointer in zend_jit.c
nielsdos Mar 30, 2024
befd724
Remove -fno-sanitize=pointer-overflow flag from CI
nielsdos Mar 31, 2024
09be586
Fix NULL pointer offsets added to the stack_map
nielsdos Mar 31, 2024
1e7a540
Fix an offset add on a potentially NULL ssa->ops
nielsdos Mar 31, 2024
f7c9a5c
Fix NULL pointer arithmetic in zend_range_info()
nielsdos Mar 31, 2024
a37ee2f
Address review comments
nielsdos Apr 1, 2024
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
2 changes: 1 addition & 1 deletion .github/workflows/push.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ jobs:
configurationParameters: >-
--${{ matrix.debug && 'enable' || 'disable' }}-debug
--${{ matrix.zts && 'enable' || 'disable' }}-zts
${{ matrix.asan && 'CFLAGS="-fsanitize=undefined,address -fno-sanitize=pointer-overflow -DZEND_TRACK_ARENA_ALLOC" LDFLAGS="-fsanitize=undefined,address -fno-sanitize=pointer-overflow" CC=clang-16 CXX=clang++-16' || '' }}
${{ matrix.asan && 'CFLAGS="-fsanitize=undefined,address -DZEND_TRACK_ARENA_ALLOC" LDFLAGS="-fsanitize=undefined,address" CC=clang-16 CXX=clang++-16' || '' }}
skipSlow: ${{ matrix.asan }}
- name: make
run: make -j$(/usr/bin/nproc) >/dev/null
Expand Down
12 changes: 6 additions & 6 deletions Zend/Optimizer/zend_func_info.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,16 @@ static uint32_t zend_range_info(const zend_call_info *call_info, const zend_ssa
&& ssa
&& !(ssa->cfg.flags & ZEND_SSA_TSSA)) {
zend_op_array *op_array = call_info->caller_op_array;
uint32_t t1 = _ssa_op1_info(op_array, ssa, call_info->arg_info[0].opline,
&ssa->ops[call_info->arg_info[0].opline - op_array->opcodes]);
uint32_t t2 = _ssa_op1_info(op_array, ssa, call_info->arg_info[1].opline,
&ssa->ops[call_info->arg_info[1].opline - op_array->opcodes]);
uint32_t t1 = _ssa_op1_info(op_array, ssa, op_array->opcodes,
ssa->ops, call_info->arg_info[0].opline - op_array->opcodes);
uint32_t t2 = _ssa_op1_info(op_array, ssa, op_array->opcodes,
ssa->ops, call_info->arg_info[1].opline - op_array->opcodes);
uint32_t t3 = 0;
uint32_t tmp = MAY_BE_RC1 | MAY_BE_ARRAY;

if (call_info->num_args == 3) {
t3 = _ssa_op1_info(op_array, ssa, call_info->arg_info[2].opline,
&ssa->ops[call_info->arg_info[2].opline - op_array->opcodes]);
t3 = _ssa_op1_info(op_array, ssa, op_array->opcodes,
ssa->ops, call_info->arg_info[2].opline - op_array->opcodes);
}
if ((t1 & MAY_BE_STRING) && (t2 & MAY_BE_STRING)) {
tmp |= MAY_BE_ARRAY_OF_LONG | MAY_BE_ARRAY_OF_DOUBLE | MAY_BE_ARRAY_OF_STRING;
Expand Down
4 changes: 2 additions & 2 deletions Zend/Optimizer/zend_inference.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5185,7 +5185,7 @@ ZEND_API bool zend_may_throw_ex(const zend_op *opline, const zend_ssa_op *ssa_op
return 0;
case ZEND_ASSIGN_DIM:
if ((opline+1)->op1_type == IS_CV) {
if (_ssa_op1_info(op_array, ssa, opline+1, ssa_op+1) & MAY_BE_UNDEF) {
if (OP1_DATA_INFO() & MAY_BE_UNDEF) {
return 1;
}
}
Expand All @@ -5196,7 +5196,7 @@ ZEND_API bool zend_may_throw_ex(const zend_op *opline, const zend_ssa_op *ssa_op
return 1;
}
if ((opline+1)->op1_type == IS_CV) {
if (_ssa_op1_info(op_array, ssa, opline+1, ssa_op+1) & MAY_BE_UNDEF) {
if (OP1_DATA_INFO() & MAY_BE_UNDEF) {
return 1;
}
}
Expand Down
29 changes: 15 additions & 14 deletions Zend/Optimizer/zend_inference.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,19 +172,20 @@ static zend_always_inline uint32_t get_ssa_var_info(const zend_ssa *ssa, int ssa
}

#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) \
Copy link
Member

Choose a reason for hiding this comment

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

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

{ \
opline += offset; \
if (opline->opN##_type == IS_CONST) { \
return _const_op_type(CRT_CONSTANT(opline->opN)); \
} else { \
return get_ssa_var_info(ssa, ssa->var_info ? ssa_op->opN##_use : -1); \
return get_ssa_var_info(ssa, ssa->var_info ? (ssa_op + offset)->opN##_use : -1); \
} \
} \

#define DEFINE_SSA_OP_DEF_INFO(opN) \
static zend_always_inline uint32_t _ssa_##opN##_def_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##_def_info(const zend_op_array *op_array, const zend_ssa *ssa, const zend_op *opline, const zend_ssa_op *ssa_op, size_t offset) \
{ \
return get_ssa_var_info(ssa, ssa->var_info ? ssa_op->opN##_def : -1); \
return get_ssa_var_info(ssa, ssa->var_info ? (ssa_op + offset)->opN##_def : -1); \
} \


Expand All @@ -195,16 +196,16 @@ DEFINE_SSA_OP_DEF_INFO(op1)
DEFINE_SSA_OP_DEF_INFO(op2)
DEFINE_SSA_OP_DEF_INFO(result)

#define OP1_INFO() (_ssa_op1_info(op_array, ssa, opline, ssa_op))
#define OP2_INFO() (_ssa_op2_info(op_array, ssa, opline, ssa_op))
#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)))
Comment on lines -200 to -201
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, done.

#define RES_USE_INFO() (_ssa_result_info(op_array, ssa, opline, ssa_op))
#define OP1_DEF_INFO() (_ssa_op1_def_info(op_array, ssa, opline, ssa_op))
#define OP2_DEF_INFO() (_ssa_op2_def_info(op_array, ssa, opline, ssa_op))
#define OP1_DATA_DEF_INFO() (_ssa_op1_def_info(op_array, ssa, (opline+1), (ssa_op+1)))
#define OP2_DATA_DEF_INFO() (_ssa_op2_def_info(op_array, ssa, (opline+1), (ssa_op+1)))
#define RES_INFO() (_ssa_result_def_info(op_array, ssa, opline, ssa_op))
#define OP1_INFO() (_ssa_op1_info(op_array, ssa, opline, ssa_op, 0))
#define OP2_INFO() (_ssa_op2_info(op_array, ssa, opline, ssa_op, 0))
#define OP1_DATA_INFO() (_ssa_op1_info(op_array, ssa, opline, ssa_op, 1))
#define OP2_DATA_INFO() (_ssa_op2_info(op_array, ssa, opline, ssa_op, 1))
#define RES_USE_INFO() (_ssa_result_info(op_array, ssa, opline, ssa_op, 0))
#define OP1_DEF_INFO() (_ssa_op1_def_info(op_array, ssa, opline, ssa_op, 0))
#define OP2_DEF_INFO() (_ssa_op2_def_info(op_array, ssa, opline, ssa_op, 0))
#define OP1_DATA_DEF_INFO() (_ssa_op1_def_info(op_array, ssa, opline, ssa_op, 1))
#define OP2_DATA_DEF_INFO() (_ssa_op2_def_info(op_array, ssa, opline, ssa_op, 1))
#define RES_INFO() (_ssa_result_def_info(op_array, ssa, opline, ssa_op, 0))

static zend_always_inline bool zend_add_will_overflow(zend_long a, zend_long b) {
return (b > 0 && a > ZEND_LONG_MAX - b)
Expand Down
19 changes: 12 additions & 7 deletions ext/opcache/jit/zend_jit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,14 +243,19 @@ static int zend_jit_is_constant_cmp_long_long(const zend_op *opline,
return 0;
}

#define ADVANCE_SSA_OP(ssa_op, offset) \
do { \
if (ssa_op) ssa_op += offset; \
} while (0)

static int zend_jit_needs_call_chain(zend_call_info *call_info, uint32_t b, const zend_op_array *op_array, zend_ssa *ssa, const zend_ssa_op *ssa_op, const zend_op *opline, int call_level, zend_jit_trace_rec *trace)
{
int skip;

if (trace) {
zend_jit_trace_rec *p = trace;

ssa_op++;
ADVANCE_SSA_OP(ssa_op, 1);
while (1) {
if (p->op == ZEND_JIT_TRACE_VM) {
switch (p->opline->opcode) {
Expand Down Expand Up @@ -305,7 +310,7 @@ static int zend_jit_needs_call_chain(zend_call_info *call_info, uint32_t b, cons
return 1;
}
}
ssa_op += zend_jit_trace_op_len(opline);
ADVANCE_SSA_OP(ssa_op, zend_jit_trace_op_len(opline));
} else if (p->op == ZEND_JIT_TRACE_ENTER ||
p->op == ZEND_JIT_TRACE_BACK ||
p->op == ZEND_JIT_TRACE_END) {
Expand All @@ -319,7 +324,7 @@ static int zend_jit_needs_call_chain(zend_call_info *call_info, uint32_t b, cons
const zend_op *end = op_array->opcodes + op_array->last;

opline++;
ssa_op++;
ADVANCE_SSA_OP(ssa_op, 1);
skip = (call_level == 1);
while (opline != end) {
if (!skip) {
Expand Down Expand Up @@ -381,7 +386,7 @@ static int zend_jit_needs_call_chain(zend_call_info *call_info, uint32_t b, cons
return 0;
}
opline++;
ssa_op++;
ADVANCE_SSA_OP(ssa_op, 1);
}

return 1;
Expand All @@ -395,7 +400,7 @@ static int zend_jit_needs_call_chain(zend_call_info *call_info, uint32_t b, cons
}

opline++;
ssa_op++;
ADVANCE_SSA_OP(ssa_op, 1);
skip = (call_level == 1);
while (opline != end) {
if (skip) {
Expand All @@ -421,7 +426,7 @@ static int zend_jit_needs_call_chain(zend_call_info *call_info, uint32_t b, cons
}
}
opline++;
ssa_op++;
ADVANCE_SSA_OP(ssa_op, 1);
}

return 0;
Expand All @@ -441,7 +446,7 @@ static uint32_t skip_valid_arguments(const zend_op_array *op_array, zend_ssa *ss
if (ZEND_TYPE_IS_SET(arg_info->type)) {
if (ZEND_TYPE_IS_ONLY_MASK(arg_info->type)) {
zend_op *opline = call_info->arg_info[num_args].opline;
zend_ssa_op *ssa_op = &ssa->ops[opline - op_array->opcodes];
zend_ssa_op *ssa_op = ssa->ops ? &ssa->ops[opline - op_array->opcodes] : NULL;
uint32_t type_mask = ZEND_TYPE_PURE_MASK(arg_info->type);
if ((OP1_INFO() & (MAY_BE_ANY|MAY_BE_UNDEF)) & ~type_mask) {
break;
Expand Down
4 changes: 2 additions & 2 deletions ext/opcache/jit/zend_jit_ir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15240,7 +15240,7 @@ static int zend_jit_switch(zend_jit_ctx *jit, const zend_op *opline, const zend_
jit->b = -1;
}
} else {
zend_ssa_op *ssa_op = &ssa->ops[opline - op_array->opcodes];
zend_ssa_op *ssa_op = ssa->ops ? &ssa->ops[opline - op_array->opcodes] : NULL;
uint32_t op1_info = OP1_INFO();
zend_jit_addr op1_addr = OP1_ADDR();
const zend_op *default_opline = ZEND_OFFSET_TO_OPLINE(opline, opline->extended_value);
Expand Down Expand Up @@ -16301,7 +16301,7 @@ static int zend_jit_trace_start(zend_jit_ctx *jit,
if (parent) {
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 :
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

parent->stack_map +
parent->exit_info[exit_num].stack_offset;

Expand Down
4 changes: 2 additions & 2 deletions ext/opcache/jit/zend_jit_trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2779,7 +2779,7 @@ static zend_jit_reg_var* zend_jit_trace_allocate_registers(zend_jit_trace_rec *t
zend_jit_trace_stack *stack;
uint32_t parent_vars_count = parent_trace ?
zend_jit_traces[parent_trace].exit_info[exit_num].stack_size : 0;
zend_jit_trace_stack *parent_stack = parent_trace ?
zend_jit_trace_stack *parent_stack = parent_vars_count ?
zend_jit_traces[parent_trace].stack_map +
zend_jit_traces[parent_trace].exit_info[exit_num].stack_offset : NULL;
checkpoint = zend_arena_checkpoint(CG(arena));
Expand Down Expand Up @@ -8386,7 +8386,7 @@ int ZEND_FASTCALL zend_jit_trace_exit(uint32_t exit_num, zend_jit_registers_buf
/* Deoptimization of VM stack state */
uint32_t i;
uint32_t stack_size = t->exit_info[exit_num].stack_size;
zend_jit_trace_stack *stack = t->stack_map + t->exit_info[exit_num].stack_offset;
zend_jit_trace_stack *stack = stack_size ? t->stack_map + t->exit_info[exit_num].stack_offset : NULL;

if (t->exit_info[exit_num].flags & ZEND_JIT_EXIT_RESTORE_CALL) {
zend_execute_data *call = (zend_execute_data *)regs->gpr[ZREG_RX];
Expand Down