#54385 closed defect (bug) (fixed)
Fatal error uploading media on PHP8
Reported by: | stevegs | Owned by: | hellofromTonya |
---|---|---|---|
Milestone: | 5.9 | Priority: | normal |
Severity: | normal | Version: | 2.5 |
Component: | Media | Keywords: | php8 has-patch needs-dev-note has-unit-tests commit |
Focuses: | Cc: |
Description
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')
Attachments (18)
Change History (71)
#1
@
3 years ago
- Component changed from General to Media
- Keywords needs-patch added
- Milestone changed from Awaiting Review to 5.9
- Version set to 2.5
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.
#2
follow-up:
↓ 3
@
3 years ago
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.
#3
in reply to:
↑ 2
@
3 years ago
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.
#4
@
3 years ago
- Owner set to adamsilverstein
- Status changed from new to assigned
Hi @stevegs, thanks for reporting this issue. I was able to reproduce this issue and will work on a fix.
This ticket was mentioned in PR #1841 on WordPress/wordpress-develop by adamsilverstein.
3 years ago
#5
- Keywords has-patch added; needs-patch removed
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
#6
@
3 years ago
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 callinground
- Update doc block for
wp_exif_frac2dec
to clarify that it returns a float or a string
#7
@
3 years ago
This looks like the same issue that was reported in https://core.trac.wordpress.org/ticket/54009 by @quantumzen.
#8
@
3 years ago
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.
#9
@
3 years ago
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?
#10
@
3 years ago
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.
#11
@
3 years ago
- ensure
wp_exif_frac2dec
always returns a float or int, matching the docblock description
#14
follow-up:
↓ 16
@
3 years ago
Sugarloaf Mountain.jpg is a second sample image from @quantumzen that reproduces the issue (from https://core.trac.wordpress.org/ticket/54009).
adamsilverstein commented on PR #1841:
3 years ago
#15
@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.
#16
in reply to:
↑ 14
@
3 years ago
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.
3 years ago
#17
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.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
3 years ago
adamsilverstein commented on PR #1841:
3 years ago
#19
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!
#20
@
3 years ago
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.
#21
@
3 years ago
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.
This ticket was mentioned in Slack in #core-media by sabernhardt. View the logs.
3 years ago
#23
@
3 years ago
- Keywords needs-dev-note has-unit-tests added
- 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").
This ticket was mentioned in Slack in #core-media by adamsilverstein. View the logs.
3 years ago
#26
follow-up:
↓ 27
@
3 years ago
Some suggestions:
- Use
str_contains()
(from PHP 8.0, but polyfilled in [52039]) instead of thatstrpos()
- Remove an extra empty line
- remove unnecessary
global $wpdb;
#27
in reply to:
↑ 26
;
follow-up:
↓ 28
@
3 years ago
Replying to TobiasBg:
Some suggestions:
- Use
str_contains()
(from PHP 8.0, but polyfilled in [52039]) instead of thatstrpos()
- 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.
#28
in reply to:
↑ 27
@
3 years ago
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.
This ticket was mentioned in Slack in #core-media by joedolson. View the logs.
3 years ago
#30
@
3 years ago
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
#31
follow-up:
↓ 33
@
3 years ago
- 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?
#32
@
3 years ago
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.
#33
in reply to:
↑ 31
@
3 years ago
Replying to adamsilverstein:
- 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 usesubstr_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.
adamsilverstein commented on PR #1841:
3 years ago
#34
Still there in the latest version...
#35
@
3 years ago
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.
#36
@
3 years ago
- 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
#37
follow-up:
↓ 40
@
3 years ago
In 54385.12.diff, improve docblock:
- Add string to return types.
- Document input string.
#38
@
3 years ago
- Keywords commit added
Marking this as commit pending any final feedback, we should get this in before beta 1 so it can get wider testing.
#39
@
3 years ago
Looks good to me. Thanks!
One minor formatting thing: There are two empty lines before the data provider function.
#40
in reply to:
↑ 37
@
3 years ago
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.
#41
@
3 years ago
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.
#42
@
3 years ago
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 returning0
. - 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 thewp_exif_frac2dec()
function.
Let me know what you all think of the current proposed solution.
adamsilverstein commented on PR #1841:
3 years ago
#43
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.
#44
@
3 years ago
@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.
#45
@
3 years ago
- Owner changed from adamsilverstein to hellofromTonya
- Status changed from assigned to reviewing
Great job everyone! Reassigning to me to prepare the commit.
hellofromtonya commented on PR #1841:
3 years ago
#48
Committed via changeset https://core.trac.wordpress.org/changeset/52269.
#50
@
3 years ago
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.
Here is a couple of the many images that stalls the import:
https://www.dekorama.no/img/p/5/5/0/5/5505.jpg
https://www.dekorama.no/img/p/5/5/2/5/5525.jpg
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.
#51
follow-up:
↓ 52
@
3 years ago
@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.
#52
in reply to:
↑ 51
@
3 years ago
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.
#53
@
3 years ago
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 :)
Screenshot of PHP errors; Image file causing these errors.