NULL dereference when calling `ob_start()` in shutdown function triggered by bailout in `php_output_lock_error()`

[TimWolla]

Description

The following code:

<?php
register_shutdown_function(function () {
    ob_start(function () { });
});

ob_start(function () {
    ob_start();
}, 1);

echo "x";

Resulted in this output:

Fatal error: ob_start(): Cannot use output buffering in output buffering display handlers in php-src/test6.php on line 7
Stack trace:
#0 php-src/test6.php(7): ob_start()
#1 php-src/test6.php(10): {closure:php-src/test6.php:6}('x', 1)
#2 {main}
php-src/Zend/zend_stack.c:40:9: runtime error: applying zero offset to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior php-src/Zend/zend_stack.c:40:9 in 
php-src/Zend/zend_stack.c:40:9: runtime error: null pointer passed as argument 1, which is declared to never be null
/usr/include/string.h:44:28: note: nonnull attribute specified here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior php-src/Zend/zend_stack.c:40:9 in 
AddressSanitizer:DEADLYSIGNAL
=================================================================
==310221==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x72fb8a388ac8 bp 0x7ffd25d9cb90 sp 0x7ffd25d9c348 T0)
==310221==The signal is caused by a WRITE memory access.
==310221==Hint: address points to the zero page.
    #0 0x72fb8a388ac8 in __memcpy_avx_unaligned_erms string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:366
    #1 0x57dde089d0b7 in __asan_memcpy (php-src/sapi/cli/php+0x249d0b7) (BuildId: 1b299382c6628a85942c79b2c673788eab3e71d4)
    #2 0x57dde4ad2dad in zend_stack_push php-src/Zend/zend_stack.c:40:2
    #3 0x57dde36d309f in php_output_handler_start php-src/main/output.c:557:19
    #4 0x57dde36d35c1 in php_output_start_user php-src/main/output.c:441:17
    #5 0x57dde36d9265 in zif_ob_start php-src/main/output.c:1384:6
    #6 0x57dde449c249 in ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER php-src/Zend/zend_vm_execute.h:1355:2
    #7 0x57dde403708c in execute_ex php-src/Zend/zend_vm_execute.h:116212:12
    #8 0x57dde3fdd229 in zend_call_function php-src/Zend/zend_execute_API.c:1010:3
    #9 0x57dde3fe1ac7 in zend_call_known_function php-src/Zend/zend_execute_API.c:1104:23
    #10 0x57dde2f7cc9e in zend_call_known_fcc php-src/Zend/zend_API.h:852:2
    #11 0x57dde2f5821f in user_shutdown_function_call php-src/ext/standard/basic_functions.c:1590:2
    #12 0x57dde47968b2 in zend_hash_apply php-src/Zend/zend_hash.c:2083:13
    #13 0x57dde2f57ecf in php_call_shutdown_functions php-src/ext/standard/basic_functions.c:1628:4
    #14 0x57dde36a89b2 in php_request_shutdown php-src/main/main.c:1980:3
    #15 0x57dde4b5d2ed in do_cli php-src/sapi/cli/php_cli.c:1158:3
    #16 0x57dde4b54ca2 in main php-src/sapi/cli/php_cli.c:1362:18
    #17 0x72fb8a22a1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #18 0x72fb8a22a28a in __libc_start_main csu/../csu/libc-start.c:360:3
    #19 0x57dde0803464 in _start (php-src/sapi/cli/php+0x2403464) (BuildId: 1b299382c6628a85942c79b2c673788eab3e71d4)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:366 in __memcpy_avx_unaligned_erms
==310221==ABORTING

But I expected this output instead: Not a segmentation fault.

---

The real world case where I encountered the issue is Twig triggering “ob_get_clean(): Cannot use output buffering in output buffering display handlers” and then Symfony Error Handler trying to use Twig to render an error page.

Somewhat related to #20824.

PHP Version

git master

Operating System

No response

[TimWolla]

zend_stack_destroy() is internally inconsistent. It frees stack->elements and resets it to NULL, but it doesn't also reset stack->max = 0 (which then would reinitialize the stack on the next push).

Either the stack->elements pointer should be left dangling (which would make the issue a more obvious use-after-free) or stack->max should be reset as well to put the stack into a safe state.

[ndossche]

stack->max should be reset as well to put the stack into a safe state.

I agree with this solution. The API should be safe and internally consistent.

[TimWolla]

I agree with this solution. The API should be safe and internally consistent.

Looking into this more, I opted for a different fix for the issue at hand: Push should not be called on the destroyed stack. The changes for zend_stack_destroy() can happen in master then (possibly a ZEND_ASSERT(stack->elements != NULL || stack->max == 0) in push).