Page MenuHomePhabricator

PHP Fatal error: Field width 139814565865856 is too long (from StatsdData.php)
Closed, ResolvedPublicPRODUCTION ERROR

Description

Error message
PHP Fatal Error: Field width 139814565865856 is too long in StatsdData.php
Stack Trace
#0 [internal function]: unknown()
#1 /srv/mediawiki/php-1.36.0-wmf.30/vendor/liuggio/statsd-php-client/src/Liuggio/StatsdClient/Entity/StatsdData.php(84): sprintf()
#2 /srv/mediawiki/php-1.36.0-wmf.30/vendor/liuggio/statsd-php-client/src/Liuggio/StatsdClient/Entity/StatsdData.php(100): Liuggio\StatsdClient\Entity\StatsdData->getMessage()
#3 [internal function]: Liuggio\StatsdClient\Entity\StatsdData->__toString()
#4 [internal function]: strval()
#5 /srv/mediawiki/php-1.36.0-wmf.30/includes/libs/stats/SamplingStatsdClient.php(100): array_map()
#6 /srv/mediawiki/php-1.36.0-wmf.30/includes/MediaWiki.php(1148): SamplingStatsdClient->send()
#7 /srv/mediawiki/php-1.36.0-wmf.30/includes/GlobalFunctions.php(1112): MediaWiki::emitBufferedStatsdData()
#8 /srv/mediawiki/php-1.36.0-wmf.30/includes/MediaWiki.php(1120): wfLogProfilingData()
#9 /srv/mediawiki/php-1.36.0-wmf.30/includes/MediaWiki.php(853): MediaWiki->restInPeace()
#10 /srv/mediawiki/php-1.36.0-wmf.30/includes/MediaWiki.php(865): MediaWiki->{closure}()
#11 /srv/mediawiki/php-1.36.0-wmf.30/includes/MediaWiki.php(587): MediaWiki->doPostOutputShutdown()
#12 /srv/mediawiki/php-1.36.0-wmf.30/index.php(53): MediaWiki->run()
#13 /srv/mediawiki/php-1.36.0-wmf.30/index.php(46): wfIndexMain()
#14 /srv/mediawiki/w/index.php(3): require()
Impact

Aborted page view, HTTP 500 internal error.

Notes

A single occurrence on ca.wikipedia.org.

Details

Request ID
7b16f041-ed02-4046-a201-a864c25c1393
Request URL
https://ca.wikipedia.org/wiki/…

Event Timeline

From StatsdData.php

 79     public function getMessage($withMetric = true)
 80     {
 81         if (!$withMetric) {
 82             $result = sprintf('%s:%s', $this->getKey(), $this->getValue());
 83         } else {
*84             $result = sprintf('%s:%s|%s', $this->getKey(), $this->getValue(), $this->getMetric());
 85         }
 86
Krinkle renamed this task from wfLogProfilingData() leads to statsd libray complaining: Field width 139814565865856 is too long to PHP Fatal error: Field width 139814565865856 is too long (from StatsdData.php).Feb 17 2021, 4:31 AM
Krinkle changed Request URL from https://ca.wikipedia.org/wiki/Usuari:Xavier_Dengra/proves to https://ca.wikipedia.org/wiki/….
Krinkle updated the task description. (Show Details)
Krinkle edited Stack Trace. (Show Details)

I'm guessing there's code somewhere that's wrongly letting some kind of string, indirectly based on user input, serve directly as the name of a statsd metric. Maybe an in_array check is failing somewhere.

This looks strange. The php code allows not to have string bigger than INT_MAX (https://github.com/php/php-src/blob/master/ext/standard/formatted_print.c#L100)
But the number is 65106 times (near at 2^16 = 65536) bigger than 2.147.483.648 (= 2^31). The sprintf gets three strings, which itself could not be bigger than INT_MAX making it worse

One fix is to use 64-bit php, another is to limit the key length in BufferingStatsdDataFactory::normalizeMetricKey to something useful? I would assume it is not coming from the value/metric, more from the key.

One unsane key build seems to be in https://gerrit.wikimedia.org/g/mediawiki/extensions/WikimediaEvents/+/ff7f55f570d4953ca387e72d65b98ddbe168f9cc/includes/WikimediaEventsHooks.php#671
but that is for dewiki only and not sure what WebRequest::getVal can return

Another one is https://gerrit.wikimedia.org/g/mediawiki/extensions/ContentTranslation/+/36529fd9af20b92aecad3d7356acb8e93d93d9f1/api/ApiContentTranslationPublish.php#114 also depends on what WebRequest::getVal can return.

Krinkle triaged this task as Medium priority.Mar 2 2021, 9:30 PM
Krinkle added a project: Performance-Team.
Krinkle moved this task from Inbox to Doing: Prio Interrupt on the Performance-Team board.
Krinkle added a subscriber: Performance-Team.
Krinkle removed a subscriber: Performance-Team.
Krinkle added a subscriber: Nikerabbit.
ApiContentTranslationPublish.php
	public function getAllowedParams() {
		return [ // …
			'to' => [
				ApiBase::PARAM_REQUIRED => true,
			],
			/* … */
	}

	protected function getCategories( array $params ) {
		/* … */
		MediaWikiServices::getInstance()->getStatsdDataFactory()
			->increment( 'cx.publish.highmt.' . $params['to'] );
		/* … */
		foreach ( $categories as $index => $category ) {
			$category = $this->removeApiCategoryNamespacePrefix( $category, $params['to'] );
			/* … */
	}

	private function removeApiCategoryNamespacePrefix( $category, $targetLanguage ) {
		$targetLanguage = MediaWikiServices::getInstance()->getLanguageFactory()->getLanguage( $targetLanguage );
		/* … */
	}

@Nikerabbit Could your team confirm that this is currently unvalidated and unnormalized, and if so, to patch it such that it either only logs stats for valid/canonical lang codes or normalises it to one?

(I don't know if this API wants to support non-canonical lang codes, they probably work today, so maybe that can continue; otherwise the the code for normalizing could be moved earlier such that requests are rejected if they are not matching the canonical form.)

ApiContentTranslationPublish.php
	public function execute() {
		$params = $this->extractRequestParams();

		/* … */

		if ( !Language::isKnownLanguageTag( $params['to'] ) ) {
			$this->dieWithError( 'apierror-cx-invalidtargetlanguage', 'invalidtargetlanguage' );
		}

		/* … */

		$this->publish(); // Calls ::saveWikitext which calls ::getCategories
	}
>

There is only one code path that calls ::getCategories and that one is guarded by Language::isKnownLanguageTag and ApiBase::dieWithError. There are no writes to $params in this class, so it seems impossible that it would get changed after validation. Also the URL indicates this is not a CX API call. Back to you @Krinkle .

This looks strange. The php code allows not to have string bigger than INT_MAX (https://github.com/php/php-src/blob/master/ext/standard/formatted_print.c#L100)
But the number is 65106 times (near at 2^16 = 65536) bigger than 2.147.483.648 (= 2^31). The sprintf gets three strings, which itself could not be bigger than INT_MAX making it worse

One fix is to use 64-bit php, another is to limit the key length in BufferingStatsdDataFactory::normalizeMetricKey to something useful? I would assume it is not coming from the value/metric, more from the key.

We should probably set a limit here for good measure.
AFAIK graphite can't store metrics over 4096 in length (due to unix path limitations), so we could add a pretty uncontroversial limit there I expect.
Just need to decide what to do in cases where the limit is reached.

One unsane key build seems to be in https://gerrit.wikimedia.org/g/mediawiki/extensions/WikimediaEvents/+/ff7f55f570d4953ca387e72d65b98ddbe168f9cc/includes/WikimediaEventsHooks.php#671
but that is for dewiki only and not sure what WebRequest::getVal can return

I'm checking internally to see if I can get rid of this entirely.
If I can not I'll add a much stricter limit to it. 20-40 chars or so for the input bit.

Change 674537 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/WikimediaEvents@master] Remove statsd metric from onBeforeInitializeWMDECampaign hook

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

Change 674537 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@master] Remove statsd metric from onBeforeInitializeWMDECampaign hook

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

Change 674573 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikimediaEvents@master] Update outdated comment

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

Change 674573 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@master] Update outdated comment

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