Skip to content

Fix GH-14643 ext/standard: segfault on user shutdown function release. #14656

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jun 25, 2024
Merged

Fix GH-14643 ext/standard: segfault on user shutdown function release. #14656

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a7a0f9
Fix GH-14643 ext/standard: segfault on user shutdown function release.
devnexen Jun 24, 2024
313913f
add test
devnexen Jun 25, 2024
85ca7a9
changes from feedback
devnexen Jun 25, 2024
4e4ec32
update test
devnexen Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions ext/standard/basic_functions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1582,9 +1582,12 @@ static void fci_release(zend_fcall_info *fci, zend_fcall_info_cache *fci_cache)
void user_shutdown_function_dtor(zval *zv) /* {{{ */
{
php_shutdown_function_entry *shutdown_function_entry = Z_PTR_P(zv);
zend_fcall_info *fci = &shutdown_function_entry->fci;

zend_fcall_info_args_clear(&shutdown_function_entry->fci, true);
fci_release(&shutdown_function_entry->fci, &shutdown_function_entry->fci_cache);
if (fci) {
zend_fcall_info_args_clear(fci, true);
fci_release(fci, &shutdown_function_entry->fci_cache);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to point to a bug elsewhere it should not be possible to not have an FCI here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, the patch also doesn't really make sense: fci is address-taken from a struct member of shutdown_function_entry, so it can only be NULL if shutdown_function_entry is NULL, but in that case it's undefined behaviour because you aren't allowed to do pointer math on NULL...
I think we should all run our development tools with ASAN + UBSAN enabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that we should check shutdown_function_entry here, and not shutdown_function_entry->fci.

My understanding of the issue is that we OOM here when registering a new shutdown function:

php-src/Zend/zend_hash.h

Line 858 in 909d331

Z_PTR_P(zv) = pemalloc(size, GC_FLAGS(ht) & IS_ARRAY_PERSISTENT);

as a consequence, Z_PTR_P(zv) remains NULL (it is initialized to NULL above), so shutdown_function_entry is NULL here:

php-src/ext/standard/basic_functions.c

Line 1584 in 909d331

php_shutdown_function_entry *shutdown_function_entry = Z_PTR_P(zv);

So the root cause is zend_hash_next_index_insert_mem(). We should probably change this function so that it doesn't leave NULL entries in case of allocation failure. We could move the malloc/memcpy before the insert, initialize tmp to that, and free when insert fails. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think moving the allocation before the insert is the right approach.

Copy link
Member Author

@devnexen devnexen Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all for your feedback, just trying to take care of "day to day" stuff as much as I can :) but at least I get better understanding of the internals.

efree(shutdown_function_entry);
}
/* }}} */
Expand Down
24 changes: 24 additions & 0 deletions ext/standard/tests/gh14643_longname.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
GH-14643: Segfault on empty user function.
--FILE--
<?php
$script1_dataflow=5000000;
class Logger {
public function __construct() {
register_shutdown_function(function () {
$this->flush();
register_shutdown_function([$this, 'flush'], true);
});
}
public function flush($final = false) {
}
}
for ($i = 0; $i < 200; $script1_dataflow++) {
$a = new Logger();
}
var_fusion($script1_connect, $script2_connect, $random_var);
?>
--EXPECTF--
Fatal error: Allowed memory size of %d bytes exhausted %s

Fatal error: Allowed memory size of %d bytes exhausted %s
Loading