#52826 closed defect (bug) (fixed)
New wp_getimagesize() causing unexpected failures
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.7.1 | Priority: | normal |
Severity: | critical | Version: | 5.7 |
Component: | Media | Keywords: | has-patch fixed-major needs-testing |
Focuses: | Cc: |
Description
The new function wp_getimagesize()
introduced in WordPress 5.7 is causing JPEG images that have more than a basic amount of EXIF data to return false where the previous implementation using getimagesize()
returned a valid array.
The function was introduced in #49889 and forces passing a second parameter that the getimagesize()
function expects as a reference, which impacts the actual behavior - previously, most calls did not pass the 2nd param, and internally, PHP executes different code now because an array ref is passed. In addition, the array is discarded because it wasn’t passed from the WordPress code.
The second parameter is also noted as only supporting JFIF files. https://www.php.net/manual/en/function.getimagesize.php
Because the second parameter for getimagesize()
is passed by reference, only passing that second parameter when on has been provided to wp_getimagesize()
would be preferable.
I've seen at least one customer report of this issue where the size data comes back as false where it was previously working, manifesting in problems with cropping images and missing information in the attachment metadata.
Attachments (5)
Change History (37)
@
4 years ago
Do not pass the second parameter unless it is explicitly provided when calling wp_getimagesize
#2
in reply to:
↑ 1
@
4 years ago
- Severity changed from normal to critical
Thank you, @SergeyBiryukov, for your consideration of this ticket for a 5.7.1 release.
I added a patch with changes that should solve the problem. I'm happy to adjust and provide a new one if there is any feedback.
I'm also increasing the severity of this issue to critical because we have observed that downgrading to the previous WordPress version (5.6.2) will only fix subsequently uploaded images. In testing, images uploaded while 5.7 was the active version are not retroactively updated after the downgrade. I believe this is because the height, width, and additional attachment metadata are written to the database on upload.
It is worth noting that the patch I provided also does not retroactively repair missing metadata for any affected images.
This ticket was mentioned in Slack in #core by bandonrandon. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.
4 years ago
#6
@
4 years ago
- Keywords has-patch removed
Hi @terriann, thanks for your patch!
However, there is just a small issue with it, if we refer to the getimagesize
function, the
hint type is
array &$image_info = null
, so we should keep consistency and use the same hint.
Furthermore, it doesn't require to add extra condition to check if it's null or not.
#8
@
4 years ago
@Mista-Flo
Furthermore, it doesn't require to add extra condition to check if it's null or not.
Unfortunately, that's not true according to our testing.
With your version of the patch:
<?php # wp post meta get 15 _wp_attachment_metadata array ( 'filesize' => 1460536, )
With @terriann's:
<?php # wp post meta get 16 _wp_attachment_metadata array ( 'width' => 1333, 'height' => 2000, 'file' => '2021/03/has-exif-4.jpg', 'sizes' => array ( ), 'image_meta' => array ( 'aperture' => '0', 'credit' => '', 'camera' => '', 'caption' => '', 'created_timestamp' => '0', 'copyright' => '', 'focal_length' => '0', 'iso' => '0', 'shutter_speed' => '0', 'title' => '', 'orientation' => '0', 'keywords' => array ( ), ), 'filesize' => 1460536, )
To clarify
The issue is only present when the following conditions are true:
- The file in question is using a custom streamWrapper implementation. e.g.
mycustomstream://path-to-file.jpg
- The file contains EXIF data
For anyone interested, here are more links:
We use a different Stream Wrapper but AWS is probably the easiest to use for testing/confirming.
@
4 years ago
There is one place where a second parameter is being passed to wp_getimagesize()
but that $info
variable is never initialized which lead to problems parsing EXIF data into the caption field
#9
@
4 years ago
- Owner set to whyisjake
- Resolution set to fixed
- Status changed from new to closed
In 50552:
#10
@
4 years ago
- Keywords dev-feedback added; needs-testing removed
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for consideration for WordPress 5.7.1.
#11
@
4 years ago
- Keywords commit fixed-major added; dev-feedback removed
[50552] looks good to backport.
#13
@
4 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
[50553] does fix EXIF not being filled but doesn't fix the originally reported issue.
#14
follow-up:
↓ 16
@
4 years ago
- Keywords needs-refresh added
As @rinatkhaziev noted above, the current patch is not fixing the original issue (with the calls that had just one argument).
That is because the PHP function getimagesize is difficult to wrap:
- the second parameter is passed by reference, and also optional
- the source code uses argument counting to determine the presence of the 2nd argument
- when the second argument is passed, and a JPEG is involved, the underlying source code executes different code internally here and then here so it's important to avoid changing what actually executes
To match the signature, and the intent of the existing calls, the wrapper here will need to conditionally call with one or two parameters, as in attachment:trac-52826.diff
#15
@
4 years ago
- Keywords commit fixed-major removed
Thanks @RogerTheriault and @rinatkhaziev: just modifying the keywords to avoid confusion now this has been reopened and allow follow up.
#16
in reply to:
↑ 14
@
4 years ago
Replying to RogerTheriault:
To match the signature, and the intent of the existing calls, the wrapper here will need to conditionally call with one or two parameters, as in attachment:trac-52826.diff
Right, thinking trac-52826.diff looks better and is easier to read/understand imho. It would also remove the confusing/hard-to-read array &$image_info = null
(which reads something like "by default the optional array $imageinfo is not an array").
@
4 years ago
Addressing consistency in parameter count passed through to getimagesize() when compared to 5.6.2
#17
@
4 years ago
I've submitted a new patch, based on SVN trunk's current status, which includes the change that was already committed: attachment:52826.4.diff. I believe this new change addresses many of the concerns here.
This solution moves away from using an is_null()
check to determine whether to pass the second parameter to PHP's native getimagesize()
. A colleague pointed out that func_num_args()
could also be used to see if the second parameter is explicitly provided.
It looks like there is a precedent in core for using func_num_args()
for both backward compatibility and shimming. Examples: /wp-includes/option.php:92 and /wp-includes/class-wp-rewrite.php:1787
Because the second parameter is a reference, my understanding is that it shouldn't need to be initialized in advance of calling wp_getimagesize( $file, $info )
much like how one wouldn't typically initilize $matches
before calling preg_match( $pattern, $subject, $matches )
.
The change in this new patch would support calling getimagesize()
with the same parameter count it had been called with in WordPress 5.6.2 and earlier.
I've tested this in the environment where we first detected the issue, which streams the files from the uploads directory in the manner @rinatkhaziev described. Images that were not storing attachment metadata for width and height after the 5.7 upgrades are now storing width, height and correctly parsing relevant EXIF data for title, caption, etc.
#18
follow-up:
↓ 19
@
4 years ago
- Keywords needs-unit-tests added
@azaozz if 52826.5.diff is what you had in mind, it looks logical to me.
It would be good to test this to ensure the data is treated as expected given comment 14. The WP test suite has a stream implementation that can be used with stream_wrapper_register( 'wptest', 'WP_Test_Stream' );
See Tests_Image_Editor_Imagick::test_streams()
for an example of how to use it.
#19
in reply to:
↑ 18
@
4 years ago
Replying to peterwilsoncc:
if 52826.5.diff is what you had in mind, it looks logical to me.
Yes, 52826.5.diff looks good imho. As far as I see both is_null()
and func_num_args()
are used elsewhere in core but the latter seems better/more suitable in this case.
This ticket was mentioned in PR #1121 on WordPress/wordpress-develop by peterwilsoncc.
4 years ago
#20
- Keywords has-unit-tests added; needs-refresh needs-unit-tests removed
https://core.trac.wordpress.org/ticket/52826
trying to write some tests for this, maybe unsuccessfully.
#21
@
4 years ago
I tried adding some unit tests in the linked pull request but either:
- I am doing something wrong with
WP_Test_Stream
within the test suite (likely), or, WP_Test_Stream
is super basic and doesn't support exif data (which seems unlikely)
If someone could drop some pointers on the PR, that would be most helpful
--
Also, I think the $info
definition that is removed in 52826.5.diff is still required so I've added that back in to the linked pull request, otherwise the functional code is identical to @terriann's
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
4 years ago
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
4 years ago
#25
@
4 years ago
- Keywords fixed-major dev-feedback added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening to commit [50586] to the 5.7 branch.
Although it's not required until the release candidate, for peace of mind it would be helpful if I could get a second committers sign-off for the 5.7 branch.
#26
@
4 years ago
- Keywords needs-testing added; has-unit-tests dev-feedback removed
Removing has-unit-tests
as unable to add tests for streams. Why? Getting the metadata for Exif requires seeking. The WP_Test_Stream
currently does not support seeking.
Errors noted:
getimagesize(): stream does not support seeking exif_read_data(): stream does not support seeking
For now, manual testing is required to validate the patch.
Ticket #52922 will extend WP_Test_Stream
for seeking capability. Once completed, then automated tests can be added to both wp_getimagesize
and wp_read_image_metadata
to valid streams.
Hi there, welcome back to WordPress Trac!
Thanks for the report, moving to 5.7.1 for investigation.