Skip to content

Fix GH-18033: NULL-ptr dereference when using register_tick_function in destructor #18047

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

Closed
wants to merge 1 commit into from
Closed

Fix GH-18033: NULL-ptr dereference when using register_tick_function in destructor #18047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions UPGRADING
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ PHP 8.5 UPGRADE NOTES
resources that were indirectly collected through cycles.
. It is now allowed to substitute static with self or the concrete class name
in final subclasses.
. The tick handlers are now deactivated after all shutdown functions, destructors
have run and the output handlers have been cleaned up.
This is a consequence of fixing GH-18033.

- Intl:
. The extension now requires at least ICU 57.1.
Expand Down
24 changes: 24 additions & 0 deletions Zend/tests/declare/gh18033_1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
GH-18033 (NULL-ptr dereference when using register_tick_function in destructor)
--DESCRIPTION--
Needs --repeat 2 or something similar to reproduce
--CREDITS--
clesmian
--FILE--
<?php
class Foo {
function __destruct() {
declare(ticks=1);
register_tick_function(
function() { }
);
echo "In destructor\n";
}
}

$bar = new Foo;
echo "Done\n";
?>
--EXPECT--
Done
In destructor
16 changes: 16 additions & 0 deletions Zend/tests/declare/gh18033_2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
--TEST--
GH-18033 (NULL-ptr dereference when using register_tick_function in ob_start)
--DESCRIPTION--
Needs --repeat 2 or something similar to reproduce
--CREDITS--
clesmian
--FILE--
<?php
ob_start(function() {
declare(ticks=1);
register_tick_function(
function() { }
);
});
?>
--EXPECT--
4 changes: 2 additions & 2 deletions main/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1904,8 +1904,6 @@ void php_request_shutdown(void *dummy)
*/
EG(current_execute_data) = NULL;

php_deactivate_ticks();

/* 0. Call any open observer end handlers that are still open after a zend_bailout */
if (ZEND_OBSERVER_ENABLED) {
zend_observer_fcall_end_all();
Expand All @@ -1926,6 +1924,8 @@ void php_request_shutdown(void *dummy)
php_output_end_all();
} zend_end_try();

php_deactivate_ticks();
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 the same issue can occur with buffered output handlers. Does this need to be moved down further?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is already below php_output_end_all and I don't think php_output_deactivate can call user code?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you're probably right! Can't verify as I'm ok mobile.

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 it doesn't call user code, at least not on purpose. Destructors are already called, so destroying the user output handler will not call any user code. The only way for php_output_deactivate() to execute user code would be if it invoked an error handler. But this would be unsafe after zend_deactivate_modules(), so it probably doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. There's even a test. I just didn't look at it well enough.


/* 4. Reset max_execution_time (no longer executing php code after response sent) */
zend_try {
zend_unset_timeout();
Expand Down
Loading