New wp_getimagesize() causing unexpected failures

[terriann]

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.

[SergeyBiryukov]

Hi there, welcome back to WordPress Trac!

Thanks for the report, moving to 5.7.1 for investigation.

[terriann]

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

[terriann]

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.

[Mista-Flo]

Properly fix the issue with good type hint

[Mista-Flo]

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.

[hellofromTonya]

Improvements to DocBlock for consistency.

[rinatkhaziev]

@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:

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

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]

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

[whyisjake]

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:

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

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

[whyisjake]

Reopening for consideration for WordPress 5.7.1.

[SergeyBiryukov]

[50552] looks good to backport.

[peterwilsoncc]

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.

[rinatkhaziev]

[50553] does fix EXIF not being filled but doesn't fix the originally reported issue.

[RogerTheriault]

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​

[peterwilsoncc]

Thanks @RogerTheriault and @rinatkhaziev: just modifying the keywords to avoid confusion now this has been reopened and allow follow up.

[azaozz]

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]

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

[terriann]

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.

[peterwilsoncc]

@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.

[azaozz]

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.

https://core.trac.wordpress.org/ticket/52826

trying to write some tests for this, maybe unsuccessfully.

[peterwilsoncc]

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

[peterwilsoncc]

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.

[peterwilsoncc]

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.

[hellofromTonya]

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.

[peterwilsoncc]

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.

[prbot]

​peterwilsoncc commented on ​PR #1121:

Merged https://core.trac.wordpress.org/changeset/50586

[peterwilsoncc]

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.