New Garbage Collector feature for WeakMap added in 8.3 #10932 causes GC buffer overflow which then causes segfault

[JacobBrownAustin]

Description

This seems to be caused by #10932
Because new code was added that adds the WeakMap's values to the Garbage Collector's root list, this causes the "GC buffer overflow" to happen. When this happen, it also seems to cause a use-after-free error in a custom error handler which causes a segmentation fault.

I have made custom patch that disables functionality in #10932 , and then our code works again just fine. For our code base, we don't even need the functionality added in #10932 because the values that we put into the WeakMap are not referencing the keys at all. I was thinking that this functionality should be disableable in php.ini because some people wouldn't want to use it (for performance reason), and also to avoid the buffer overflow that will crash program with potential segmentation faults.

This issue is currently preventing our code using WeakMap from supporting PHP 8.3.

Here are some examples.

1. Here's an example showing when the "GC buffer overflow (GC disabled)" "warning" is triggered which is what triggers the mess that follows. Unlike PHP 8.1 & 8.2, the zend_gc_collect_cycles() isn't able to cope with all of our values in WeakMap, and they cause the buffer to overflow, which later causes worse things.

Test 'Magento\GraphQl\App\GraphQlCheckoutMutationsStateTest::testCreateEmptyCart' started

Breakpoint 1, gc_grow_root_buffer () at /usr/src/php/Zend/zend_gc.c:585
585                             zend_error(E_WARNING, "GC buffer overflow (GC disabled)\n");
(gdb) bt
#0  gc_grow_root_buffer () at /usr/src/php/Zend/zend_gc.c:585
#1  0x000055f0c40c3374 in gc_extra_root (ref=0x7fde80116a80) at /usr/src/php/Zend/zend_gc.c:724
#2  0x000055f0c40c392c in gc_scan_black (ref=0x7fde8010be60, stack=0x7fde7cdcd000) at /usr/src/php/Zend/zend_gc.c:845
#3  0x000055f0c40c49c1 in gc_scan (ref=0x7fde82fbe840, stack=0x7ffe4ddc1780) at /usr/src/php/Zend/zend_gc.c:1227
#4  0x000055f0c40c4fe0 in gc_scan_roots (stack=0x7ffe4ddc1780) at /usr/src/php/Zend/zend_gc.c:1373
#5  0x000055f0c40c616d in zend_gc_collect_cycles () at /usr/src/php/Zend/zend_gc.c:1814
#6  0x000055f0c40c30b3 in gc_possible_root_when_full (ref=0x7fde7cdbad80) at /usr/src/php/Zend/zend_gc.c:644
#7  0x000055f0c40c3233 in gc_possible_root (ref=0x7fde7cdbad80) at /usr/src/php/Zend/zend_gc.c:694
#8  0x000055f0c40ad5d4 in execute_ex (ex=0x7fde8aa17020) at /usr/src/php/Zend/zend_vm_execute.h:60527
#9  0x000055f0c40ae729 in zend_execute (op_array=0x7fde8aa77840, return_value=0x0) at /usr/src/php/Zend/zend_vm_execute.h:61604
#10 0x000055f0c3fe498d in zend_execute_scripts (type=8, retval=0x0, file_count=3) at /usr/src/php/Zend/zend.c:1881
#11 0x000055f0c3f29ca2 in php_execute_script (primary_file=0x7ffe4ddc3fa0) at /usr/src/php/main/main.c:2507
#12 0x000055f0c4171c89 in do_cli (argc=7, argv=0x55f0c5e364e0) at /usr/src/php/sapi/cli/php_cli.c:966
#13 0x000055f0c4172ac5 in main (argc=7, argv=0x55f0c5e364e0) at /usr/src/php/sapi/cli/php_cli.c:1340
(gdb) p gc_globals
$1 = {buf = 0x7fdba34f3010, gc_enabled = true, gc_active = true, gc_protected = false, gc_full = false, unused = 0, first_unused = 20116, gc_threshold = 20001, buf_size = 1073741824,
  num_roots = 20115, gc_runs = 38, collected = 688111, activated_at = 11360300210371, collector_time = 601211798, dtor_time = 0, free_time = 44720929}
(gdb) select-frame 4
(gdb) info locals
idx = 1
end = 20001
current = 0x7fdba34f3018
(gdb) select-frame 3
(gdb) info locals
ht = 0x7fde82fb9ae0
p = 0x7fde7e8876a0
zv = 0x7fde7e887680
n = 0
_stack = 0x7fde7cdc3000
_top = 66
__PRETTY_FUNCTION__ = "gc_scan"
(gdb) select-frame 2
(gdb) info locals
key = 0x7fde7cdddc40
entry = 0x7fde7dc87220
table = 0x7fde7cdd2000
len = 3556
obj = 0x7fde843512b8
ht = 0x7fde82f92a20
p = 0x7fde82669de0
zv = 0x7fde7cdddc40
n = 544
_stack = 0x7fde7cdcd000
_top = 18
__PRETTY_FUNCTION__ = "gc_scan_black"
(gdb) select-frame 1
(gdb) info locals
idx = 20115
newRoot = 0x7fde7cdddc50
__PRETTY_FUNCTION__ = "gc_extra_root"
(gdb) select-frame 0
(gdb) info locals
new_size = 86394340900864

2. This example is from phpuit running integration test with debug compile of PHP 8.3.3 so it aborts instead of segfaults. This first example is showing what happens in PHPUnit's error handler when it tries to convert the Exception created in the custom error handler into a string. This isn't showing the bug itself, but the side affect that this bug is messing with the garbage collector so much that it causes an array in the Exception to be unable to be accessed. When this happens in PHP 8.3.3 that is not compiled with debug assertions, then it segfaults instead of aborts.

1) Magento\GraphQl\App\GraphQlCheckoutMutationsStateTest::testCreateEmptyCart
/usr/src/php/Zend/zend_hash.c(2681) : ht=0x7f7a3dc7c120 is already destroyed
php: /usr/src/php/Zend/zend_hash.c:74: _zend_is_inconsistent: Assertion `0' failed.

Thread 1 "php" received signal SIGABRT, Aborted.
0x00007f7a63203e2c in ?? () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) bt
#0  0x00007f7a63203e2c in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x00007f7a631b4fb2 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x00007f7a6319f472 in abort () from /lib/x86_64-linux-gnu/libc.so.6
#3  0x00007f7a6319f395 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#4  0x00007f7a631adeb2 in __assert_fail () from /lib/x86_64-linux-gnu/libc.so.6
#5  0x000055ee349fa980 in _zend_is_inconsistent (ht=0x7f7a3dc7c120, 
    file=0x55ee355880f7 "/usr/src/php/Zend/zend_hash.c", line=2681) at /usr/src/php/Zend/zend_hash.c:74
#6  0x000055ee34a039ff in zend_hash_find_known_hash (ht=0x7f7a3dc7c120, key=0x55ee37144b60)
    at /usr/src/php/Zend/zend_hash.c:2681
#7  0x000055ee34a16539 in zend_hash_find_ex (ht=0x7f7a3dc7c120, key=0x55ee37144b60, known_hash=true)
    at /usr/src/php/Zend/zend_hash.h:188
#8  0x000055ee34a934bb in ZEND_ISSET_ISEMPTY_DIM_OBJ_SPEC_CV_CONST_HANDLER ()
    at /usr/src/php/Zend/zend_vm_execute.h:44021
#9  0x000055ee34aadbd8 in execute_ex (ex=0x7f7a6021a400) at /usr/src/php/Zend/zend_vm_execute.h:60903
#10 0x000055ee349c89db in zend_call_function (fci=0x7ffe6058ac60, fci_cache=0x7ffe6058ac30)
    at /usr/src/php/Zend/zend_execute_API.c:957
#11 0x000055ee349c8f50 in zend_call_known_function (fn=0x7f7a495e37b8, object=0x7f7a3b069460, 
    called_scope=0x7f7a495e3490, retval_ptr=0x7ffe6058ad90, param_count=0, params=0x0, named_params=0x0)
    at /usr/src/php/Zend/zend_execute_API.c:1051
#12 0x000055ee34ae32af in zend_call_known_instance_method (fn=0x7f7a495e37b8, object=0x7f7a3b069460, 
    retval_ptr=0x7ffe6058ad90, param_count=0, params=0x0) at /usr/src/php/Zend/zend_API.h:853
#13 0x000055ee34ae32e9 in zend_call_known_instance_method_with_0_params (fn=0x7f7a495e37b8, object=0x7f7a3b069460, 
    retval_ptr=0x7ffe6058ad90) at /usr/src/php/Zend/zend_API.h:859
#14 0x000055ee34ae8fa5 in zend_std_cast_object_tostring (readobj=0x7f7a3b069460, writeobj=0x7ffe6058add0, type=6)
    at /usr/src/php/Zend/zend_object_handlers.c:1934
#15 0x000055ee349d3991 in __zval_get_string_func (op=0x7f7a6021a260, try=false)
    at /usr/src/php/Zend/zend_operators.c:1034
#16 0x000055ee349d3a79 in zval_get_string_func (op=0x7f7a6021a260) at /usr/src/php/Zend/zend_operators.c:1055
--Type <RET> for more, q to quit, c to continue without paging--c
#17 0x000055ee34a16e1b in zval_get_string (op=0x7f7a6021a260) at /usr/src/php/Zend/zend_operators.h:305                     
#18 0x000055ee34a87a79 in ZEND_CAST_SPEC_CV_HANDLER () at /usr/src/php/Zend/zend_vm_execute.h:39753                              
#19 0x000055ee34aad6f8 in execute_ex (ex=0x7f7a60217020) at /usr/src/php/Zend/zend_vm_execute.h:60591                                
#20 0x000055ee34aae729 in zend_execute (op_array=0x7f7a60277840, return_value=0x0) at /usr/src/php/Zend/zend_vm_execute.h:61604           
#21 0x000055ee349e498d in zend_execute_scripts (type=8, retval=0x0, file_count=3) at /usr/src/php/Zend/zend.c:1881                              
#22 0x000055ee34929ca2 in php_execute_script (primary_file=0x7ffe6058c6c0) at /usr/src/php/main/main.c:2507                                          
#23 0x000055ee34b71c89 in do_cli (argc=7, argv=0x55ee37137410) at /usr/src/php/sapi/cli/php_cli.c:966                                                
#24 0x000055ee34b72ac5 in main (argc=7, argv=0x55ee37137410) at /usr/src/php/sapi/cli/php_cli.c:1340                                                        
(gdb)          

PHP Version

PHP 8.3.3

Operating System

Official Docker image, "php:8.3-cli" (currently 8.3.3)

[arnaud-lb]

Thank you for this bug report!

Based on your GDB session we can figure that we are growing the buffer unnecessarily, as there are only 20K roots in it when gc_grow_root_buffer() triggers the GC buffer overflow (GC disabled) warning. I believe that #13572 fixes the root cause of this issue. Could you confirm that it fixes the issue for you?

The second issue is a side effect of triggering the GC buffer overflow (GC disabled) warning: We execute PHP code during GC because of the error handler, which corrupts the GC state. This is a different issue that should be fixed separately.

[JacobBrownAustin]

Wow! Thanks so much for the quick response and fix! I will see if I can make time to test it today (probably tonight) as getting our application working in PHP 8.3 is a high priority for us.

I see that you were able to make a test for it. Was it able to reproduce the issue?

I was suspecting that there were two issues because the segfault was really confusing to me, but I was able to reproduce it consistently with the custom error handle. To me, it didn't look safe at all to run custom error handler in the middle of a garbage collector function.

[JacobBrownAustin]

To me, it didn't look safe at all to run custom error handler in the middle of a garbage collector function.

When I was thinking about it yesterday, I would have just disabled the ability to call custom error handling at all when entering garbage collector, then reenable it once garbage collector is finished. Maybe you have a better idea that would work.

[arnaud-lb]

I agree, or alternatively we should delay the error until the GC ends. In general I think we should delay errors handlers until a safe point is reached, like we do for timeouts and signals, but that's an other story.

I see that you were able to make a test for it. Was it able to reproduce the issue?

Yes it was

[JacobBrownAustin]

I just did a quick test during lunch and that fix does pass our integration tests.
I still need to undo all of the changes I made to my Docker image that adds debug symbols and such, so I'm going to try rebuilding it again later tonight so that the PHP build is just like the official Docker image, but with that one fix.

[JacobBrownAustin]

I've done some more testing with not-debug builds, and the test passes just fine with your fix, #13572 .
Thank you! Do you know what version of PHP we could expect this fix?

[arnaud-lb]

Thank you! Depending review time, this could be fixed in the next 8.3