PHP 7.3 Compatibility

[Jan-E]

Streamlined PR, in stead of https://github.com/phpv8/v8js/pull/363

[Jan-E]

All tests in Appveyor passed: https://ci.appveyor.com/project/stesie/v8js/build/1.0.263 @stesie Feel free to merge

[stesie]

Hey @Jan-E, thanks for the effort you've put into this. I've briefly tried it out, first on my NixOS box ... and then on Ubuntu Xenial to confirm, ... there seems to be a memory issue in the code path handling V8 extensions, as those two tests fail:

Test V8::registerExtension() : Basic extension registering [tests/extensions_basic.phpt]
Test V8::registerExtension() : Circular dependencies [tests/extensions_circular_dependency.phpt]

... they don't directly "fail" but crash the interpreter on shutdown. I'm unsure if this also happens on Windows yet unnoticed by the test runner since the output is actually correct

I'll dig deeper soon, maybe tonight ... likely first build with address sanitizer or so, since the current backtraces that just crash in _efree aren't helpful

[Jan-E]

I ran directly php /php-sdk/php73dev/ext/v8js/tests/extensions_basic.php, where extensions_basic.php is

<?php

V8Js::registerExtension('a', 'print("world!\n");', array('b'));
V8Js::registerExtension('b', 'print("Hello ");');

var_dump(V8JS::getExtensions());

$a = new V8Js('myobj', array(), array('a'));
?>
===EOF===

and php.ini is

[PHP] 
extension_dir = "ext" 
extension=v8js 

Then I also experienced a Unhandled exception at 0x000000007769F6B2 (ntdll.dll) in php.exe: 0xC0000374: A heap has been corrupted (parameters: 0x0000000077707C70). occurred at closing down. The strange thing is that the VS2017 debugger does not even mention php_v8js.dll:

[Jan-E]

The output window in the debugger only mentions v8_libbase.dll, which is V8-6.8.275.3 on my test system:

'php.exe' (Win32): Loaded 'N:\php-sdk\php73dev\x64\Release\php-7.3.0alpha3\php.exe'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\ntdll.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\kernel32.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\KernelBase.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'N:\php-sdk\php73dev\x64\Release\php-7.3.0alpha3\php7.dll'. Cannot find or open the PDB file.
'php.exe' (Win32): Loaded 'C:\Windows\System32\ws2_32.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\msvcrt.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\rpcrt4.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\nsi.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\IPHLPAPI.DLL'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\winnsi.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\ole32.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\gdi32.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\user32.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\lpk.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\usp10.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\advapi32.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\sechost.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\dnsapi.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\bcrypt.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\vcruntime140.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-crt-runtime-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\ucrtbase.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-core-timezone-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-core-file-l2-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-core-localization-l1-2-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-core-synch-l1-2-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-core-processthreads-l1-1-1.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-core-file-l1-2-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-crt-string-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-crt-heap-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-crt-stdio-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-crt-convert-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-crt-environment-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-crt-math-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-crt-locale-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-crt-filesystem-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-crt-time-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-crt-utility-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-crt-conio-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\shell32.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\shlwapi.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\imm32.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\msctf.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\bcryptprimitives.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'N:\php-sdk\php73dev\x64\Release\php-7.3.0alpha3\v8_libbase.dll'. Cannot find or open the PDB file.
'php.exe' (Win32): Loaded 'C:\Windows\System32\winmm.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\dbghelp.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\msvcp140.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\api-ms-win-crt-multibyte-l1-1-0.dll'. Symbols loaded.
'php.exe' (Win32): Loaded 'C:\Windows\System32\cryptbase.dll'. Symbols loaded.
The thread 0x1240 has exited with code 0 (0x0).
Unhandled exception at 0x000000007769F6B2 (ntdll.dll) in php.exe: 0xC0000374: A heap has been corrupted (parameters: 0x0000000077707C70).

[Jan-E]

With the same extension sources and the same V8-dll's the crash does not happen on PHP 7.2.8 RC1. So it really is PHP 7.3 specific. @cmb69 Any idea?

[Jan-E]

Or @remicollet ?

[Jan-E]

Simplest test script.

php.ini

[PHP] 
extension_dir = "ext" 
extension=v8js 

basic_test.php

<?php
V8Js::registerExtension('a', 'b', array('c'));
?>
===EOF===

Run: php basic_test.php

And fwiw: recompiling with V8-5.8.283.31 did not make any difference

[Jan-E]

Compiled distro here: https://phpdev.toolsforresearch.com/php-7.3.0alpha3-nts-Win32-VC15-x64.zip

Phpinfo dump: https://phpdev.toolsforresearch.com/php-7.3.0alpha3-nts-Win32-VC15-x64.htm

[Jan-E]

On July 4 & 5 @dstogov did a lot of work on zval_dstor and zval_ptr_dstor: https://github.com/php/php-src/commits/master?after=69a49af0d320be6d62028bd6a6996b49d4f200bc+34

See for instance 'Use zval_ptr_dtor() imstead of zval_dtor()': https://github.com/php/php-src/commit/169d4545931a43df258608c6cfb59b3e4bb92776

zval_dtor is used in v8js_array_access_dispatch: https://github.com/phpv8/v8js/blob/php7/v8js_array_access.cc#L56

Could this be related?

[Jan-E]

See https://github.com/m6w6/ext-http/issues/78#issuecomment-400352296 as well.

And the commit https://github.com/m6w6/ext-http/commit/377d576a6e68def5708cf1ffcd3c233c4dddf356 fix shutdown crash with 7.3/master: replace zval_ptr_dtor with zval_internal_dtor

Mayve @dstogov is trying to fix that in master

[stesie]

root@ubuntu-xenial:/opt/php-7.3.0alpha3# ./sapi/cli/php  -a
Interactive mode enabled

php > V8Js::registerExtension('a', 'b', array('c'));
php > =================================================================
==30037==ERROR: AddressSanitizer: heap-use-after-free on address 0x60300000dae4 at pc 0x000000ab767f bp 0x7ffd808885e0 sp 0x7ffd808885d0
READ of size 4 at 0x60300000dae4 thread T0                                                                                                                                                   
    #0 0xab767e in _str_dtor (/opt/php-7.3.0alpha3/sapi/cli/php+0xab767e)
    #1 0xa3e548 in zend_hash_destroy (/opt/php-7.3.0alpha3/sapi/cli/php+0xa3e548)
    #2 0xab8052 in zend_interned_strings_dtor (/opt/php-7.3.0alpha3/sapi/cli/php+0xab8052)
    #3 0x8593f6 in php_module_shutdown (/opt/php-7.3.0alpha3/sapi/cli/php+0x8593f6)
    #4 0xe272a1 in main (/opt/php-7.3.0alpha3/sapi/cli/php+0xe272a1)
    #5 0x7f555401c82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #6 0x4266f8 in _start (/opt/php-7.3.0alpha3/sapi/cli/php+0x4266f8)

0x60300000dae4 is located 4 bytes inside of 32-byte region [0x60300000dae0,0x60300000db00)
freed by thread T0 here:                                                                                                                                                                     
    #0 0x7f55564602ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)
    #1 0xa3e548 in zend_hash_destroy (/opt/php-7.3.0alpha3/sapi/cli/php+0xa3e548)
    #2 0x842101 in v8js_jsext_free_storage /opt/php-7.3.0alpha3/ext/v8js/v8js_class.cc:269
    #3 0x846ac0 in zm_shutdown_v8js /opt/php-7.3.0alpha3/ext/v8js/v8js_main.cc:176
    #4 0x9e5552 in module_destructor_zval (/opt/php-7.3.0alpha3/sapi/cli/php+0x9e5552)
    #5 0xa43a52 in zend_hash_graceful_reverse_destroy (/opt/php-7.3.0alpha3/sapi/cli/php+0xa43a52)
    #6 0xa03a39 in zend_destroy_modules (/opt/php-7.3.0alpha3/sapi/cli/php+0xa03a39)
    #7 0x9e601f in zend_shutdown (/opt/php-7.3.0alpha3/sapi/cli/php+0x9e601f)
    #8 0x859383 in php_module_shutdown (/opt/php-7.3.0alpha3/sapi/cli/php+0x859383)
    #9 0xe272a1 in main (/opt/php-7.3.0alpha3/sapi/cli/php+0xe272a1)
    #10 0x7f555401c82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

previously allocated by thread T0 here:
    #0 0x7f5556460602 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x98602)
    #1 0x9397da in __zend_malloc (/opt/php-7.3.0alpha3/sapi/cli/php+0x9397da)
    #2 0xab7a53 in zend_interned_strings_init (/opt/php-7.3.0alpha3/sapi/cli/php+0xab7a53)
    #3 0x9e5f33 in zend_startup (/opt/php-7.3.0alpha3/sapi/cli/php+0x9e5f33)
    #4 0x8582c9 in php_module_startup (/opt/php-7.3.0alpha3/sapi/cli/php+0x8582c9)
    #5 0xe22e28 in php_cli_startup (/opt/php-7.3.0alpha3/sapi/cli/php+0xe22e28)
    #6 0xe2711e in main (/opt/php-7.3.0alpha3/sapi/cli/php+0xe2711e)
    #7 0x7f555401c82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

SUMMARY: AddressSanitizer: heap-use-after-free ??:0 _str_dtor

[stesie]

accidentally tripped by a typo:

root@ubuntu-xenial:/opt/php-7.3.0alpha3# ./sapi/cli/php  -a
Interactive mode enabled

php > V8Js::registerExtension('a', 'b', array('' + 'c'));

Warning: A non-numeric value encountered in php shell code on line 1

Warning: A non-numeric value encountered in php shell code on line 1

Warning: V8Js::registerExtension(): Invalid dependency array passed in php shell code on line 1
ASAN:SIGSEGV
=================================================================
==30038==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000004 (pc 0x00000084213d bp 0x60300000db40 sp 0x7ffcc780b170 T0)
    #0 0x84213c in v8js_jsext_free_storage /opt/php-7.3.0alpha3/ext/v8js/v8js_class.cc:277                                                                                                   
    #1 0x8424cd in v8js_register_extension /opt/php-7.3.0alpha3/ext/v8js/v8js_class.cc:1039
    #2 0x8424cd in zim_V8Js_registerExtension /opt/php-7.3.0alpha3/ext/v8js/v8js_class.cc:1100
    #3 0xde3919 in execute_ex (/opt/php-7.3.0alpha3/sapi/cli/php+0xde3919)
    #4 0xe1da4a in zend_execute (/opt/php-7.3.0alpha3/sapi/cli/php+0xe1da4a)
    #5 0x99c460 in zend_eval_stringl (/opt/php-7.3.0alpha3/sapi/cli/php+0x99c460)
    #6 0x620324 in readline_shell_run /opt/php-7.3.0alpha3/ext/readline/readline_cli.c:681
    #7 0xe250f1 in do_cli (/opt/php-7.3.0alpha3/sapi/cli/php+0xe250f1)
    #8 0xe2720c in main (/opt/php-7.3.0alpha3/sapi/cli/php+0xe2720c)
    #9 0x7fc3e28d782f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #10 0x4266f8 in _start (/opt/php-7.3.0alpha3/sapi/cli/php+0x4266f8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /opt/php-7.3.0alpha3/ext/v8js/v8js_class.cc:277 v8js_jsext_free_storage
==30038==ABORTING

... but also shouldn't happen :D

[Jan-E]

0x60300000dae4 is located 4 bytes inside of 32-byte region [0x60300000dae0,0x60300000db00) freed by thread T0 here:

I have seen something like that before: https://github.com/php/pecl-mail-mailparse/pull/5#issuecomment-404110915

==26680== Address 0x10a90d64 is 4 bytes inside a block of size 40 free'd

@remicollet @cmb69 Could this be the same problem?

Something wrong with zend_hash_destroy, like it apparently was here as well: https://github.com/Microsoft/msphpsql/pull/810#issuecomment-404816127

[stesie]

@Jan-E nah, I don't think so.

The point here is that v8js uses interned strings to track loaded extensions (i.e. strings that aren't bound to request cycle, but live until interpreter exit) and puts those into a hash table. PHP 7.2 didn't free those manually constructed interned strings itself, ... with PHP 7.3 that changed, i.e. PHP 7.3 now frees them itself. Hence if this extension continues to free them there's a double free on shutdown

[Jan-E]

The point here is that v8js uses interned strings to track loaded extensions (i.e. strings that aren't bound to request cycle, but live until interpreter exit) and puts those into a hash table. PHP 7.2 didn't free those manually constructed interned strings itself, ... with PHP 7.3 that changed, i.e. PHP 7.3 now frees them itself.

I do not see this change documented in https://github.com/php/php-src/blob/master/UPGRADING.INTERNALS

@cmb69 Shouldn't this be there?

[cmb69]

It seems to me that no interned strings are used as of efad52d739720e88107e934f8874125d0fbbee62, but rather some persistent strings. The latter are not touched by the Zend engine, as far as I know.

[Jan-E]

You might be right on the persistent strings. Still, this commit seems to have solved the heap corruption: https://github.com/phpv8/v8js/commit/aa041b4597f12ff15547bb85aae2c90aaa50d3ae

So there must be a difference between 7.2 and 7.3.

[stesie]

Yeah, this wasn't the right fix. I've reverted it and tried a different route.

When freeing we must make sure the string isn't interned (and simple leave it alone then). If the string however is not interned, but a normal persisten one, then we need to free that. V8Js::registerExtension('a', 'b', array(\sprintf('%s', 'c'))); is a good example for that, since the result of the \sprintf cannot be interned, ... without the free that would leak that memory