segfault in Zend/tests/assign_coalesce_001.phpt

[divinity76]

Description

Doing the following:

hans@DESKTOP-EE15SLU:~/projects/php-src$ git log -1
commit da6a4e799aafdf33c8701fcdd632d646f61e612e (HEAD -> master, origin/master, origin/HEAD)
Merge: 1cc0a16752 ae44ab47a7
Author: Jakub Zelenka <[email protected]>
Date:   Sun Feb 4 12:01:09 2024 +0000

    Merge branch 'PHP-8.3'
hans@DESKTOP-EE15SLU:~/projects/php-src$ git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
hans@DESKTOP-EE15SLU:~/projects/php-src$ ./buildconf >/dev/null
hans@DESKTOP-EE15SLU:~/projects/php-src$ ./configure --disable-all --disable-cgi --enable-cli >/dev/null
hans@DESKTOP-EE15SLU:~/projects/php-src$ make clean >/dev/null;
hans@DESKTOP-EE15SLU:~/projects/php-src$ make -j$(nproc) >/dev/null
/home/hans/projects/php-src/ext/pcre/pcre2lib/pcre2_compile.c: In function ‘compile_regex’:
/home/hans/projects/php-src/ext/pcre/pcre2lib/pcre2_compile.c:8169:17: warning: storing the address of local variable ‘capitem’ in ‘*cb.open_caps’ [-Wdangling-pointer=]
 8169 |   cb->open_caps = &capitem;
      |   ~~~~~~~~~~~~~~^~~~~~~~~~
/home/hans/projects/php-src/ext/pcre/pcre2lib/pcre2_compile.c:8106:14: note: ‘capitem’ declared here
 8106 | open_capitem capitem;
      |              ^~~~~~~
/home/hans/projects/php-src/ext/pcre/pcre2lib/pcre2_compile.c:8100:39: note: ‘cb’ declared here
 8100 |   branch_chain *bcptr, compile_block *cb, PCRE2_SIZE *lengthptr)
In file included from /home/hans/projects/php-src/Zend/zend.h:32,
                 from /home/hans/projects/php-src/Zend/zend_API.c:22:
In function ‘zend_check_magic_method_implementation’,
    inlined from ‘zend_register_functions’ at /home/hans/projects/php-src/Zend/zend_API.c:3035:4:
/home/hans/projects/php-src/Zend/zend_API.c:2636:34: warning: array subscript ‘zend_function {aka const union _zend_function}[0]’ is partly outside array bounds of ‘unsigned char[136]’ [-Warray-bounds=]
 2636 |         if (ZSTR_VAL(fptr->common.function_name)[0] != '_'
/home/hans/projects/php-src/Zend/zend_string.h:68:26: note: in definition of macro ‘ZSTR_VAL’
   68 | #define ZSTR_VAL(zstr)  (zstr)->val
      |                          ^~~~
/home/hans/projects/php-src/Zend/zend_API.c: In function ‘zend_register_functions’:
/home/hans/projects/php-src/Zend/zend_API.c:2926:32: note: object of size 136 allocated by ‘malloc’
 2926 |                 reg_function = malloc(sizeof(zend_internal_function));
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hans@DESKTOP-EE15SLU:~/projects/php-src$ ./sapi/cli/php ./run-tests.php Zend/tests/assign_coalesce_001.phpt --show-diff

resulted in:

=====================================================================
PHP         : /home/hans/projects/php-src/sapi/cli/php
PHP_SAPI    : cli
PHP_VERSION : 8.4.0-dev
ZEND_VERSION: 4.4.0-dev
PHP_OS      : Linux - Linux DESKTOP-EE15SLU 5.15.133.1-microsoft-standard-WSL2 #1 SMP Thu Oct 5 21:02:42 UTC 2023 x86_64
INI actual  : /home/hans/projects/php-src
More .INIs  :
---------------------------------------------------------------------
PHP         : /home/hans/projects/php-src/sapi/phpdbg/phpdbg
PHP_SAPI    : phpdbg
PHP_VERSION : 8.4.0-dev
ZEND_VERSION: 4.4.0-dev
PHP_OS      : Linux - Linux DESKTOP-EE15SLU 5.15.133.1-microsoft-standard-WSL2 #1 SMP Thu Oct 5 21:02:42 UTC 2023 x86_64
INI actual  : /home/hans/projects/php-src
More .INIs  :
---------------------------------------------------------------------
CWD         : /home/hans/projects/php-src
Extra dirs  :
VALGRIND    : Not used
=====================================================================
Running selected tests.
TEST 1/1 [Zend/tests/assign_coalesce_001.phpt]
========DIFF========
--
     }

     Static props:
042- int(123)
043- string(3) "foo"
044- id(foo)
045- id(foo)
046- id(bar)
047- int(123)
048- string(3) "foo"
042+ Segmentation fault
043+
044+ Termsig=11
========DONE========
FAIL Coalesce assign (??=): Basic behavior [Zend/tests/assign_coalesce_001.phpt]
=====================================================================
Number of tests :     1                 1
Tests skipped   :     0 (  0.0%) --------
Tests warned    :     0 (  0.0%) (  0.0%)
Tests failed    :     1 (100.0%) (100.0%)
Tests passed    :     0 (  0.0%) (  0.0%)
---------------------------------------------------------------------
Time taken      : 0.003 seconds
=====================================================================

=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
Coalesce assign (??=): Basic behavior [Zend/tests/assign_coalesce_001.phpt]
=====================================================================

but I expected the test to pass.

PHP Version

8.4.0-dev commit da6a4e7

Operating System

WSL2 Ubuntu 24.04-dev

[iluuu1994]

Can you try a debug build and the address/undefined sanitizers with USE_ZEND_ALLOC=0? Is the GCC version of 24.04-dev stable?

[devnexen]

Static props:
AddressSanitizer:DEADLYSIGNAL
=================================================================
==206313==ERROR: AddressSanitizer: SEGV on unknown address 0x001d7fff80c3 (pc 0x55d437cbda4a bp 0x7ffe4bfe0fc0 sp 0x7ffe4bfe0fa0 T0)
==206313==The signal is caused by a READ memory access.
    #0 0x55d437cbda4a in zend_string_hash_val /home/dcarlier/Contribs/php-src/Zend/zend_string.h:141
    #1 0x55d437cbda4a in zend_hash_find /home/dcarlier/Contribs/php-src/Zend/zend_hash.c:2666
    #2 0x55d437ecf1c9 in zend_hash_find_ptr /home/dcarlier/Contribs/php-src/Zend/zend_hash.h:869
    #3 0x55d437ecf1c9 in zend_std_get_static_property_with_info /home/dcarlier/Contribs/php-src/Zend/zend_object_handlers.c:1583
    #4 0x55d437d09f53 in zend_fetch_static_property_address_ex /home/dcarlier/Contribs/php-src/Zend/zend_execute.c:3399
    #5 0x55d437e0f754 in zend_fetch_static_property_address /home/dcarlier/Contribs/php-src/Zend/zend_execute.c:3452
    #6 0x55d437e0f754 in zend_fetch_static_prop_helper_SPEC /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:861
    #7 0x55d437e2d50a in ZEND_FETCH_STATIC_PROP_IS_SPEC_HANDLER /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:910
    #8 0x55d437e2d50a in execute_ex /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:57065
    #9 0x55d437e608d5 in zend_execute /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:62621
    #10 0x55d437c70067 in zend_execute_scripts /home/dcarlier/Contribs/php-src/Zend/zend.c:1885
    #11 0x55d437b35cb6 in php_execute_script /home/dcarlier/Contribs/php-src/main/main.c:2504
    #12 0x55d437fdfc31 in do_cli /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:966
    #13 0x55d437698540 in main /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:1340
    #14 0x7f50f742814f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #15 0x7f50f7428208 in __libc_start_main_impl ../csu/libc-start.c:360

[divinity76]

@iluuu1994 annoyingly, no: the segfault does not happen in debug builds, the test pass with ./configure --enable-debug
luckily @devnexen is able to reproduce with address sanitizer (valgrind?)

Is the GCC version of 24.04-dev stable?

should be, based on gcc release version 13.2.0 with some ubuntu patches

[devnexen]

with a pure debug build, the segfault does not occur (zeroed field ?) however with asan you get the report.

[iluuu1994]

Thank you. I'm using version 13.2.1 20231205 (Red Hat 13.2.1-6) (GCC). I suppose it's possible this is a miscompilation on the Ubuntu version, or something that was fixed in version 13.2.1.

@devnexen You might also try memory sanitizer, if you suspect this could be due to an uninitialized memory read.

[devnexen]

might be not it, works fine with msan.

[divinity76]

doing run-tests.php with -m --show-mem with a release-build gave me a slightly different output from @devnexen

$ ./sapi/cli/php ./run-tests.php Zend/tests/assign_coalesce_001.phpt --show-mem -m

=====================================================================
PHP         : /home/hans/projects/php-src/sapi/cli/php
PHP_SAPI    : cli
PHP_VERSION : 8.4.0-dev
ZEND_VERSION: 4.4.0-dev
PHP_OS      : Linux - Linux DESKTOP-EE15SLU 5.15.133.1-microsoft-standard-WSL2 #1 SMP Thu Oct 5 21:02:42 UTC 2023 x86_64INI actual  : /home/hans/projects/php-src
More .INIs  :
---------------------------------------------------------------------
PHP         : /home/hans/projects/php-src/sapi/phpdbg/phpdbg
PHP_SAPI    : phpdbg
PHP_VERSION : 8.4.0-dev
ZEND_VERSION: 4.4.0-dev
PHP_OS      : Linux - Linux DESKTOP-EE15SLU 5.15.133.1-microsoft-standard-WSL2 #1 SMP Thu Oct 5 21:02:42 UTC 2023 x86_64INI actual  : /home/hans/projects/php-src
More .INIs  :
---------------------------------------------------------------------
CWD         : /home/hans/projects/php-src
Extra dirs  :
VALGRIND    : valgrind-3.22.0 (memcheck)
=====================================================================
Running selected tests.
TEST 1/1 [Zend/tests/assign_coalesce_001.phpt]
========MEM========
==1046412== Invalid read of size 8
==1046412==    at 0x56006B: zend_string_hash_val (zend_string.h:141)
==1046412==    by 0x56006B: zend_hash_find (zend_hash.c:2666)
==1046412==    by 0x5DFE50: zend_hash_find_ptr (zend_hash.h:869)
==1046412==    by 0x5DFE50: zend_std_get_static_property_with_info (zend_object_handlers.c:1583)
==1046412==    by 0x572523: zend_fetch_static_property_address_ex (zend_execute.c:3399)
==1046412==    by 0x5AC945: zend_fetch_static_property_address (zend_execute.c:3452)
==1046412==    by 0x5AC945: zend_fetch_static_prop_helper_SPEC (zend_vm_execute.h:861)
==1046412==    by 0x5B3510: ZEND_FETCH_STATIC_PROP_IS_SPEC_HANDLER (zend_vm_execute.h:910)
==1046412==    by 0x5B3510: execute_ex (zend_vm_execute.h:57065)
==1046412==    by 0x5BD2BF: zend_execute (zend_vm_execute.h:62621)
==1046412==    by 0x54A771: zend_execute_scripts (zend.c:1885)
==1046412==    by 0x4DDC11: php_execute_script (main.c:2504)
==1046412==    by 0x6343BB: do_cli (php_cli.c:966)
==1046412==    by 0x33FC93: main (php_cli.c:1340)
==1046412==  Address 0xe800000618 is not stack'd, malloc'd or (recently) free'd
==1046412==
==1046412==
==1046412== Process terminating with default action of signal 11 (SIGSEGV)
==1046412==  Access not within mapped region at address 0xE800000618
==1046412==    at 0x56006B: zend_string_hash_val (zend_string.h:141)
==1046412==    by 0x56006B: zend_hash_find (zend_hash.c:2666)
==1046412==    by 0x5DFE50: zend_hash_find_ptr (zend_hash.h:869)
==1046412==    by 0x5DFE50: zend_std_get_static_property_with_info (zend_object_handlers.c:1583)
==1046412==    by 0x572523: zend_fetch_static_property_address_ex (zend_execute.c:3399)
==1046412==    by 0x5AC945: zend_fetch_static_property_address (zend_execute.c:3452)
==1046412==    by 0x5AC945: zend_fetch_static_prop_helper_SPEC (zend_vm_execute.h:861)
==1046412==    by 0x5B3510: ZEND_FETCH_STATIC_PROP_IS_SPEC_HANDLER (zend_vm_execute.h:910)
==1046412==    by 0x5B3510: execute_ex (zend_vm_execute.h:57065)
==1046412==    by 0x5BD2BF: zend_execute (zend_vm_execute.h:62621)
==1046412==    by 0x54A771: zend_execute_scripts (zend.c:1885)
==1046412==    by 0x4DDC11: php_execute_script (main.c:2504)
==1046412==    by 0x6343BB: do_cli (php_cli.c:966)
==1046412==    by 0x33FC93: main (php_cli.c:1340)
==1046412==  If you believe this happened as a result of a stack
==1046412==  overflow in your program's main thread (unlikely but
==1046412==  possible), you can try to increase the size of the
==1046412==  main thread stack using the --main-stacksize= flag.
==1046412==  The main thread stack size used in this run was 8388608.
========DONE========
LEAK&FAIL Coalesce assign (??=): Basic behavior [Zend/tests/assign_coalesce_001.phpt]
=====================================================================
Number of tests :     1                 1
Tests skipped   :     0 (  0.0%) --------
Tests warned    :     0 (  0.0%) (  0.0%)
Tests failed    :     0 (  0.0%) (  0.0%)
Tests leaked    :     1 (100.0%) (100.0%)
Tests passed    :     0 (  0.0%) (  0.0%)
---------------------------------------------------------------------
Time taken      : 0.856 seconds
=====================================================================

=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
Coalesce assign (??=): Basic behavior [Zend/tests/assign_coalesce_001.phpt]
=====================================================================

=====================================================================
LEAKED TEST SUMMARY
---------------------------------------------------------------------
Coalesce assign (??=): Basic behavior [Zend/tests/assign_coalesce_001.phpt]
=====================================================================

[divinity76]

seems when the issue does happen, Z_STR_P returns a bogus pointer in

php-src/Zend/zend_execute.c

Line 3398 in fe064d7

name = Z_STR_P(RT_CONSTANT(opline, opline->op1));

and opline->op1.var and opline->op1.num are both 80, whatever that means - but in debug mode, where it doesn't happen, opline->op1.var and opline->op1.num are both 2544

[divinity76]

@iluuu1994 it's all your fault! semi-kidding, seems the guilty commit is bf4ec8b, doing

git revert bf4ec8bd9d9d30713a9d3294b946084a6e9e088a

fixes it?
@devnexen can you confirm?

[iluuu1994]

@divinity76 Thanks for the analysis. It makes sense for this to fix the release build if any of the assumptions are wrong. It seems odd for this to fix debug builds, given that they are usually compiled to asserts, unless ZEND_ASSUME is used explicitly. Can you try converting the ZEND_ASSUMEs in zend_hash.h to ZEND_ASSERTs and see if any of them fail? You'll need a debug build for this.

[devnexen]

I do confirm. Maybe we can give it another try for gcc >= 14, after all it is pretty recent feature.

[divinity76]

@iluuu1994 seems in release builds, ZEND_ASSERT is just an alias for ZEND_ASSUME, and for debug builds, the issue doesn't reproduce..

And when hotpatching ZEND_ASSERT to actually assert in release builds, the issue goes away! even on release, replacing

#if ZEND_DEBUG
# define ZEND_ASSERT(c)	assert(c)
#else
# define ZEND_ASSERT(c)	assert(c)
#endif

with just

# define ZEND_ASSERT(c)	assert(c)

make the segfault go away

[iluuu1994]

@divinity76 It's possible this is a GCC bug for __attribute__((assume())) that is fixed in 13.2.1. As @devnexen said, the feature is brand new (C++23). It seems unlikely that an Ubuntu-specific patch broke this, but who knows.

[divinity76]

yeah seems likely, also seems gcc 13.2.1 isn't even released yet, it's a dev-version of gcc

how about we replace

#elif defined(__GNUC__) && !defined(__clang__) && __GNUC__ >= 13

with

#elif defined(__GNUC__) && !defined(__clang__) && (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) >= 130201

? (this method of version checking is documented in https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html )

[iluuu1994]

@divinity76 David mentioned that the issue is reproducible with address sanitizer in debug builds, which is why I suggested converting the remaining assumes to asserts. I was hoping this would reveal some invalid assertion. Can you try that, with address sanitizer?

Alternatively I can try it in a docker container tomorrow.

yeah seems likely, also seems gcc 13.2.1 isn't even released yet, it's a dev-version of gcc

Hmm, not sure why Fedora ships with 13.2.1 then.

[divinity76]

Can you try that, with address sanitizer?

No, i don't know how to use it, and don't want to study it today. Hope someone else can do it 👍

Hmm, not sure why Fedora ships with 13.2.1 then.

I guess a combo of Fedora being bleeding-edge and multiple Fedora developers also being gcc developers

[iluuu1994]

No, i don't know how to use it, and don't want to study it today. Hope someone else can do it 👍

Fair enough. I'll try it tomorrow.

[divinity76]

@iluuu1994

Alternatively I can try it in a docker container tomorrow.

fwiw if testing it through a Docker container is troublesome, the instructions for building gcc13.2.0 from source is roughly (or with a bit of luck, exactly)

mkdir /tmp/gcc;
cd /tmp/gcc;
wget 'https://gcc.gnu.org/pub/gcc/releases/gcc-13.2.0/gcc-13.2.0.tar.xz' -O- | tar xfvJ -;
cd gcc-13.2.0;
./contrib/download_prerequisites;
./configure --disable-multilib;
make -j$(nproc);
mkdir installdir;
make install DESTDIR=/tmp/gcc/gcc-13.2.0/installdir;
export PATH=/tmp/gcc/gcc-13.2.0/installdir/usr/local/bin:$PATH;
$ type gcc
gcc is /tmp/gcc/gcc-13.2.0/installdir/usr/local/bin/gcc
$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/tmp/gcc/gcc-13.2.0/installdir/usr/local/bin/../libexec/gcc/x86_64-pc-linux-gnu/13.2.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ./configure --disable-multilib
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 13.2.0 (GCC)

[iluuu1994]

@divinity76 It's not too bad, I just haven't gotten around to it yet. However, building GCC locally may be easier. Thank you!

[iluuu1994]

It turns out I can reproduce this on 13.2.1 after all. I'll revert the commit tomorrow, and see if I can identify the assumption that's responsible, and create a reproducer to submit upstream.

[divinity76]

@iluuu1994 GLHF, I suspect it won't be easy 🥲

[divinity76]

I suggest we revert bf4ec8b from master for the duration of the investigation, and perhaps re-introduce it when it's ready. having master segfault on run-tests is a burden

(for example, I thought it was something I did when I got segfaults testing an unrelated pull request #13194 )

[iluuu1994]

Reverted here: 2f89438

[divinity76]

@iluuu1994 just curious, any luck tracking down the exact assume responsible? How about minimal reproducible sample?

[iluuu1994]

Did not get to it yet.