Skip to content

Commit

Permalink
JIT: Better code for ADD/SUB/MUL and references in tracing JIT.
Browse files Browse the repository at this point in the history
  • Loading branch information
@dstogov
dstogov committed Aug 30, 2021
1 parent c19e4b9 commit 7690fa0
Showing 1 changed file with 32 additions and 4 deletions.
36 changes: 32 additions & 4 deletions ext/opcache/jit/zend_jit_trace.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4208,9 +4208,37 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par
case ZEND_MUL:
// case ZEND_DIV: // TODO: check for division by zero ???
op1_info = OP1_INFO();
CHECK_OP1_TRACE_TYPE();
op1_addr = OP1_REG_ADDR();

This comment has been minimized.

Sign in to view

Copy link
@kamil-tekiela

kamil-tekiela Aug 30, 2021

Member

Hi Dmitry, It looks like this change broke ext/date/tests/bug30096.phpt on Travis. The difference is not calculated properly on Arm

This comment has been minimized.

Sign in to view

Copy link
@dstogov

dstogov Aug 30, 2021

Author Member

@kamil-tekiela thanks for notice. I'll fix this in an hour or revert.

This comment has been minimized.

Sign in to view

Copy link
@dstogov

dstogov Aug 30, 2021

Author Member

the problem should be fixed by f1f4403
if (opline->op1_type != IS_CONST
&& orig_op1_type != IS_UNKNOWN
&& (orig_op1_type & IS_TRACE_REFERENCE)) {
if (!zend_jit_fetch_reference(&dasm_state, opline, orig_op1_type, &op1_info, &op1_addr,
!ssa->var_info[ssa_op->op1_use].guarded_reference, 1)) {
goto jit_failure;
}
if (opline->op1_type == IS_CV
&& ssa->vars[ssa_op->op1_use].alias == NO_ALIAS) {
ssa->var_info[ssa_op->op1_use].guarded_reference = 1;
}
} else {
CHECK_OP1_TRACE_TYPE();
}
op2_info = OP2_INFO();
CHECK_OP2_TRACE_TYPE();
op2_addr = OP2_REG_ADDR();
if (opline->op2_type != IS_CONST
&& orig_op2_type != IS_UNKNOWN
&& (orig_op2_type & IS_TRACE_REFERENCE)) {
if (!zend_jit_fetch_reference(&dasm_state, opline, orig_op2_type, &op2_info, &op2_addr,
!ssa->var_info[ssa_op->op2_use].guarded_reference, 1)) {
goto jit_failure;
}
if (opline->op2_type == IS_CV
&& ssa->vars[ssa_op->op2_use].alias == NO_ALIAS) {
ssa->var_info[ssa_op->op2_use].guarded_reference = 1;
}
} else {
CHECK_OP2_TRACE_TYPE();
}
if ((op1_info & MAY_BE_UNDEF) || (op2_info & MAY_BE_UNDEF)) {
break;
}
Expand Down Expand Up @@ -4245,8 +4273,8 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par
}
} else {
if (!zend_jit_math(&dasm_state, opline,
op1_info, OP1_REG_ADDR(),
op2_info, OP2_REG_ADDR(),
op1_info, op1_addr,
op2_info, op2_addr,
res_use_info, res_info, res_addr,
(op1_info & MAY_BE_LONG) && (op2_info & MAY_BE_LONG) && (res_info & (MAY_BE_DOUBLE|MAY_BE_GUARD)) && zend_may_overflow(opline, ssa_op, op_array, ssa),
zend_may_throw(opline, ssa_op, op_array, ssa))) {
Expand Down

4 comments on commit 7690fa0

@shqking
Copy link
Contributor

@shqking shqking commented on 7690fa0 Sep 2, 2021 •
edited

Choose a reason for hiding this comment

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

Hi, Dmitry.

I noticed that symfony test in community job failed after this commit. See https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=19657&view=results
It's a heap buffer overflow when testing src/Symfony/Component/Messenger/Bridge/Doctrine

Here is the ASAN log.

php ./phpunit src/Symfony/Component/Messenger/Bridge/Doctrine --exclude-group tty,benchmark,intl-data,transient
PHPUnit 9.5.9 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing /opt/php-oss/symfony/src/Symfony/Component/Messenger/Bridge/Doctrine
...........=================================================================
==10221==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000af05c8 at pc 0x55c3248b5008 bp 0x7ffc24a027f0 sp 0x7ffc24a027e0
READ of size 8 at 0x603000af05c8 thread T0
    #0 0x55c3248b5007 in zend_array_dup /opt/php-src/Zend/zend_hash.c:2107
    #1 0x7fa541631def in zend_jit_add_arrays_helper ext/opcache/jit/zend_jit_helpers.c:1990
    #2 0x49a61947  (/dev/zero (deleted)+0x806a947)

0x603000af05c8 is located 8 bytes to the right of 32-byte region [0x603000af05a0,0x603000af05c0)
allocated by thread T0 here:
    #0 0x7fa5496cfbc8 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10dbc8)
    #1 0x55c3247dbb67 in __zend_malloc /opt/php-src/Zend/zend_alloc.c:3043
    #2 0x55c3247da281 in tracked_malloc /opt/php-src/Zend/zend_alloc.c:2746
    #3 0x55c3247d9308 in _malloc_custom /opt/php-src/Zend/zend_alloc.c:2419
    #4 0x55c3247d9575 in _emalloc /opt/php-src/Zend/zend_alloc.c:2538
    #5 0x49a617d0  (/dev/zero (deleted)+0x806a7d0)
    #6 0x55c324a5d226 in zend_execute /opt/php-src/Zend/zend_vm_execute.h:58843
    #7 0x55c324874176 in zend_execute_scripts /opt/php-src/Zend/zend.c:1761
    #8 0x55c32470aada in php_execute_script /opt/php-src/main/main.c:2519
    #9 0x55c324c30ead in do_cli /opt/php-src/sapi/cli/php_cli.c:965
    #10 0x55c324c33136 in main /opt/php-src/sapi/cli/php_cli.c:1367
    #11 0x7fa5476960b2 in __libc_start_main (/usr/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

SUMMARY: AddressSanitizer: heap-buffer-overflow /opt/php-src/Zend/zend_hash.c:2107 in zend_array_dup
Shadow bytes around the buggy address:
  0x0c0680156060: fd fd fd fa fa fa fd fd fd fd fa fa fd fd fd fd
  0x0c0680156070: fa fa fd fd fd fd fa fa fd fd fd fd fa fa fd fd
  0x0c0680156080: fd fa fa fa fd fd fd fd fa fa fd fd fd fd fa fa
  0x0c0680156090: fd fd fd fd fa fa fd fd fd fd fa fa fd fd fd fd
  0x0c06801560a0: fa fa fd fd fd fd fa fa fd fd fd fd fa fa 00 00
=>0x0c06801560b0: 00 00 fa fa 00 00 00 00 fa[fa]fd fd fd fa fa fa
  0x0c06801560c0: fd fd fd fd fa fa 00 00 00 00 fa fa fa fa fa fa
  0x0c06801560d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c06801560e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c06801560f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0680156100: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==10221==ABORTING

In my local tests, I found that only tracing JIT is affected. Both JIT/x86 and JIT/arm64 in all ZTS/NTS/HYRBID/CALL modes are affected.

Note that, this overflow does not occur previously(i.e. the community job test for symfony succeeded yesterday)
mainly because that the symfony is updated now.

Could you help to take a look at this when you have spare time? Thanks.

@dstogov
Copy link
Member Author

@dstogov dstogov commented on 7690fa0 Sep 2, 2021 via email

Choose a reason for hiding this comment

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

@dstogov
Copy link
Member Author

@dstogov dstogov commented on 7690fa0 Sep 2, 2021 via email

Choose a reason for hiding this comment

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

@shqking
Copy link
Contributor

@shqking shqking commented on 7690fa0 Sep 3, 2021

Choose a reason for hiding this comment

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

Thanks for your fix in cbc925e
Tests with JIT/arm64 and JIT/x86 are both passed now.

Please sign in to comment.