Skip to content

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

Closed
ndossche wants to merge 1 commit into
php:masterfrom
ndossche:fix-18033
Closed

ndossche wants to merge 1 commit into
php:masterfrom
ndossche:fix-18033

Conversation

@ndossche

Copy link
Copy Markdown
Member

The problem is that php_request_shutdown calls php_deactivate_ticks prior to running destructors and the shutdown functions and finalizing output handlers.
So if a destructor or shutdown function re-registers a tick function, then the user tick functions handler will be added back to PG(tick_functions). When the next request happens, the list PG(tick_functions) still contains an entry to call the user tick functions (added in the previous request during shutdown). This causes a NULL deref eventually because run_user_tick_functions assumes that if it is called then BG(user_tick_functions) must be non-NULL.

Fix this by moving the tick handler deactivation.

@ndossche ndossche linked an issue Mar 13, 2025 that may be closed by this pull request
…on in destructor

The problem is that `php_request_shutdown` calls `php_deactivate_ticks` prior
to running destructors and the shutdown functions and finalizing output
handlers.
So if a destructor or shutdown function re-registers a tick function,
then the user tick functions handler will be added back to `PG(tick_functions)`.
When the next request happens, the list `PG(tick_functions)` still contains an
entry to call the user tick functions (added in the previous request
during shutdown). This causes a NULL deref eventually because
`run_user_tick_functions` assumes that if it is called then
`BG(user_tick_functions)` must be non-NULL.

Fix this by moving the tick handler deactivation.
Comment thread main/main.c
php_output_end_all();
} zend_end_try();

php_deactivate_ticks();

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@ndossche
ndossche requested a review from arnaud-lb March 13, 2025 19:48
@ndossche
ndossche marked this pull request as ready for review March 13, 2025 19:48
@ndossche
ndossche requested a review from bukka as a code owner March 13, 2025 19:48
@ndossche
ndossche removed the request for review from bukka March 13, 2025 19:48

@arnaud-lb arnaud-lb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me!

Comment thread main/main.c
php_output_end_all();
} zend_end_try();

php_deactivate_ticks();

Copy link
Copy Markdown
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.

@ndossche ndossche closed this in 867ed15 Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NULL-ptr dereference when using register_tick_function in destructor

3 participants