WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 8 months ago

Last modified 8 months ago

#52826 closed defect (bug) (fixed)

New wp_getimagesize() causing unexpected failures

Reported by: terriann Owned by: whyisjake
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)

trac-52826.diff (1.3 KB) - added by terriann 9 months ago.
Do not pass the second parameter unless it is explicitly provided when calling wp_getimagesize
52826.2.diff (1.3 KB) - added by Mista-Flo 9 months ago.
Properly fix the issue with good type hint
52826.3 (1.3 KB) - added by hellofromTonya 9 months ago.
Improvements to DocBlock for consistency.
52826.4.diff (1.5 KB) - added by terriann 9 months 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
52826.5.diff (1.4 KB) - added by terriann 9 months ago.
Addressing consistency in parameter count passed through to getimagesize() when compared to 5.6.2

Download all attachments as: .zip

Change History (37)

#1 follow-up: @SergeyBiryukov
9 months ago

  • Milestone changed from Awaiting Review to 5.7.1

Hi there, welcome back to WordPress Trac!

Thanks for the report, moving to 5.7.1 for investigation.

@terriann
9 months ago

Do not pass the second parameter unless it is explicitly provided when calling wp_getimagesize

#2 in reply to: ↑ 1 @terriann
9 months 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.


9 months ago

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


9 months ago

@Mista-Flo
9 months ago

Properly fix the issue with good type hint

#5 @hellofromTonya
9 months ago

  • Keywords has-patch added

#6 @Mista-Flo
9 months 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.

#7 @Mista-Flo
9 months ago

  • Keywords has-patch needs-testing added

@hellofromTonya
9 months ago

Improvements to DocBlock for consistency.

#8 @rinatkhaziev
9 months 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.

The underlying PHP source is likely here

@terriann
9 months 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 @whyisjake
9 months ago

  • Owner set to whyisjake
  • Resolution set to fixed
  • Status changed from new to closed

In 50552:

Media: Pass the appropriate reference into wp_getimagesize.

With changes that were introduced in [49889] the second parameter for getimagesize() function is expecting a a reference.

Previously, most calls did not pass the 2nd param, and as a result, we are getting unexpected results.

This was only a problem with applications that are using a custom stream wrapper, and the image contained EXIF data.

For more see:

Fixes #52826.
Props terriann, SergeyBiryukov, Mista-Flo, hellofromTonya, rinatkhaziev, whyisjake.

#10 @whyisjake
9 months 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 @SergeyBiryukov
9 months ago

  • Keywords commit fixed-major added; dev-feedback removed

[50552] looks good to backport.

#12 @peterwilsoncc
9 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 50553:

Media: Pass the appropriate reference into wp_getimagesize.

With changes that were introduced in #49889 the second parameter for getimagesize() function is expecting a a reference.

Previously, most calls did not pass the 2nd param, and as a result, we are getting unexpected results.

This was only a problem with applications that are using a custom stream wrapper, and the image contained EXIF data.

For more see:

https://github.com/humanmade/S3-Uploads/issues/496
https://github.com/aws/aws-sdk-php/issues/1923

Fixes #52826.
Merges [50552] to the 5.7 branch.
Props terriann, SergeyBiryukov, Mista-Flo, hellofromTonya, rinatkhaziev, whyisjake.

#13 @rinatkhaziev
9 months 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.

Last edited 9 months ago by SergeyBiryukov (previous) (diff)

#14 follow-up: @RogerTheriault
9 months 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 @peterwilsoncc
9 months 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 @azaozz
9 months 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").

@terriann
9 months ago

Addressing consistency in parameter count passed through to getimagesize() when compared to 5.6.2

#17 @terriann
9 months 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: @peterwilsoncc
9 months 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 @azaozz
8 months 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.


8 months ago

  • 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 @peterwilsoncc
8 months 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.


8 months ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


8 months ago

#24 @peterwilsoncc
8 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 50586:

Media: Conditionally pass 2nd parameter to getimagesize().

In the wrapper function wp_getimagesize() check if the second parameter was passed before sending it to the PHP function getimagesize().

The PHP function has a different execution path depending on the number of parameters passed, this ensures the wrapper function follows the appropriate path.

Follow up to [50552].
Props azaozz, hellofromtonya, Mista-Flo, peterwilsoncc, rinatkhaziev, RogerTheriault, SergeyBiryukov, terriann, whyisjake.
Fixes #52826.

#25 @peterwilsoncc
8 months 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 @hellofromTonya
8 months 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.

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


8 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


8 months ago

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


8 months ago

#30 @peterwilsoncc
8 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 50662:

Media: Conditionally pass 2nd parameter to getimagesize().

In the wrapper function wp_getimagesize() check if the second parameter was passed before sending it to the PHP function getimagesize().

The PHP function has a different execution path depending on the number of parameters passed, this ensures the wrapper function follows the appropriate path.

Follow up to [50552].
Props azaozz, hellofromtonya, Mista-Flo, peterwilsoncc, rinatkhaziev, RogerTheriault, SergeyBiryukov, terriann, whyisjake.
Merges [50586] to the 5.7 branch.
Fixes #52826.

#32 @peterwilsoncc
8 months ago

In 50771:

Build/Test Tools: Add seeking support to stream test library.

Introduces seeking to the WP_Test_Stream stream wrapper. This allows the testing of wp_getimagesize() and wp_read_image_metadata() among others. Includes tests for the latter.

Props hellofromTonya.
Fixes #52922.
See #52826.

Note: See TracTickets for help on using tickets.