Fatal error uploading media on PHP8

[stevegs]

The attached image file gives a fatal error on a WP 5.8.1 system running on PHP8. This doesn't happen under PHP7.4, nor does it if the image (which is straight off a camera) is brought into Paintshop Pro and then saved without making any modification (though Paintshop reduces the file size by ~50% and tags it 'JFIF' - the original is tagged 'EXIF')

[stevegs]

Screenshot of PHP errors; Image file causing these errors.

[stevegs]

Image file causing these errors

[TobiasBg]

Welcome to the WordPress Trac and thanks for your report!

It looks like this can happen if the wp_exif_frac2dec() function returns a string, instead of a number. According to its documentation, it shouldn't be doing that, but does in a few edge cases.

[stevegs]

The strange thing about this is only some EXIF files cause the problem, but it seems no JFIF file does so.

I've generally found PHP8 is a lot more strict (especially regarding typedefs or unexpected nulls) than its predecessor, having found many similar errors in some plugins (resolved by updating, re-writing or deleting). Hope this has been helpful.

[praem90]

As per the ​https://developer.wordpress.org/reference/functions/wp_exif_frac2dec/#source, it looks like the function would return the same string if there is no slash (/) or the denominator is empty.

The given string may not be casted into int.

And the documentation needs to updated.

Replying to stevegs:

The strange thing about this is only some EXIF files cause the problem, but it seems no JFIF file does so.

I've generally found PHP8 is a lot more strict (especially regarding typedefs or unexpected nulls) than its predecessor, having found many similar errors in some plugins (resolved by updating, re-writing or deleting). Hope this has been helpful.

[adamsilverstein]

Hi @stevegs, thanks for reporting this issue. I was able to reproduce this issue and will work on a fix.

Fix an issue where certain images could cause a fatal error under PHP 8+, because wp_exif_frac2dec unexpectedly returns a string for certain images.

Since PHP 8, argument #1 ($num) passed to the function round() must be of type int|float, otherwise a PHP Fatal error: Uncaught TypeError error is thrown.

Trac ticket: https://core.trac.wordpress.org/ticket/54385

[adamsilverstein]

In my testing with your image, the value passed to wp_exif_frac2dec wound up being "0/0", and that string was returned by the function directly because empty( $denominator ) winds up being true in the conditional (empty( 0 ) is true).

In 54385.2.diff​:

• Cast the result to float before calling round
• Update doc block for wp_exif_frac2dec to clarify that it returns a float or a string

[adamsilverstein]

This looks like the same issue that was reported in https://core.trac.wordpress.org/ticket/54009 by @quantumzen.

[TobiasBg]

Wouldn't it be better to make wp_exif_frac2dec() always return an int or float, as inline-documented, and as the name indicates?

For the first if, a type conversion like
return 0 + $str; // Cast to int or float.
or
return (float) $str;
should be sufficient.

For fallback after the second if, I would lean to
return 0;
as that's what a
(float) wp_exif_frac2dec( '0/0' );
as in the suggested patch would result in, too.

[adamsilverstein]

Wouldn't it be better to make wp_exif_frac2dec() always return an int or float, as inline-documented, and as the name indicates?

Good question, I considered that. The issue there is potentially breaking other code that uses the function and expects it to return the original string as it does now and has since WordPress 2.5. When a behavior has been in place for a long time like this, changing it can have unexpected results. Casting the return feels like a safer change to make, and we already cast the return in the two other places wp_exif_frac2dec is used in core.

I'll do a quick search of the plugin repo to see if plugins use this directly. What do you think @SergeyBiryukov?

[adamsilverstein]

I'll do a quick search of the plugin repo

Reviewing current usage by plugins, the most popular ones (from this search: ​https://wpdirectory.net/search/01FM2JG73C9T9369YKYXJE6SNR) actually use it the exact way core does, meaning they round the results directly and will break under PHP 8 unless we cast the response.

@TobiasBg Indeed, casting the return might be better. I'll update the patch to work that way instead as you suggestive.

[adamsilverstein]

In 54385.3.diff​

• ensure wp_exif_frac2dec always returns a float or int, matching the docblock description

[adamsilverstein]

#54009 was marked as a duplicate.

[adamsilverstein]

Sugarloaf Mountain.jpg​ is a second sample image from @quantumzen that reproduces the issue (from https://core.trac.wordpress.org/ticket/54009).

@adamsilverstein 👋🏻 Hope life is treating you well!

Hi @jrfnl :) thanks for the feedback, hope things are well with you!

You make good points, this function is a mess.

Are there any unit tests for this function at all ? Might it be an idea to add some and then to fix the function to only handle valid input and return 0 for invalid input ?

Tests would be helpful, I agree. I was trying to fix the PHP fatal error for images that are missing meta (and call this function with '0/0') without doing much else, but agree a more complete fix would be better.

Since this a bug fix we can continue to work on it, I'll add some more logic to enforce that (int)/(int) is what is passed, catch division by 0 etc, and add some tests that prove this out. returning 0 for any issues makes sense to me.

Let me know if I can help.

I'll pick this up some time tomorrow US Mountain time as I'd like to get it in soon, if you have time and add something before then that would be fantastic.

[stevegs]

Replying to adamsilverstein:

Sugarloaf Mountain.jpg​ is a second sample image from @quantumzen that reproduces the issue (from https://core.trac.wordpress.org/ticket/54009).

What a beautiful photo, Adam!

Thanks guys. For a temporary fix, I have changed my image.php file as follows:

Old line 844:
$meta['aperture'] = round( wp_exif_frac2dec( $exif['FNumber'] ), 2 );
New line 844:
$meta['aperture'] = round( (float) wp_exif_frac2dec( $exif['FNumber'] ), 2 );

and the original image now loads fine.

I've noticed wp_exif_frac2dec() appears in 2 other locations in image.php: in both these cases the return is locally cast to a string. From my relatively newbie point of view, since this is an arithmetic function, it should always return a float. It should then be the responsibility of anything that calls it to recast to a string (or whatever else) to suit its own purpose. But then, as others have pointed out here, there is the risk something elsewhere that calls the function will break under PHP8 if it hasn't done any recasting required. Happy grepping!

Meanwhile, I have advised our photographer to bring her photos into Paintshop Pro, which strips out all the EXIF info, before uploading.

I'll pick this up some time tomorrow US Mountain time as I'd like to get it in soon, if you have time and add something before then that would be fantastic.

Feel free to ping me on Slack when you start working on it and I'll let you know whether I'm available.

Feel free to ping me on Slack when you start working on it and I'll let you know whether I'm available.

Picking this up again, I'll ping you for review when ready!

[TobiasBg]

Thanks, @adamsilverstein!

It seems that your latest patches contain a few other code changes as well, that are unrelated?

For consistency with the other two, should the last if check maybe be flipped to

// The denominator must not be 0.
if ( 0 === $denominator ) {
    return 0;
}

return $numerator / $denominator;

That way, all if checks return 0.

[adamsilverstein]

It seems that your latest patches contain a few other code changes as well, that are unrelated?

Oops, rebased to trunk, let me fix that diff

if ( 0 === $denominator ) {

makes sense for consistency, will change.

I also switched back to using empty which is more consistent for PHP 7.

[adamsilverstein]

In 54385.7.diff​

• Return 0 if fraction parsing error occurs
• Ensure only one "/" in fraction
• Ensure denominator and numerator are numeric.
• Add tests.

Possibly worth adding a dev note for this change? The function is now a little more strict about input handling and will return 0 instead of the passed string in error cases (like "0/0").

[adamsilverstein]

In 54385.8.diff​

• add one more test case with an int return value.

[TobiasBg]

Some suggestions:

In 54385.9.diff​

• Use str_contains() (from PHP 8.0, but polyfilled in [52039]) instead of that strpos()
• Remove an extra empty line
• remove unnecessary global $wpdb;

[stevegs]

Replying to TobiasBg:

Some suggestions:

In 54385.9.diff​

• Use str_contains() (from PHP 8.0, but polyfilled in [52039]) instead of that strpos()
• Remove an extra empty line
• remove unnecessary global $wpdb;

str_contains() is new in PHP8. Not everyone has access to PHP8 (one of the many reasons why I took my hosting away from 123-Reg was because they don't offer the latest version of PHP). So I suggest retain the less elegant strpos() for now to maintain backward compatibility.

[TobiasBg]

Replying to stevegs:

str_contains() is new in PHP8. Not everyone has access to PHP8.

That's correct, and that's why WordPress 5.9 will ship with its own copy of str_contains(), thanks to the recent code change [52039] :-) So starting with WP 5.9, for which the fix of this ticket is aimed for, it's perfectly save to use str_contains() even if someone uses it on PHP < 8.0.

[adamsilverstein]

Thanks for the feedback and new patches @TobiasBg
. I'll incorporate this feedback/changes in an upcoming patch.

After reviewing the values that are passed to the function, I have decided to restore the behavior that returns a string, this is what is expected by the calls to it, see ​https://github.com/WordPress/wordpress-develop/pull/1841/files#r752314252

[adamsilverstein]

In 54385.10.diff​

• Cleaned up strings, doc blocks a bit.
• Improved test case structure
• Add some new test cases for typical values after researching exif values that are passed to the function in our use.

props @jrf for feedback on the GitHub draft PR.

Thanks for the feedback @stevegs and @TobiasBg... re: str_contains, in the latest patch I use substr_count( $str, '/' ) to ensure the string contains one and only one / which @jrf suggested on the PR. What do you think?

[TobiasBg]

Thanks for the link to the PR! With the weird things that are getting passed to the function, return $str; makes more sense than return 0; then, indeed.

And of course, that new substr_count check is what we actually want here. Nice catch!

One more thing: string should then be added to the @return docblock. And both the @param and @return should maybe get a description.

[stevegs]

Replying to adamsilverstein:

In 54385.10.diff​

• Cleaned up strings, doc blocks a bit.
• Improved test case structure
• Add some new test cases for typical values after researching exif values that are passed to the function in our use.

props @jrf for feedback on the GitHub draft PR.

Thanks for the feedback @stevegs and @TobiasBg... re: str_contains, in the latest patch I use substr_count( $str, '/' ) to ensure the string contains one and only one / which @jrf suggested on the PR. What do you think?

This seems to be the most elegant solution. I've checked that substr_count('2//5', '/') returns 2, as does substr_count('2/24/5', '/') - so it will catch more than one / whether consecutive or not. And it's backward compatible with PHP7.

Still there in the latest version...

removed in ​https://github.com/WordPress/wordpress-develop/pull/1841/commits/be0030df033a7d82c023fa88bbae7f4441f80a49

[adamsilverstein]

One more thing: string should then be added to the @return docblock. And both the @param and @return should maybe get a description.

Good suggestions, I'll add that.

[adamsilverstein]

54385.11.diff​

• Address latest feedback from PR
• Handle null or empty array input values properly, add test cases to demonstrate
• Improve test case naming, removing duplicate keys

[adamsilverstein]

In 54385.12.diff​, improve docblock:

• Add string to return types.
• Document input string.

[adamsilverstein]

Marking this as commit pending any final feedback, we should get this in before beta 1 so it can get wider testing.

[TobiasBg]

Looks good to me. Thanks!
One minor formatting thing: There are two empty lines before the data provider function.

[stevegs]

Replying to adamsilverstein:

In 54385.12.diff​, improve docblock:

• Add string to return types.
• Document input string.

Just allowing a string to be returned will not, on its own, fix the original error I reported: namely line 844 of image.php expects a numeric. So it would become the responsibility of the calling function to cast appropriately.

My Olympus camera can report a shutter speed in several formats, eg. 1/60 (as normally expected, for speeds significantly <1sec), 1/2.5 (a hybrid fraction for speeds near 1sec) and 4" (double-quote appended and no foreslash for >1sec). The first two would be returned after performing the division function; the last would be returned as a string - so that must be catered for.

Although division by zero must be trapped, if both $numerator and $denominator are numeric, a number should be returned IMO. Question is what? Mathematically, it should be a very large number - and the sign of the numerator should be preserved - eg. gmp_sign($numerator) * 1e9 would be a reasonable approximation. Alternative, but not mathematically correct, returns could be 0 (zero) or simply $numerator (ie. $denominator is set to 1). Though I suspect cameras that return (eg.) 0/0 have not recorded shutter speed, in which case a return of 0 would be appropriate (but certainly not a string or a NULL). Other EXIF information includes aperture, usually expressed in the form 'f/2.8'. But I have seen entries in EXIF tables of 'f/NaN'...

So should we step back and consider what we are trying to achieve with this function? Trying to be 'all things to all men' adds huge complication. But if image.php is just being used to generate various sizes of images for quicker downloading to web browsers (as it is in my case), no EXIF info is needed at all.

[adamsilverstein]

So should we step back and consider what we are trying to achieve with this function? Trying to be 'all things to all men' adds huge complication. But if image.php is just being used to generate various sizes of images for quicker downloading to web browsers (as it is in my case), no EXIF info is needed at all.

The main purpose of this function is handle image uploads by normalizing EXIF data. The main point of this ticket is to address the issue with images that cause PHP 8 to throw a fatal error.

The original function was written to return the passed value when it didn't look like a fraction and we probably need to stick to that behavior because in many cases EXIF data comes in an unexpected form.

Just allowing a string to be returned will not, on its own, fix the original error I reported: namely line 844 of image.php expects a numeric. So it would become the responsibility of the calling function to cast appropriately.

Good point! I dove so deep into fixing the function I removed that bit. I think the fix here is just casting the return to a float before rounding it.

Though I suspect cameras that return (eg.) 0/0 have not recorded shutter speed, in which case a return of 0 would be appropriate (but certainly not a string or a NULL).

Passing "0/0" will return "0/0" - the stored exif value. I don't think we should change that, at this point it es expected behavior.

[jrf]

After discussion with @adamsilverstein and with his permission, I've added a few commits to the ​draft PR on GitHub to stabilize the proposed solution further.

The solution I propose:

• Maintains BC for valid inputs.
• Fixes handling of invalid inputs to be in line with the documented (original) return type of int|float by returning 0.
• Safeguards the behaviour via tests for all possible ways code could flow through the function.
• Safeguards the BC of the wp_read_image_metadata() by adding some defensive coding around the calls to the wp_exif_frac2dec() function.

Let me know what you all think of the current proposed solution.

Thanks for the updates @jrfnl and your feedback on the approach. Now that you have pointed out Tests_Image_Meta I think it would make sense to add at least one of the two test images from the ticket (I'll double check that the issue is the same with both images). I'll work on that.

[adamsilverstein]

@jrf thanks for all your help on this ticket, turns out this little function is a good example for the larger conversation around input validation and PHP 8+.

54385.15.diff​ includes all the latest changes from the PR.

In addition, I added the Sugarloaf Mountain image and a new test to the Tests_Image_Meta tests, and verified that before the PR changes to wp_exif_frac2dec that test failed with a PHP fatal under PHP 8.

Nice to have the test coverage in Tests_Image_Meta, which demonstrates both that we have fixed the reported issue and that we haven't broken existing image meta extraction.

[hellofromTonya]

Great job everyone! Reassigning to me to prepare the commit.

[adamsilverstein]

Thanks @hellofromTonya!

[hellofromTonya]

In 52269:

Media: Fix TypeError and improve wp_exif_frac2dec() to only return int or float.

For certain images, wp_exif_frac2dec() unexpectedly returned a string instead of int or float. This can occur when an image is missing meta and calls the function with '0/0'. For those images, a fatal error was thrown on PHP 8.0+:

TypeError: round(): Argument #1 ($num) must be of type int|float, string given

Upon deeper review, inconsistent and unexpected results were returned from different types of input values passed to the function.

Changes are:

• Maintains backwards-compatibility for valid input values.
• Fixes handling of invalid input values by bailing out to return the documented type of int|float by returning 0.
• Improves the fractional conditional check.
• Improves the calculated fraction handling to ensure (a) the numerator and denominator are both numeric and (b) the denominator is not equal to zero.
• Safeguards the behavior via tests for all possible ways code could flow through the function.
• Safeguards the backwards-compatibility of the wp_read_image_metadata() by adding some defensive coding around the calls to the wp_exif_frac2dec() function.

These changes fix the fatal error and make the function more secure, stable, and predictable while maintaining backwards-compatibility for valid input values.

Follow-up to [6313], [9119], [22319], [28367], [45611], [47287].

Props adamsilverstein, jrf, peterwilsoncc, praem90, stevegs, tobiasbg.
Fixes #54385.

Committed via changeset https://core.trac.wordpress.org/changeset/52269.

[hellofromTonya]

In 52270:

Media: Replace tests/phpunit/data/images/sugar-mountain.jpg test image.

The test image included in [52269] was invalid at 0 bytes. This commit adds the correct test image.

Follow-up [52269].

See #54385.

[Gomle]

I am sorry if this does not fit in. I am not a developer.

But I can confirm this bug, as I am in the middle of migrating from another platform to Woocommerce.

I have run into this issue while importing thousands of products. Some of the productimages that are to be migrated have meta information, and they stall the import, but it don't tell me which ones it is.

There is about 18500 product images.

Any suggestions to how I best can solve it?

I have spent days not understanding why it stops - so any help would be appreciated, even if it is a "quick fix" for me right now, that won't do any good in the long term.

Will perhaps this fix it for importing as well?

Old line 844:

$meta['aperture'] = round( wp_exif_frac2dec( $exif['FNumber'] ), 2 );

New line 844:

$meta['aperture'] = round( (float) wp_exif_frac2dec( $exif['FNumber'] ), 2 );

I am running WP 5.8.2, and Woocommerce 5.9.0
Kadence theme.
All other plugins are deactivated for the sake of trying to get the import to work.
NO luck so far.

[jrf]

@Gomle This is not the right place for support questions. As you can read in the thread above, this issue has been fixed and the fix will be included in WP 5.9.

So the short of it is:

• Either try your import with the WP 5.9 beta version (which includes the patch and will be released later today).
• Or wait for WP 5.9 to be released, upgrade and do the import then.
• Or try your hand at applying the real patch - [52269] - to your current install.

[stevegs]

Replying to jrf:

@Gomle This is not the right place for support questions. As you can read in the thread above, this issue has been fixed and the fix will be included in WP 5.9.

So the short of it is:

• Either try your import with the WP 5.9 beta version (which includes the patch and will be released later today).
• Or wait for WP 5.9 to be released, upgrade and do the import then.
• Or try your hand at applying the real patch - [52269] - to your current install.

@Gomle: Two other options if you don't want to mess with your WP core files:

• Roll back to PHP 7.4 for now if your system administrator allows it. That version of PHP has not yet been classed as 'obsolete'
• Import your images into any photo editing software that strips out Exif info (eg. Paintshop Pro) and then upload.

I've looked at one of your images: the F-number comes back as NaN (ie. Not a Number), which got returned as a string before this fix. This causes PHP8 to (correctly) throw a wobbly: PHP 7.4 ignores the error.

[Gomle]

I fully understand. Sorry, and thanks anyway. Keep up the insanely good work you all are doing. At least I represent a huge part of the community being the storeowner that I am. And I'm sure I speak for everyone when I say that we all greatly appreciate all your hard work.
I'm out :)