Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#54637 closed defect (bug) (fixed)

wp_read_image_metadata() sends `false` instead of `array` for `$iptc` and `$exif` parameters to `wp_read_image_metadata` filter

Reported by: volodymyrkolesnykov's profile volodymyrkolesnykov Owned by: kirasong's profile kirasong
Milestone: 6.0 Priority: normal
Severity: normal Version: 5.8.2
Component: Media Keywords: has-patch needs-testing
Focuses: Cc:

Description

According to the documentation (https://developer.wordpress.org/reference/hooks/wp_read_image_metadata/), the exif_read_metadata filter expects its $iptc and $exif parameters to be arrays.

However, under some circumstances wp_read_image_metadata() function passes false instead of arrays.

This happens, for example, when exif_read_data() fails to parse metadata, in which case it returns false (see https://www.php.net/manual/en/function.exif-read-data.php).

https://github.com/WordPress/WordPress/blob/5.8.2/wp-admin/includes/image.php#L808

The shape of the returned value is never checked, WP just uses empty() function to see if specific keys are set (empty() will return false for scalars).

Finally, that false is passed to the filter: https://github.com/WordPress/WordPress/blob/5.8.2/wp-admin/includes/image.php#L894

The same story is with iptcparse(): https://github.com/WordPress/WordPress/blob/5.8.2/wp-admin/includes/image.php#L740, https://www.php.net/manual/en/function.iptcparse.php

I see two possible ways to fix this:

  1. Ensure that $iptc and $exif are arrays — check the return value of the parsing functions, or
  2. Update the documentation/PHP doc blocks to mention that those parameters can be false.

I can submit a patch for this issue, just not sure which of the solutions is preferred.

Attachments (2)

thumbnail_villarreal.jpg (56.1 KB) - added by volodymyrkolesnykov 2 years ago.
The described situation can be reproduced with this file (make sure PHP is compiled with --enable-exif)
20220404-0B6A9801.jpg (5.7 MB) - added by kirasong 2 years ago.
File to test empty IPTC

Change History (12)

@volodymyrkolesnykov
2 years ago

The described situation can be reproduced with this file (make sure PHP is compiled with --enable-exif)

#1 @sabernhardt
2 years ago

  • Component changed from General to Media

#2 in reply to: ↑ description ; follow-up: @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.0

Hi there, welcome back to WordPress Trac! Thanks for the ticket.

Replying to volodymyrkolesnykov:

I see two possible ways to fix this:

  1. Ensure that $iptc and $exif are arrays — check the return value of the parsing functions, or
  2. Update the documentation/PHP doc blocks to mention that those parameters can be false.

I can submit a patch for this issue, just not sure which of the solutions is preferred.

I think I'd prefer the first option here, seems in line with a similar fix for wp_generate_attachment_metadata() in [51162] / #52603 (more details in comment:2:ticket:52603).

This ticket was mentioned in PR #2068 on WordPress/wordpress-develop by sjinks.


2 years ago
#3

  • Keywords has-patch added

This PR addresses the issue when the wp_read_image_metadata hook receives incorrect (according to the documentation) arguments (false instead of array).

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

#4 in reply to: ↑ 2 @volodymyrkolesnykov
2 years ago

Replying to SergeyBiryukov:

I have submitted a patch against WP trunk. Does it make sense to backport it to 5.8/5.7/etc branches?

#5 @SergeyBiryukov
2 years ago

Thanks for the PR!

At this point, as per the WordPress 5.9 release schedule, I think only issues introduced during the 5.9 cycle or dedicated tasks (e.g. the About page, documentation or test improvements) are being addressed. However, this looks good to commit once trunk is open for general bug fixes again (most likely in early January).

Backporting to older branches generally applies to security fixes only.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

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


2 years ago

#7 @sumitsingh
2 years ago

  • Keywords needs-testing added

#8 @kirasong
2 years ago

  • Owner set to mikeschroder
  • Status changed from new to assigned

I'm going to test this for commit, to see if we can land it before RC.

@kirasong
2 years ago

File to test empty IPTC

#9 @kirasong
2 years ago

Okay! I looked into this and wasn't able to reproduce it with exif with the image provided.

Despite the errors, exif_read_data() still returned an array.
It's possible it does so in fewer cases at this point.

I was, however, able to reproduce it in IPTC with an image that I manually created with no IPTC.
EXIF is strangely harder to avoid, since even tools built to remove it, or save without it seem to leave the section intact. If anyone manages to get an image without it, please let me know.

Regardless, because both functions can return false (based on docs, and my skim of the source for exif), and this patch solves that issue when manually tested, it seems safer to force the return values to array().

Going to go ahead and get this ready for commit.

#10 @kirasong
2 years ago

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

In 53303:

Media: Ensure wp_read_image_metadata filter returns array for $iptc and $exif.

Makes the behavior of the filter lines up with its documentation.
Previously, both $iptc and $exif could return false when exif_read_data() or iptcparse() failed.

Now, if those functions do not return an array, the results are explicitly set to array().

Props volodymyrkolesnykov, SergeyBiryukov, sabernhardt, sumitsingh, mikeschroder.
Fixes #54637.

Note: See TracTickets for help on using tickets.