WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 12 months ago

Last modified 7 months ago

#46038 closed defect (bug) (fixed)

Exiting during the WSoD shutdown handler prevents all other shutdown handlers from being called

Reported by: johnbillion Owned by: schlessera
Milestone: 5.1 Priority: high
Severity: normal Version: 5.1
Component: Site Health Keywords: servehappy has-patch dev-feedback
Focuses: Cc:
PR Number:

Description

When multiple shutdown handlers are registered, any shutdown handler that calls exit will immediately terminate processing and prevent subsequent shutdown handlers from being called.

Ref: https://secure.php.net/manual/en/function.register-shutdown-function.php

The newly introduced WSoD protection shutdown handler (WP_Shutdown_Handler::display_error_template()) calls wp_die() by default. If a wp-content/php-error.php drop-in is present, it loads that instead but still manually calls die().

The WSoD protection shutdown handler is registered very early on in wp-settings.php, which means that all subsequent shutdown handlers -- including the various other shutdown handlers in core, anything hooked onto the shutdown hook, and any shutdown handlers registered by plugins -- do not fire after a fatal error occurs, which they did prior to the introduction of the WSoD protection shutdown handler.

This can be demonstrated with the following code:

add_action( 'shutdown', function() {
    echo 'Goodbye, World!';
} );
function_that_does_not_exist();

Prior to the introduction of the WSoD protection shutdown handler, the callback attached to the shutdown action would be called and it would do its thing.

WordPress core has shutdown handlers registered for destructors for the wpdb and WP_Object_Cache classes, along with the shutdown_action_hook() function. This should all continue to fire after a fatal error.

The WSoD protection shutdown handler needs to be altered so it does not exit, either indirectly via wp_die() or directly via die().

Attachments (2)

46038.diff (3.6 KB) - added by schlessera 12 months ago.
(replaced with an updated version)
46038.2.diff (3.2 KB) - added by flixos90 12 months ago.

Download all attachments as: .zip

Change History (14)

#1 @johnbillion
12 months ago

It's pretty likely that this breaks plugins or libraries that log or otherwise process fatal errors. Sentry for PHP and the Query Monitor plugin have both broken.

#2 @flixos90
12 months ago

  • Priority changed from normal to high

#3 @schlessera
12 months ago

I'm currently investigating how to solve this. But first of all, I'd like to address the issue of the DB/Cache destructors mentioned above.

According to the PHP manual:

The destructor will be called even if script execution is stopped using exit(). Calling exit() in a destructor will prevent the remaining shutdown routines from executing.

So, while we do need to address the exit() we have in our WSOD protection, I don't understand why there are "shutdown handlers" to call the destructors in the first place. That seems like a PHP-4-era left-over. The entire point of having destructors is that you can rely on them being called.

Furthermore, the only thing the Core-provided destructors do is return true;. If you look at the signature from the PHP manual, one can see that this is a bug: destructors are not meant to return anything:

__destruct ( void ) : void

This makes perfect sense, as they are not meant to be directly called anyway.

So I suggest creating two follow-up bug tickets:

  • Do not register shutdown handlers to call destructors explicitly
  • Adapt wpdb::__destruct() and WP_Object_Cache::__destruct() signatures and code to have a return type of void

#4 @schlessera
12 months ago

  • Keywords has-patch dev-feedback added; needs-patch removed

I uploaded a patch that adds an 'exit' argument to wp_die() that defaults to true but that you can set to false. This allows us to reuse the complete response type handling (checking headers, response type, etc...) but just skip the die() at the very end. This way, all other shutdown handlers can still run in sequence after the WSOD protection.

As this allows additional random output to be generated after the wp_die() output, I also added an open div tag to display: none whatever comes next. This is of course debatable, not sure whether this is needed, or whether this is even a good approach to hide the subsequent dump.

@johnbillion Can you test whether this solves the issues with the two plugins you mentioned?

@schlessera
12 months ago

(replaced with an updated version)

This ticket was mentioned in Slack in #core-php by flixos90. View the logs.


12 months ago

#6 @flixos90
12 months ago

  • Owner set to flixos90
  • Status changed from new to reviewing

@flixos90
12 months ago

#7 @flixos90
12 months ago

46038.2.diff applies against the latest trunk changes and fixes a few issues with undefined variables or array keys. It furthermore simply calls return after the php-error.php drop-in inclusion, rather than die() or wp_die().

#8 @flixos90
12 months ago

  • Owner changed from flixos90 to schlessera

#9 @flixos90
12 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 44671:

Bootstrap/Load: Ensure that the fatal error shutdown handler does not prevent other shutdown handlers from being called.

This changeset adds support for a new wp_die() argument exit, which defaults to true and determines whether wp_die() should actually terminate the request. The new fatal error handler then calls wp_die() with that argument set to false, as calling die() or exit from a PHP shutdown function prevents other shutdown functions from being called.

Props schlessera, johnbillion.
Fixes #46038. See #44458.

#10 @johnbillion
12 months ago

In 44682:

Bootstrap/Load: Update the @since entry for wp_die().

See #46038

#11 @flixos90
12 months ago

In 44717:

Bootstrap/Load: Revert fatal error recovery mechanism from 5.1 to polish for 5.2.

Due to the high number of follow-up tickets and associated security concerns, it was decided to reschedule the fatal error recovery feature for WordPress 5.2, in order to address these issues properly. The feature will continue to be developed, with iterations being merged into trunk early in the 5.2 release cycle.

Fixes #46141. See #44458, #45932, #45940, #46038, #46047, #46068.

#12 @spacedmonkey
7 months ago

  • Component changed from Bootstrap/Load to Site Health
Note: See TracTickets for help on using tickets.