Repository navigation
Hash table use-after-destroy in function table during stream wrapper cleanup (Zend/zend_hash.c) #20286
Description
Description
The following code:
<?php
class lib {
function stream_open() {
return true;
}
function stream_read($count) {
function a() {}
include('lib://');
}
function stream_close() {
include('lib://');
}
}
stream_wrapper_register('lib', lib::class);
include('lib://test.php');Resulted in this output:
/path/to/php-src/Zend/zend_hash.c(2681) : ht=0x7fc36de8e048 is already destroyed
php: /path/to/php-src/Zend/zend_hash.c:73: void _zend_is_inconsistent(const HashTable *, const char *, int): Assertion `0' failed.
Aborted
Commit:
7455f6a5b0f
Configuration:
CC="clang" CXX="clang++" CFLAGS="-fsanitize=address -g -O0" CXXFLAGS="-fsanitize=address -g -O0" ./configure --enable-debug --enable-address-sanitizer --disable-shared --with-pic
PHP Version
PHP 8.6.0-dev (cli) (built: Oct 25 2025 17:19:26) (NTS DEBUG)
Copyright (c) The PHP Group
Zend Engine v4.6.0-dev, Copyright (c) Zend Technologies
with Zend OPcache v8.6.0-dev, Copyright (c), by Zend Technologies
Operating System
Ubuntu 22.04
Activity
devnexen commentedon Oct 25, 2025
ndossche commentedon Oct 25, 2025
Ouch! This code causes the stream_close call to execute so late that
EG(active)is false already! This should be disallowed.zend_call_function& friends prevent this by checkingEG(active), butzend_call_method_if_existsdoesn't.This issue should only be triggerable via resources, so perhaps just fixing this in
php_userstreamop_closeinstead of adding this extra bloat to the Zend API is enough.devnexen commentedon Oct 27, 2025
would something like this works ?
let me know if you prefer to work on it yourself though.
ndossche commentedon Oct 27, 2025
I haven't deeply looked into it. But that seems to go in the right direction although I don't immediately know why you'd need to NULL the wrapper
devnexen commentedon Oct 27, 2025
Because it will attempt to destroy the resource otherwise, leading to UB.
iluuu1994 commentedon Oct 28, 2025
Related to #19849. Normally, resources are closed in
zend_close_rsrc_list()before shutting down the executor. However, in this example, another resource is opened when closing the resources, which will be left unclosed.zend_destroy_rsrc_list(&EG(regular_list));(inzend_deactivate()) will then attempt to close it again, which includes the call to userland after the executor has shut down.We could simply keep closing the resources again in
zend_close_rsrc_list(), but there's some concern for infinite recursion. This is not an isolated incidence (there have been at least a handful similar issues), so IMO skipping function calls inzend_call_method_if_exists()whenEG(active)isfalseis reasonable.arnaud-lb commentedon Oct 30, 2025
There are similar considerations for objects: Destructors can create new objects, that need to be destroyed. For objects we chose to keep calling destructors.
The advantage is that it doesn't add an exception to the rule "object destructors are always called".
This does loop infinitely if destructors keep creating new objects (until growing the object store exceeds the memory limit), but doesn't appear to be an issue in practice. Programs that trigger this behavior are broken anyway.
I'm in favor of continuing to close the resources in
zend_close_rsrc_list()while new ones are created, unless the chances of infinite loops are higher in the case of resources.iluuu1994 commentedon Oct 30, 2025
Okay, if you prefer this solution it works for me. Regardless, it may be a good idea to add the check to
zend_call_method_if_exists()so we can avoid triggering callbacks when the VM is shut down. We can keep aZEND_UNREACHABLE()in debug builds so such cases can still be caught through fuzzing.ndossche commentedon Oct 30, 2025
I agree with Arnaud that we should keep closing in
zend_close_rsrc_listuntil the list is empty.I still think we should add an assert to
zend_call_method_if_existsas well though, at least.Fix phpGH-20286: use-after-destroy during userland stream_close()
Fix GH-20286: use-after-destroy during userland stream_close()
Merge branch 'PHP-8.3' into PHP-8.4
Merge branch 'PHP-8.4' into PHP-8.5
Merge branch 'PHP-8.5'