From: Timo Tijhof <krinklemail@gmail.com>
Date: Thu, 27 Sep 2018 23:54:39 +0000 (+0100)
Subject: exception: Report uncaught "Catchable" fatal to "fatal" channel
X-Git-Tag: 1.34.0-rc.0~3971
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=c8ca57f903da259f0be435f8a5e9234deefee26b

exception: Report uncaught "Catchable" fatal to "fatal" channel

Things like "Catchable fatal error: Must be X, null given" stop
execution immediately after the error handler callback, and produce
an HTTP 500 Internal Server Error page. They are very fatal.

Per <https://secure.php.net/manual/en/language.errors.php7.php>,
on PHP 7 this results in a TypeError throwable which will
eventually fatal the request and be reported through
set_exception_handler, or be caught by our top-level 'catch'
and artificially forwarded to our set_exception_handler callback.

On HHVM, these fatal error types are PHP5-like in that they
don't have a throwable object to throw yet, instead the error
meta-data is sent directly as parameeters to set_error_handler,
the same as for warnings. We need to intercept them there so
that they are reported correctly.

Sample from PHP 7:

> MediaWiki\Storage\MutableRevisionRecord::newFromParentRevision( null );
> [no req]   TypeError from line 50 of .../MutableRevisionRecord.php:
> Argument 1 passed to ...\MutableRevisionRecord::newFromParentRevision()
> must be an instance of MediaWiki\Storage\RevisionRecord, null given,
> called in /var/www/mediawiki/maintenance/eval.php(78) ...

[exit status: error(255)]

Sample from HHVM:

> MediaWiki\Storage\MutableRevisionRecord::newFromParentRevision( null );
> [hphp] set_error_handler called with:
> ...  $type = 4096 // E_RECOVERABLE_ERROR
>
> [hphp] [...] Catchable fatal error:
> Argument 1 passed to ...\MutableRevisionRecord::newFromParentRevision()
> must be an instance of MediaWiki\Storage\RevisionRecord, null given
> ...

[exit status: error(255)]

Interestingly, if you forget to return false from set_error_handler
for fatal errors on HHVM, it can actually continue execution.

Bug: T205677
Change-Id: I18dd2ff37fa2c2679d0c598cbeff0c61c2fe0253
---

diff --git a/includes/exception/MWExceptionHandler.php b/includes/exception/MWExceptionHandler.php
index bd823b5e07..0e81a43bab 100644
--- a/includes/exception/MWExceptionHandler.php
+++ b/includes/exception/MWExceptionHandler.php
@@ -35,12 +35,35 @@ class MWExceptionHandler {
 	 * @var string $reservedMemory
 	 */
 	protected static $reservedMemory;
+
 	/**
+	 * Error types that, if unhandled, are fatal to the request.
+	 *
+	 * On PHP 7, these error types may be thrown as Error objects, which
+	 * implement Throwable (but not Exception).
+	 *
+	 * On HHVM, these invoke the set_error_handler callback, similar to how
+	 * (non-fatal) warnings and notices are reported, except that after this
+	 * handler runs for fatal error tpyes, script execution stops!
+	 *
+	 * The user will be shown an HTTP 500 Internal Server Error.
+	 * As such, these should be sent to MediaWiki's "fatal" or "exception"
+	 * channel. Normally, the error handler logs them to the "error" channel.
+	 *
 	 * @var array $fatalErrorTypes
 	 */
 	protected static $fatalErrorTypes = [
-		E_ERROR, E_PARSE, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR,
-		/* HHVM's FATAL_ERROR level */ 16777217,
+		E_ERROR,
+		E_PARSE,
+		E_CORE_ERROR,
+		E_COMPILE_ERROR,
+		E_USER_ERROR,
+
+		// E.g. "Catchable fatal error: Argument X must be Y, null given"
+		E_RECOVERABLE_ERROR,
+
+		// HHVM's FATAL_ERROR constant
+		16777217,
 	];
 	/**
 	 * @var bool $handledFatalCallback
@@ -192,10 +215,6 @@ class MWExceptionHandler {
 		// behaviour given the null was not part of the code and is likely not
 		// accounted for.
 		switch ( $level ) {
-			case E_RECOVERABLE_ERROR:
-				$levelName = 'Error';
-				$severity = LogLevel::ERROR;
-				break;
 			case E_WARNING:
 			case E_CORE_WARNING:
 			case E_COMPILE_WARNING:
@@ -231,9 +250,9 @@ class MWExceptionHandler {
 		self::logError( $e, 'error', $severity );
 
 		// If $wgPropagateErrors is true return false so PHP shows/logs the error normally.
-		// Ignore $wgPropagateErrors if the error should break execution, or track_errors is set
+		// Ignore $wgPropagateErrors if track_errors is set
 		// (which means someone is counting on regular PHP error handling behavior).
-		return !( $wgPropagateErrors || $level == E_RECOVERABLE_ERROR || ini_get( 'track_errors' ) );
+		return !( $wgPropagateErrors || ini_get( 'track_errors' ) );
 	}
 
 	/**