Skip to content
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.

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

PHP 8.2 Segmentation fault on nesbot/carbon #11455

Closed
zeriyoshi opened this issue Jun 15, 2023 · 11 comments
Closed

PHP 8.2 Segmentation fault on nesbot/carbon #11455

zeriyoshi opened this issue Jun 15, 2023 · 11 comments

Comments

@zeriyoshi
Copy link
Contributor

zeriyoshi commented Jun 15, 2023

Description

There is a bug in nesbot/carbon that causes a segmentation violation when serialize / unserialize is executed in PHP 8.2 and later.

In response to this bug, nesbot/carbon has a workaround that implements a userland serialization algorithm, which significantly worse performance. (briannesbitt/Carbon#2644)

This can be reproduced by following these steps:

$ git clone --branch=php82_segv --depth=1 "https://github.com/zeriyoshi/Carbon.git" "Carbon"
$ cd "Carbon"
$ composer install
$ composer exec -- phpunit tests/Carbon/SerializationTest.php --debug --filter testSerialization
PHPUnit 9.6.9 by Sebastian Bergmann and contributors.

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

Test 'Tests\Carbon\SerializationTest::testSerialization' started
Segmentation fault
Script phpunit handling the __exec_command event returned with error code 139

backtrace (gdb):

0x0000aaaac274195c in zend_mm_alloc_small (heap=0xffff93a00040, bin_num=9, __zend_filename=0xaaaac31d94e0 "/usr/src/php/Zend/zend_string.h", __zend_lineno=152, __zend_orig_filename=0x0, __zend_orig_lineno=0) at /usr/src/php/Zend/zend_alloc.c:1313
1313			heap->free_slot[bin_num] = p->next_free_slot;
(gdb) backtrace
#0  0x0000aaaac274195c in zend_mm_alloc_small (heap=0xffff93a00040, bin_num=9, __zend_filename=0xaaaac31d94e0 "/usr/src/php/Zend/zend_string.h", __zend_lineno=152, __zend_orig_filename=0x0, __zend_orig_lineno=0) at /usr/src/php/Zend/zend_alloc.c:1313
#1  0x0000aaaac2741c38 in zend_mm_alloc_heap (heap=0xffff93a00040, size=88, __zend_filename=0xaaaac31d94e0 "/usr/src/php/Zend/zend_string.h", __zend_lineno=152, __zend_orig_filename=0x0, __zend_orig_lineno=0) at /usr/src/php/Zend/zend_alloc.c:1384
#2  0x0000aaaac2744ae4 in _emalloc (size=56, __zend_filename=0xaaaac31d94e0 "/usr/src/php/Zend/zend_string.h", __zend_lineno=152, __zend_orig_filename=0x0, __zend_orig_lineno=0) at /usr/src/php/Zend/zend_alloc.c:2594
#3  0x0000aaaac28775c4 in zend_string_alloc (len=29, persistent=false) at /usr/src/php/Zend/zend_string.h:152
#4  0x0000aaaac2877634 in zend_string_init (str=0xffff92e4dcd8 "Symfony\\Component\\Translation", len=29, persistent=false) at /usr/src/php/Zend/zend_string.h:174
#5  0x0000aaaac2878244 in zend_new_interned_string_request (str=0xffff92e4dcc0) at /usr/src/php/Zend/zend_string.c:249
#6  0x0000aaaac275e75c in zend_compile_use (ast=0xffff92edd108) at /usr/src/php/Zend/zend_compile.c:8172
#7  0x0000aaaac2764528 in zend_compile_stmt (ast=0xffff92edd108) at /usr/src/php/Zend/zend_compile.c:10174
#8  0x0000aaaac2764168 in zend_compile_top_stmt (ast=0xffff92edd108) at /usr/src/php/Zend/zend_compile.c:10078
#9  0x0000aaaac2764094 in zend_compile_top_stmt (ast=0xffff92edd390) at /usr/src/php/Zend/zend_compile.c:10064
#10 0x0000aaaac2711f80 in zend_compile (type=2) at /usr/src/php/Zend/zend_language_scanner.c:620
#11 0x0000aaaac27120ac in compile_file (file_handle=0xffffdd80d960, type=2) at /usr/src/php/Zend/zend_language_scanner.c:655
#12 0x0000aaaac2493ab0 in phar_compile_file (file_handle=0xffffdd80d960, type=2) at /usr/src/php/ext/phar/phar.c:3355
#13 0x0000aaaac2712258 in compile_filename (type=2, filename=0xffff92e57480) at /usr/src/php/Zend/zend_language_scanner.c:706
#14 0x0000aaaac27c6c04 in zend_include_or_eval (inc_filename_zv=0xffff93a1cdb0, type=2) at /usr/src/php/Zend/zend_execute.c:4799
#15 0x0000aaaac282d7c8 in ZEND_INCLUDE_OR_EVAL_SPEC_CV_HANDLER () at /usr/src/php/Zend/zend_vm_execute.h:38948
#16 0x0000aaaac2854fd4 in execute_ex (ex=0xffff93a1cc90) at /usr/src/php/Zend/zend_vm_execute.h:59395
#17 0x0000aaaac276caf4 in zend_call_function (fci=0xffffdd80df18, fci_cache=0xffffdd80def8) at /usr/src/php/Zend/zend_execute_API.c:947
#18 0x0000aaaac276cfdc in zend_call_known_function (fn=0xffff93a08b78, object=0xffff93a78400, called_scope=0xffff93a07440, retval_ptr=0x0, param_count=1, params=0xffffdd80dfa0, named_params=0x0) at /usr/src/php/Zend/zend_execute_API.c:1041
#19 0x0000aaaac25040c0 in spl_perform_autoload (class_name=0xffff9344b230, lc_name=0xffff9344baa0) at /usr/src/php/ext/spl/php_spl.c:445
#20 0x0000aaaac276d78c in zend_lookup_class_ex (name=0xffff9344b230, key=0xffff9344baa0, flags=512) at /usr/src/php/Zend/zend_execute_API.c:1207
#21 0x0000aaaac276e4f0 in zend_fetch_class_by_name (class_name=0xffff9344b230, key=0xffff9344baa0, fetch_type=512) at /usr/src/php/Zend/zend_execute_API.c:1703
#22 0x0000aaaac27e36ec in ZEND_NEW_SPEC_CONST_UNUSED_HANDLER () at /usr/src/php/Zend/zend_vm_execute.h:10271
#23 0x0000aaaac28528fc in execute_ex (ex=0xffff93a17020) at /usr/src/php/Zend/zend_vm_execute.h:56937
#24 0x0000aaaac2855fd4 in zend_execute (op_array=0xffff93a85000, return_value=0x0) at /usr/src/php/Zend/zend_vm_execute.h:60396
#25 0x0000aaaac27868ac in zend_execute_scripts (type=8, retval=0x0, file_count=3) at /usr/src/php/Zend/zend.c:1827
#26 0x0000aaaac26d3bcc in php_execute_script (primary_file=0xffffdd8109f0) at /usr/src/php/main/main.c:2542
#27 0x0000aaaac291ccfc in do_cli (argc=5, argv=0xaaaaecc05270) at /usr/src/php/sapi/cli/php_cli.c:964
#28 0x0000aaaac291d950 in main (argc=5, argv=0xaaaaecc05270) at /usr/src/php/sapi/cli/php_cli.c:1333

zbacktrace (gdb)

[0xffff93a1cd60] Composer\Autoload\{closure}("/root/Carbon/vendor/composer/../../src/Carbon/Translator.php") /root/Carbon/vendor/composer/ClassLoader.php:576
[0xffff93a1cc90] Composer\Autoload\ClassLoader->loadClass("Carbon\Translator") /root/Carbon/vendor/composer/ClassLoader.php:427
[0xffff93a1ca60] Tests\AbstractTestCase->tearDown() /root/Carbon/tests/AbstractTestCase.php:99
[0xffff93a1c340] PHPUnit\Framework\TestCase->runBare() /root/Carbon/vendor/phpunit/phpunit/src/Framework/TestCase.php:1260
[0xffff93a1b140] PHPUnit\Framework\TestResult->run(object[0xffff93a1b190]) /root/Carbon/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
[0xffff93a1a530] PHPUnit\Framework\TestCase->run(object[0xffff93a1a580]) /root/Carbon/vendor/phpunit/phpunit/src/Framework/TestCase.php:964
[0xffff93a19e30] PHPUnit\Framework\TestSuite->run(object[0xffff93a19e80]) /root/Carbon/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
[0xffff93a17a00] PHPUnit\TextUI\TestRunner->run(object[0xffff93a17a50], reference, reference, true) /root/Carbon/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
[0xffff93a17630] PHPUnit\TextUI\Command->run(array(4)[0xffff93a17680], true) /root/Carbon/vendor/phpunit/phpunit/src/TextUI/Command.php:144
[0xffff93a17520] PHPUnit\TextUI\Command->main() /root/Carbon/vendor/phpunit/phpunit/src/TextUI/Command.php:97
[0xffff93a171e0] (main) /root/Carbon/vendor/phpunit/phpunit/phpunit:107
[0xffff93a17020] (main) /root/Carbon/vendor/bin/phpunit:122

PHP Version

PHP 8.2.7
PHP 8.2.8-dev
PHP 8.3.0-dev master 96ea06a

Operating System

Debian 11.7

@zeriyoshi
Copy link
Contributor Author

The problem was solved by importing the ext/date of the PHP-8.1 branch into the PHP-8.2 branch. Perhaps this is an ext/date issue. I will investigate further.

@zeriyoshi
Copy link
Contributor Author

  • PHP 8.2.3 OK (but Test failed)
  • PHP 8.2.4 Segmentation Fault

Segmentation violation appears to have occurred after this commit: 85fbc6e

@zeriyoshi
Copy link
Contributor Author

I am trying to come up with the smallest code that will reproduce the problem, but it is difficult.
Hopefully the above information will shed some light...
@derickr @kylekatarnls

@zeriyoshi
Copy link
Contributor Author

memo: lldb backtrace

(lldb) r vendor/bin/phpunit tests/Carbon/SerializationTest.php --debug --filter testSerialization
Process 96301 launched: '/Users/g-kudo/Development/php-src/sapi/cli/php' (arm64)
PHPUnit 9.6.9 by Sebastian Bergmann and contributors.

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

Test 'Tests\Carbon\SerializationTest::testSerialization' started
Assertion failed: (zval_gc_type((ref)->gc.u.type_info) == 7 || zval_gc_type((ref)->gc.u.type_info) == 8), function gc_possible_root, file zend_gc.c, line 647.
Process 96301 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert
    frame #4: 0x00000001006501c8 php`gc_possible_root(ref=0x00000001033f6ea0) at zend_gc.c:647:2
   644                  return;
   645          }
   646 
-> 647          ZEND_ASSERT(GC_TYPE(ref) == IS_ARRAY || GC_TYPE(ref) == IS_OBJECT);
   648          ZEND_ASSERT(GC_INFO(ref) == 0);
   649 
   650          newRoot = GC_IDX2PTR(idx);
Target 0: (php) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert
    frame #0: 0x000000019c150724 libsystem_kernel.dylib`__pthread_kill + 8
    frame #1: 0x000000019c187c28 libsystem_pthread.dylib`pthread_kill + 288
    frame #2: 0x000000019c095ae8 libsystem_c.dylib`abort + 180
    frame #3: 0x000000019c094e44 libsystem_c.dylib`__assert_rtn + 272
  * frame #4: 0x00000001006501c8 php`gc_possible_root(ref=0x00000001033f6ea0) at zend_gc.c:647:2
    frame #5: 0x0000000100673854 php`gc_check_possible_root(ref=0x00000001033f6ea0) at zend_gc.h:88:3
    frame #6: 0x0000000100672914 php`i_zval_ptr_dtor(zval_ptr=0x0000000103098450) at zend_variables.h:46:4
    frame #7: 0x00000001006725ac php`zend_object_std_dtor(object=0x0000000103098408) at zend_objects.c:71:5
    frame #8: 0x0000000100020aac php`date_object_free_storage_date(object=0x0000000103098408) at php_date.c:2282:2
    frame #9: 0x000000010067d050 php`zend_objects_store_del(object=0x0000000103098408) at zend_objects_API.c:200:4
    frame #10: 0x0000000100529828 php`rc_dtor_func(p=0x0000000103098408) at zend_variables.c:57:2
    frame #11: 0x000000010056e174 php`zval_ptr_dtor_nogc(zval_ptr=0x000000010221d280) at zend_variables.h:35:3
    frame #12: 0x0000000100628920 php`zend_vm_stack_free_args(call=0x000000010221d230) at zend_execute.h:324:4
    frame #13: 0x00000001005f5678 php`ZEND_DO_FCALL_BY_NAME_SPEC_RETVAL_USED_HANDLER(execute_data=0x000000010221cf90) at zend_vm_execute.h:1680:3
    frame #14: 0x00000001005727f0 php`execute_ex(ex=0x0000000102217020) at zend_vm_execute.h:56856:7
    frame #15: 0x0000000100572bec php`zend_execute(op_array=0x000000010228a000, return_value=0x0000000000000000) at zend_vm_execute.h:61445:2
    frame #16: 0x000000010052dde4 php`zend_execute_scripts(type=8, retval=0x0000000000000000, file_count=3) at zend.c:1862:4
    frame #17: 0x000000010045d6ec php`php_execute_script(primary_file=0x000000016fdfe830) at main.c:2482:13
    frame #18: 0x000000010071b2f4 php`do_cli(argc=6, argv=0x0000000102007670) at php_cli.c:959:5
    frame #19: 0x000000010071a430 php`main(argc=6, argv=0x0000000102007670) at php_cli.c:1328:18
    frame #20: 0x000000019be2ff28 dyld`start + 2236

@nielsdos
Copy link
Member

nielsdos commented Jun 17, 2023

@zeriyoshi
Looks like a refcounting bug in add_common_properties. I modified one of the ext/date testcases a bit to reproduce this issue:

<?php
class MyDateTimeImmutable extends DateTimeImmutable {
    public function __construct(
        string $datetime = "now",
        ?DateTimeZone $timezone = null,
        public ?stdClass $myProperty = null,
    ) {
        parent::__construct($datetime, $timezone);
    }
}

$datetime = new MyDateTimeImmutable('2022-12-22T11:26:00Z', myProperty: new stdClass);
$datetime->myProperty->field = str_repeat("hello", 3);
$serialized = serialize($datetime);
var_dump($datetime->myProperty);
$unserialized = unserialize($serialized);

var_dump($unserialized);

This code causes a crash in malloc().

The fix is simple, it's a matter of adding the refcount if a new entry is added to the hash table.
EDIT: removed outdated patch, see PR.

Executing my testcase now works. I tried this with your Carbon fork, and it no longer crashes in malloc(), but the test fails. This is because Carbon tries to serialize a Closure stored in $localTranslator->$ordinal:

https://github.com/briannesbitt/Carbon/blob/cfb8701c35ade1c9c00cfd8a3a3fe029b5a75e77/src/Carbon/Lang/en.php#L71-L83

So although my patch fixes the underlying bug in this issue, I'm not so sure Carbon can just get rid of their custom serialization.
Please let me know if I missed something. If not I can PR this up.

@zeriyoshi
Copy link
Contributor Author

@nielsdos
That seems to make sense!

We have also identified an issue on the Carbon side that needs to be addressed by Carbon. As soon as this fix is incorporated into PHP 8.2-dev, I will suggest a fix to Carbon (though it may take some time, as I am not familiar with Carbon. A fix by the maintainer would be even better :) ).

@kylekatarnls
Copy link
Contributor

Indeed, on Carbon side, we use the custom serialization to dump/restore some other parameters, also we'll need to support the bugged versions for a while as it makes maintenance more difficult if few users are stuck on an old patch, but putting this out of the way for the next patch is already super useful.

@nielsdos
Copy link
Member

Great, thanks for checking. I opened a PR.

@zeriyoshi
Copy link
Contributor Author

@nielsdos
Thanks! Can you tell me how you identified the problem this time as I would like to help in the future?

@nielsdos
Copy link
Member

nielsdos commented Jun 19, 2023

@zeriyoshi
A crash in the Zend heap usually means a refcounting bug, an overflow or a double free.
There were no buffers in the commit you linked so I guessed it was probably a refcounting bug.
I then read the code of the commit and saw there was no refcounting when adding to the HashTable, but in general I expect to see refcounting there unless the zval will be uniquely owned or can't be an object/array/string. So I guessed that must be the problem and created a testcase to check that. I saw it crashed and then checked what Carbon was doing, and I saw Carbon crashed due the localTranslator serialization. I figured that out by just skipping over objects during serialization.
Then I made the patch and saw the problem went away.

@zeriyoshi
Copy link
Contributor Author

@nielsdos Thank you! It was very easy to understand. I will use it as a reference!

nielsdos added a commit that referenced this issue Jun 19, 2023
* PHP-8.2:
  Fix GH-11455: Segmentation fault with custom object date properties
  Revert "Fix GH-11404: DOMDocument::savexml and friends ommit xmlns="" declaration for null namespace, creating incorrect xml representation of the DOM"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants