Page MenuHomePhabricator

Unable to view certain pages on incubator.wikimedia.org (Fatal error: operator not supported)
Closed, ResolvedPublic

Description

Error

Request URL: /wiki/Wt/en (at incubator.wikimedia.org)
Request ID: XK4KSgpAICgAAEsXOUUAAACV

message
Fatal error: [] operator not supported for strings

PHP Fatal Error: Uncaught Error: [] operator not supported for strings in /srv/mediawiki/php-1.33.0-wmf.25/extensions/WikimediaIncubator/includes/WikimediaIncubator.php:402
trace
#0 /srv/mediawiki/php-1.33.0-wmf.24/includes/Hooks.php(174): WikimediaIncubator::onGetUserPermissionsErrors(Title, User, string, string)
#1 /srv/mediawiki/php-1.33.0-wmf.24/includes/Hooks.php(202): Hooks::callHook(string, array, array, NULL)
#2 /srv/mediawiki/php-1.33.0-wmf.24/includes/Title.php(2278): Hooks::run(string, array)
#3 /srv/mediawiki/php-1.33.0-wmf.24/includes/Title.php(2837): Title->checkPermissionHooks(string, User, array, string, boolean)
#4 /srv/mediawiki/php-1.33.0-wmf.24/includes/Title.php(2117): Title->getUserPermissionsErrorsInternal(string, User, string, boolean)
#5 /srv/mediawiki/php-1.33.0-wmf.24/includes/Title.php(2099): Title->userCan(string, User, boolean)
#6 /srv/mediawiki/php-1.33.0-wmf.24/includes/skins/SkinTemplate.php(914): Title->quickUserCan(string, User)
#7 /srv/mediawiki/php-1.33.0-wmf.24/includes/skins/SkinTemplate.php(466): SkinTemplate->buildContentNavigationUrls()
#8 /srv/mediawiki/php-1.33.0-wmf.24/includes/skins/SkinTemplate.php(228): SkinTemplate->prepareQuickTemplate()
#9 /srv/mediawiki/php-1.33.0-wmf.24/includes/OutputPage.php(2723): SkinTemplate->outputPage()
#10 /srv/mediawiki/php-1.33.0-wmf.24/includes/exception/MWExceptionRenderer.php(136): OutputPage->output()
#11 /srv/mediawiki/php-1.33.0-wmf.24/includes/exception/MWExceptionRenderer.php(53): MWExceptionRenderer::reportHTML(Error)
#12 /srv/mediawiki/php-1.33.0-wmf.24/includes/exception/MWExceptionHandler.php(98): MWExceptionRenderer::output(Error, integer)
#13 /srv/mediawiki/php-1.33.0-wmf.24/includes/exception/MWExceptionHandler.php(172): MWExceptionHandler::report(Error)
#14 /srv/mediawiki/php-1.33.0-wmf.24/includes/MediaWiki.php(547): MWExceptionHandler::handleException(Error)
#15 /srv/mediawiki/php-1.33.0-wmf.24/index.php(42): MediaWiki->run()

Impact

  • Users are consistently unable to view certain pages on Wikimedia Incubator.
  • The fatal is triggered from a GET request and affects anonymous users as well. These result in uncached HTTP 500 Error responses which is a risk for load, caching, and false alarms affecting MW deployments and SRE alerts.

Notes

Affects both PHP 7.2 and HHVM (error message varies slightly).

Users have encountered this since at least php-1.33.0-wmf.21.

Event Timeline

Krinkle created this task.Apr 10 2019, 4:00 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 10 2019, 4:00 PM
Krinkle updated the task description. (Show Details)Apr 10 2019, 4:25 PM

Unfortunately this does not list the line of failure. Only the stracktrace is shown, but not the line inside that function

$prefixdata is handled as array, but self::analyzePrefix always returns an array - looks safe

The change from 1.33-wmf.21 is https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/WikimediaIncubator/+/496221/ but on other lines

Last change from 1.33-wmf.22 in that function is https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/WikimediaIncubator/+/496221/

This comment was removed by Krinkle.
Krinkle updated the task description. (Show Details)Apr 13 2019, 4:29 PM

@Umherirrender Line number now included.

Krinkle triaged this task as High priority.EditedApr 15 2019, 9:29 PM

It looks like the problem is a faulty hook parameter, which might actually originate from other extensions in production that hook into the same event.

https://codesearch.wmflabs.org/deployed/?q=function.%2BetUserPermissionsErrors&i=nope&files=&repos=

Some set a string here (such as Flow). Which seems actually valid according to the docs.

&$result: … $result can be returned as a single error message key (string), or an array of error message keys …

So, looks like Incubator is in error after all.

EDIT: Except whenever it is set to a string, the hook should return false which would end the callback sequence. If Incubator is getting a string, that must mean a previous caller declared a permission error without returning false?

Reedy added a subscriber: Reedy.Apr 15 2019, 9:40 PM
reedy@deploy1001:~$ mwscript eval.php incubatorwiki
> var_dump( $wgHooks['getUserPermissionsErrors'] );
array(2) {
  [0]=>
  string(46) "WikimediaIncubator::onGetUserPermissionsErrors"
  [1]=>
  string(39) "PageTranslationHooks::preventPatrolling"
}
	 * Prevent patrol links from appearing on translation pages.
	 * Hook: getUserPermissionsErrors
	 *
	 * @param Title $title
	 * @param User $user
	 * @param string $action
	 * @param mixed &$result
	 *
	 * @return bool
	 */
	public static function preventPatrolling( Title $title, User $user, $action, &$result ) {
		if ( $action !== 'patrol' ) {
			return true;
		}

		$page = TranslatablePage::isTranslationPage( $title );

		if ( $page !== false ) {
			$result = [ 'tpt-patrolling-blocked' ];
			return false;
		}

		return true;
	}
Reedy added a comment.Apr 15 2019, 9:49 PM

It looks like the problem is a faulty hook parameter, which might actually originate from other extensions in production that hook into the same event.

https://codesearch.wmflabs.org/deployed/?q=function.%2BetUserPermissionsErrors&i=nope&files=&repos=

Some set a string here (such as Flow). Which seems actually valid according to the docs.

&$result: … $result can be returned as a single error message key (string), or an array of error message keys …

So, looks like Incubator is in error after all.

EDIT: Except whenever it is set to a string, the hook should return false which would end the callback sequence. If Incubator is getting a string, that must mean a previous caller declared a permission error without returning false?

Probably need to look at userCan too...

		$result = '';
		if ( !Hooks::run( 'userCan', [ &$page, &$user, $action, &$result ] ) ) {
			return $result ? [] : [ [ 'badaccess-group0' ] ];
		}
		// Check getUserPermissionsErrors hook
		if ( !Hooks::run( 'getUserPermissionsErrors', [ &$page, &$user, $action, &$result ] ) ) {
			$errors = $this->resultToError( $errors, $result );
		}
> var_dump( $wgHooks['userCan'] );
array(1) {
  [0]=>
  string(33) "JsonConfig\JCSingleton::onuserCan"
}
Reedy added a comment.EditedApr 15 2019, 9:51 PM
	/**
	 * Prohibit creation of the pages that are part of our namespaces but have not been explicitly
	 * allowed. Bad capitalization is due to "userCan" hook name
	 * @param Title &$title
	 * @param User &$user
	 * @param string $action
	 * @param null &$result
	 * @return bool
	 */
	public static function onuserCan(
		/** @noinspection PhpUnusedParameterInspection */
		&$title, &$user, $action, &$result = null
	) {
		if ( !self::jsonConfigIsStorage() ) {
			return true;
		}

		if ( $action === 'create' && self::parseTitle( $title ) === null ) {
			// prohibit creation of the pages for the namespace that we handle,
			// if the title is not matching declared rules
			$result = false;
			return false;
		}
		return true;
	}

I note the stack trace is out of date....

2019-04-15 19:22:52 [XLTaDApAAEMAAENpQwoAAABT] mw1272 incubatorwiki 1.33.0-wmf.25 fatal ERROR: [XLTaDApAAEMAAENpQwoAAABT] /wiki/Wp/roa-rup   PHP Fatal Error from line 402 of /srv/mediawiki/php-1.33.0-wmf.25/extensions/WikimediaIncubator/includes/WikimediaIncubator.php: Uncaught Error: [] operator not supported for strings in /srv/mediawiki/php-1.33.0-wmf.25/extensions/WikimediaIncubator/includes/WikimediaIncubator.php:402
Stack trace:
#0 /srv/mediawiki/php-1.33.0-wmf.25/includes/Hooks.php(174): WikimediaIncubator::onGetUserPermissionsErrors(Object(Title), Object(User), 'read', '')
#1 /srv/mediawiki/php-1.33.0-wmf.25/includes/Hooks.php(202): Hooks::callHook('getUserPermissi...', Array, Array, NULL)
#2 /srv/mediawiki/php-1.33.0-wmf.25/includes/Permissions/PermissionManager.php(302): Hooks::run('getUserPermissi...', Array)
#3 /srv/mediawiki/php-1.33.0-wmf.25/includes/Permissions/PermissionManager.php(258): MediaWiki\Permissions\PermissionManager->checkPermissionHooks('read', Object(User), Array, 'quick', true, Object(Title))
#4 /srv/mediawiki/php-1.33.0-wmf.25/includes/Permissions/PermissionManager.php(111): MediaWiki\Permissions\PermissionManager->getPermissionErrorsInternal('read', Object(User), Object(Title), 'quick', true)
#5 /srv/mediawiki/php-1.33.0-wmf.25/includes/Title {"fatal_exception":{"class":"ErrorException","message":"PHP Fatal Error: Uncaught Error: [] operator not supported for strings in /srv/mediawiki/php-1.33.0-wmf.25/extensions/WikimediaIncubator/includes/WikimediaIncubator.php:402
Stack trace:
#0 /srv/mediawiki/php-1.33.0-wmf.25/includes/Hooks.php(174): WikimediaIncubator::onGetUserPermissionsErrors(Object(Title), Object(User), 'read', '')
#1 /srv/mediawiki/php-1.33.0-wmf.25/includes/Hooks.php(202): Hooks::callHook('getUserPermissi...', Array, Array, NULL)
#2 /srv/mediawiki/php-1.33.0-wmf.25/includes/Permissions/PermissionManager.php(302): Hooks::run('getUserPermissi...', Array)
#3 /srv/mediawiki/php-1.33.0-wmf.25/includes/Permissions/PermissionManager.php(258): MediaWiki\\Permissions\\PermissionManager->checkPermissionHooks('read', Object(User), Array, 'quick', true, Object(Title))
#4 /srv/mediawiki/php-1.33.0-wmf.25/includes/Permissions/PermissionManager.php(111): MediaWiki\\Permissions\\PermissionManager->getPermissionErrorsInternal('read', Object(User), Object(Title), 'quick', true)
#5 /srv/mediawiki/php-1.33.0-wmf.25/includes/Title","code":1,"file":"/srv/mediawiki/php-1.33.0-wmf.25/extensions/WikimediaIncubator/includes/WikimediaIncubator.php","line":402,"trace":"#0 [internal function]: MWExceptionHandler::handleFatalError()

Ha, so it's actually an empty string by default. Interesting. Yeah, so Incubator just needs a patch then to ignore that and simply assign plainly like other hook handlers do…

Reedy added a comment.Apr 16 2019, 5:16 PM

Ha, so it's actually an empty string by default. Interesting. Yeah, so Incubator just needs a patch then to ignore that and simply assign plainly like other hook handlers do…

And not care about overwriting?

Change 504379 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/WikimediaIncubator@master] Unconditionally overwrite $result in WikimediaIncubator::onGetUserPermissionsErrors

https://gerrit.wikimedia.org/r/504379

Change 504379 merged by jenkins-bot:
[mediawiki/extensions/WikimediaIncubator@master] Unconditionally overwrite $result in WikimediaIncubator::onGetUserPermissionsErrors

https://gerrit.wikimedia.org/r/504379

Change 504385 had a related patch set uploaded (by Krinkle; owner: Reedy):
[mediawiki/extensions/WikimediaIncubator@wmf/1.33.0-wmf.25] Unconditionally overwrite $result in WikimediaIncubator::onGetUserPermissionsErrors

https://gerrit.wikimedia.org/r/504385

Change 504385 merged by jenkins-bot:
[mediawiki/extensions/WikimediaIncubator@wmf/1.33.0-wmf.25] Unconditionally overwrite $result in WikimediaIncubator::onGetUserPermissionsErrors

https://gerrit.wikimedia.org/r/504385

Mentioned in SAL (#wikimedia-operations) [2019-04-16T17:56:51Z] <reedy@deploy1001> Synchronized php-1.33.0-wmf.25/extensions/WikimediaIncubator/: T220623 (duration: 00m 53s)

Krinkle closed this task as Resolved.Apr 19 2019, 3:45 PM
Krinkle assigned this task to Reedy.

The line @Reedy quoted above from Translate actually had the same problem before it was fixed: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/Translate/+/5cd35b2fed14373152c544f365d608ed4c1c9a23%5E%21/tag/PageTranslationHooks.php

I suppose there has been some copy-paste going on and it wouldn't really hurt to audit other users of the hook.