Page MenuHomePhabricator

PHP error "non well formed numeric value encountered" from FormatMetadata->formatCoords
Open, MediumPublicPRODUCTION ERROR

Description

Error

Request URL: commonswiki /wiki/File: … .jpg
Request ID: XRT8AgpAAEQAACgRp9oAAAAD

message
PHP Notice: A non well formed numeric value encountered
trace
#0 /srv/mediawiki/php-1.34.0-wmf.11/includes/media/FormatMetadata.php(1424): MWExceptionHandler::handleError(integer, string, string, integer, array)
#1 /srv/mediawiki/php-1.34.0-wmf.11/includes/media/FormatMetadata.php(689): FormatMetadata->formatCoords(string, string)
#2 /srv/mediawiki/php-1.34.0-wmf.11/includes/media/FormatMetadata.php(88): FormatMetadata->makeFormattedData(array)
#3 /srv/mediawiki/php-1.34.0-wmf.11/includes/media/MediaHandler.php(500): FormatMetadata::getFormattedData(array, RequestContext)
#4 /srv/mediawiki/php-1.34.0-wmf.11/includes/media/ExifBitmapHandler.php(136): MediaHandler->formatMetadataHelper(array, RequestContext)
#5 /srv/mediawiki/php-1.34.0-wmf.11/includes/filerepo/file/File.php(1849): ExifBitmapHandler->formatMetadata(LocalFile, RequestContext)
#6 /srv/mediawiki/php-1.34.0-wmf.11/includes/page/ImagePage.php(126): File->formatMetadata(RequestContext)
#7 /srv/mediawiki/php-1.34.0-wmf.11/includes/actions/ViewAction.php(68): ImagePage->view()
#8 /srv/mediawiki/php-1.34.0-wmf.11/includes/MediaWiki.php(499): ViewAction->show()
#9 /srv/mediawiki/php-1.34.0-wmf.11/includes/MediaWiki.php(294): MediaWiki->performAction(ImagePage, Title)
#10 /srv/mediawiki/php-1.34.0-wmf.11/includes/MediaWiki.php(884): MediaWiki->performRequest()
#11 /srv/mediawiki/php-1.34.0-wmf.11/includes/MediaWiki.php(515): MediaWiki->main()
#12 /srv/mediawiki/php-1.34.0-wmf.11/index.php(42): MediaWiki->run()
#13 /srv/mediawiki/w/index.php(3): require(string)

Impact

The error is unhandled by the code and not made fatal by PHP, as such the code is continuing to execute without being aware of the problem and thus creates output that appears valid and as intended but actually isn't.

In this case, it seems the internal EXIF data from the uploaded photo is:

imageinfo/metadata
{
    "name": "GPSLatitude",
    "value": "1/1"
},
{
    "name": "GPSLongitude",
    "value": "1/1"
},

But is shown on Commons as:

Latitude1° 0′ 0″ N
Longitude1° 0′ 0″ E

If this is valid and correct, it should produce it without an application-level error.

If this is not correct, it should handled in some way (e.g. skip, or a placeholder message informing the user).

Notes

From Logstash:

  • Observed ~ 250 times in the last 30 days, since the deployment of 1.34-wmf.6 (possibly earlier).
  • Affects commons.wikimedia.org and en.wikipedia.org most, but also seen on it.wikipedia.org, zh.wikipedia.org, and de.wikipedia.org.

The low frequency is in part because only PHP 7.2 is able to detect this error. The actual frequency is likely much higher given most traffic is served by HHVM still.

Event Timeline

Restricted Application added subscribers: Stang, Aklapper. · View Herald Transcript
Krinkle renamed this task from PHP error "non well formed numeric value encountered" from FormatMetadata:formatCoords to PHP error "non well formed numeric value encountered" from FormatMetadata->formatCoords.Jun 27 2019, 6:10 PM

The obvious thing to notice is that formatCoords' docblock says the first parameter is an int, while here it's actually a string. And in fact, it seems that the string will be something like "12345/6789" (or "1/1" in this case). *If* this is the correct format (the docblock comment "Degrees, minutes and seconds" doesn't help much), then an explicit int cast should be added.

Anyway, in the first if branch there's $nCoord = -$coord;. On both HHVM and PHP7+, doing that on "num/den" would result in -num, as an integer. The difference is that PHP7 emits the notice reported in the task title, whereas HHVM doesn't.
Then we get to $deg = floor( $nCoord ), and this one will always return +-num, so either way we get rid of the denominator without using it. Then the next lines are similar; some of the operations performed emit notices under PHP7, although the resulting values are the same as in HHVM.

The proper solution depends on whether the input to formatCoords being a rational number is right. If it is, then the method should first of all cast it to a float (e.g. list( $num, $den ) = array_map( 'intval', explode( ',', $coord ) ); $coord = $num / $den;), and maybe add some validation. If it isn't, well, it depends.

mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:06 PM
Krinkle triaged this task as Medium priority.Sep 25 2019, 4:18 PM

This has become more frequency and noisy, following the PHP 7.2 rollout to 100% of external web requests. (Makes sense as this did not affect HHVM, but noting here for the record.)

Noting here that this is still cropping up a fair amount as of 1.35.0-wmf.4:

brennen@mwlog1001:/srv/mw-log$ head -1 error.log | awk '{ print $1 " " $2; }' # start of log
2019-10-31 08:05:58

brennen@mwlog1001:/srv/mw-log$ grep -c includes/media/FormatMetadata ./error.log
1300

It is also possible to make Exif::exifGPStoNumber better to work with that number

But than a file is needed with the bad metadata to find the best way to handle it.

It is possible to get the image name from commons to have a file to test with?

Yeah if someone can give a list of a few images that triggered this, then this can be looked into. I do remember there being a limitation with our Coordinate parsings in Exif being left in there... we might have to trawl through some old commit logs / tickets to check what that issue was.

We have seen something similar, not exactly the same as this reported one, but same file: FormatMetadata.php
https://logstash.wikimedia.org/goto/8cdeea98921fbe0d1ee284fafa4b8bcd

 @timestamp	   	2019-11-26T06:02:31
t @version	   	1
t _id	   	AW6mTeA1arkxubcmWzbD
t _index	   	logstash-deploy-2019.11.26
# _score	   	 - 
t _type	   	mediawiki
t caught_by	   	mwe_handler
t channel	   	error
t exception.class	   	ErrorException
t exception.code	   	0
t exception.file	   	/srv/mediawiki/php-1.35.0-wmf.5/includes/media/FormatMetadata.php:1432
t exception.message	   	PHP Notice: A non well formed numeric value encountered
t exception.trace	   	#0 /srv/mediawiki/php-1.35.0-wmf.5/includes/media/FormatMetadata.php(1432): MWExceptionHandler::handleError(integer, string, string, integer, array)
#1 /srv/mediawiki/php-1.35.0-wmf.5/includes/media/FormatMetadata.php(698): FormatMetadata->formatCoords(string, string)
#2 /srv/mediawiki/php-1.35.0-wmf.5/includes/media/FormatMetadata.php(88): FormatMetadata->makeFormattedData(array)
#3 /srv/mediawiki/php-1.35.0-wmf.5/includes/media/MediaHandler.php(500): FormatMetadata::getFormattedData(array, RequestContext)
#4 /srv/mediawiki/php-1.35.0-wmf.5/includes/media/ExifBitmapHandler.php(136): MediaHandler->formatMetadataHelper(array, RequestContext)
#5 /srv/mediawiki/php-1.35.0-wmf.5/includes/filerepo/file/File.php(1853): ExifBitmapHandler->formatMetadata(ForeignDBFile, RequestContext)
#6 /srv/mediawiki/php-1.35.0-wmf.5/includes/page/ImagePage.php(124): File->formatMetadata(RequestContext)
#7 /srv/mediawiki/php-1.35.0-wmf.5/includes/actions/ViewAction.php(63): ImagePage->view()
#8 /srv/mediawiki/php-1.35.0-wmf.5/includes/MediaWiki.php(514): ViewAction->show()
#9 /srv/mediawiki/php-1.35.0-wmf.5/includes/MediaWiki.php(304): MediaWiki->performAction(ImagePage, Title)
#10 /srv/mediawiki/php-1.35.0-wmf.5/includes/MediaWiki.php(967): MediaWiki->performRequest()
#11 /srv/mediawiki/php-1.35.0-wmf.5/includes/MediaWiki.php(530): MediaWiki->main()
#12 /srv/mediawiki/php-1.35.0-wmf.5/index.php(46): MediaWiki->run()
#13 /srv/mediawiki/w/index.php(3): require(string)
#14 {main}
t exception_id	   	Xdy-9wpAAD8AAE0-JncAAAEC
t exception_url	   	/wiki/%D7%A7%D7%95%D7%91%D7%A5:Pluvianus_aegyptius_3_Luc_Viatour.jpg
t facility	   	user
t host	   	mw1268
t http_method	   	GET
t ip	   	10.64.0.63
t level	   	ERROR
t logsource	   	mw1268
t message	   	[Xdy-9wpAAD8AAE0-JncAAAEC] /wiki/%D7%A7%D7%95%D7%91%D7%A5:Pluvianus_aegyptius_3_Luc_Viatour.jpg   ErrorException from line 1432 of /srv/mediawiki/php-1.35.0-wmf.5/includes/media/FormatMetadata.php: PHP Notice: A non well formed numeric value encountered
t mwversion	   	1.35.0-wmf.5
t normalized_message	   	[{exception_id}] {exception_url}   ErrorException from line 1432 of /srv/mediawiki/php-1.35.0-wmf.5/includes/media/FormatMetadata.php: PHP Notice: A non well formed numeric value encountered
t phpversion	   	7.2.24-1+0~20191026.31+debian9~1.gbpbbacde+wmf1
t program	   	mediawiki
t referrer	   	 - 
t reqId	   	Xdy-9wpAAD8AAE0-JncAAAEC
t server	   	he.wikiquote.org
t severity	   	err
t shard	   	s3
t tags	   	input-kafka-rsyslog-udp-localhost, rsyslog-udp-localhost, kafka, es
t timestamp	   	2019-11-26T08:02:31+00:00
t type	   	mediawiki
t unique_id	   	Xdy-9wpAAD8AAE0-JncAAAEC
t url	   	/wiki/%D7%A7%D7%95%D7%91%D7%A5:Pluvianus_aegyptius_3_Luc_Viatour.jpg
t wiki	   	hewikiquote

These are user errors, so FormatMetadata should probably return a StatusValue, but that's a massive amount of refactoring...

Change 553039 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/core@master] media: Log and fail gracefully on invalid EXIF coordinates

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

The last stack trace shows a image - https://he.wikiquote.org/wiki/קובץ:Pluvianus_aegyptius_3_Luc_Viatour.jpg

The image is using GPSLatitude = 1/1 and GPSLongitude = 1/1

When uploading the image in my local wiki the GPS is not stored as metadata, because the *Ref are missing and mediawiki ignores the exif GPS.

The image was uploaded 2009 under older php and mediawiki, maybe the code already was improved.

Refresh the metadata for that image and it should be okay, but that must be done by a maintenance script as there is no gui action to do that.
Needed for all media types which can contain exif data

Change 553039 merged by jenkins-bot:
[mediawiki/core@master] media: Log and fail gracefully on invalid EXIF coordinates

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

I was going to say this is still happening... But that's because it's not deployed yet

ErrorException from line 1432 of /srv/mediawiki/php-1.35.0-wmf.8/includes/media/FormatMetadata.php: PHP Notice: A non well formed numeric value encountered
ErrorException from line 1433 of /srv/mediawiki/php-1.35.0-wmf.8/includes/media/FormatMetadata.php: PHP Notice: A non well formed numeric value encountered

Change 558638 had a related patch set uploaded (by Reedy; owner: Thiemo Kreuz (WMDE)):
[mediawiki/core@REL1_34] media: Log and fail gracefully on invalid EXIF coordinates

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

Change 558639 had a related patch set uploaded (by Reedy; owner: Thiemo Kreuz (WMDE)):
[mediawiki/core@REL1_33] media: Log and fail gracefully on invalid EXIF coordinates

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

Change 558640 had a related patch set uploaded (by Reedy; owner: Thiemo Kreuz (WMDE)):
[mediawiki/core@REL1_32] media: Log and fail gracefully on invalid EXIF coordinates

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

Change 558641 had a related patch set uploaded (by Reedy; owner: Thiemo Kreuz (WMDE)):
[mediawiki/core@REL1_31] media: Log and fail gracefully on invalid EXIF coordinates

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

Change 558641 merged by jenkins-bot:
[mediawiki/core@REL1_31] media: Log and fail gracefully on invalid EXIF coordinates

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

Change 558640 merged by jenkins-bot:
[mediawiki/core@REL1_32] media: Log and fail gracefully on invalid EXIF coordinates

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

Change 558639 merged by jenkins-bot:
[mediawiki/core@REL1_33] media: Log and fail gracefully on invalid EXIF coordinates

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

Change 558638 merged by jenkins-bot:
[mediawiki/core@REL1_34] media: Log and fail gracefully on invalid EXIF coordinates

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

This was reopened without further information. Does this happen on the same code? Or was it closed/opened on accident?

There is still code in FormatMetadata.php that assumes GPSAltitude is always a number. I see this error about 50 times in the past 30 days. I'm not going to make a patch for this as it is rare and what we are looking at here appears to be a rabbit hole: there are sooooo many code paths in this file that just assume what they get is an integer or float, when it is most likely a string, and possibly even an empty one. I don't know how to fix this without touching half of the file.

The patch before was about latitude and longitude.

thiemowmde added a subscriber: thiemowmde.

There is still code in FormatMetadata.php that assumes GPSAltitude is always a number. I see this error about 50 times in the past 30 days. I'm not going to make a patch for this as it is rare and what we are looking at here appears to be a rabbit hole: there are sooooo many code paths in this file that just assume what they get is an integer or float, when it is most likely a string, and possibly even an empty one. I don't know how to fix this without touching half of the file.

The patch before was about latitude and longitude.

Is that still happen? With T267370 all numeric parts in FormatMetadata::formatNum going to extra log and should not trigger php warnings

Multiple patches have been made in the meantime. Here are some I found in close proximity to the code we are talking about:

There are probably more. It might be that these changes somehow fixed or at least silenced the errors described in this ticket here. Hard to tell. Note that none of these patches contains any tests, which worries me very much. For the record: I think I would not have accepted such fundamental changes without tests.