From 8aa625b795835f430b8b47aaa919f187df1707d6 Mon Sep 17 00:00:00 2001 From: Tonya Mork Date: Mon, 29 Nov 2021 19:34:51 +0000 Subject: [PATCH] Media: Fix `TypeError` and improve `wp_exif_frac2dec()` to only return `int` or `float`. For certain images, `wp_exif_frac2dec()` unexpectedly returned a string instead of `int` or `float`. This can occur when an image is missing meta and calls the function with `'0/0'`. For those images, a fatal error was thrown on PHP 8.0+: {{{ TypeError: round(): Argument #1 ($num) must be of type int|float, string given }}} Upon deeper review, inconsistent and unexpected results were returned from different types of input values passed to the function. Changes are: * Maintains backwards-compatibility for valid input values. * Fixes handling of invalid input values by bailing out to return the documented type of `int|float` by returning `0`. * Improves the fractional conditional check. * Improves the calculated fraction handling to ensure (a) the numerator and denominator are both numeric and (b) the denominator is not equal to zero. * Safeguards the behavior via tests for all possible ways code could flow through the function. * Safeguards the backwards-compatibility of the `wp_read_image_metadata()` by adding some defensive coding around the calls to the `wp_exif_frac2dec()` function. These changes fix the fatal error and make the function more secure, stable, and predictable while maintaining backwards-compatibility for valid input values. Follow-up to [6313], [9119], [22319], [28367], [45611], [47287]. Props adamsilverstein, jrf, peterwilsoncc, praem90, stevegs, tobiasbg. Fixes #54385. git-svn-id: https://develop.svn.wordpress.org/trunk@52269 602fd350-edb4-49c9-b593-d223f7449a82 --- src/wp-admin/includes/image.php | 47 +++-- .../data/images/sugarloaf-mountain.jpg | 0 tests/phpunit/tests/image/functions.php | 126 ++++++++++++++ tests/phpunit/tests/image/meta.php | 164 ++++++++++-------- 4 files changed, 254 insertions(+), 83 deletions(-) create mode 100644 tests/phpunit/data/images/sugarloaf-mountain.jpg diff --git a/src/wp-admin/includes/image.php b/src/wp-admin/includes/image.php index d8d5d5b34b468..9cab18a22a6b8 100644 --- a/src/wp-admin/includes/image.php +++ b/src/wp-admin/includes/image.php @@ -646,19 +646,40 @@ function wp_generate_attachment_metadata( $attachment_id, $file ) { * * @since 2.5.0 * - * @param string $str - * @return int|float + * @param string $str Fraction string. + * @return int|float Returns calculated fraction or integer 0 on invalid input. */ function wp_exif_frac2dec( $str ) { - if ( false === strpos( $str, '/' ) ) { - return $str; + if ( ! is_scalar( $str ) || is_bool( $str ) ) { + return 0; + } + + if ( ! is_string( $str ) ) { + return $str; // This can only be an integer or float, so this is fine. + } + + // Fractions passed as a string must contain a single `/`. + if ( substr_count( $str, '/' ) !== 1 ) { + if ( is_numeric( $str ) ) { + return (float) $str; + } + + return 0; } list( $numerator, $denominator ) = explode( '/', $str ); - if ( ! empty( $denominator ) ) { - return $numerator / $denominator; + + // Both the numerator and the denominator must be numbers. + if ( ! is_numeric( $numerator ) || ! is_numeric( $denominator ) ) { + return 0; + } + + // The denominator must not be zero. + if ( 0 == $denominator ) { // phpcs:ignore WordPress.PHP.StrictComparisons.LooseComparison -- Deliberate loose comparison. + return 0; } - return $str; + + return $numerator / $denominator; } /** @@ -840,7 +861,7 @@ function wp_read_image_metadata( $file ) { if ( empty( $meta['copyright'] ) && ! empty( $exif['Copyright'] ) ) { $meta['copyright'] = trim( $exif['Copyright'] ); } - if ( ! empty( $exif['FNumber'] ) ) { + if ( ! empty( $exif['FNumber'] ) && is_scalar( $exif['FNumber'] ) ) { $meta['aperture'] = round( wp_exif_frac2dec( $exif['FNumber'] ), 2 ); } if ( ! empty( $exif['Model'] ) ) { @@ -850,14 +871,20 @@ function wp_read_image_metadata( $file ) { $meta['created_timestamp'] = wp_exif_date2ts( $exif['DateTimeDigitized'] ); } if ( ! empty( $exif['FocalLength'] ) ) { - $meta['focal_length'] = (string) wp_exif_frac2dec( $exif['FocalLength'] ); + $meta['focal_length'] = (string) $exif['FocalLength']; + if ( is_scalar( $exif['FocalLength'] ) ) { + $meta['focal_length'] = (string) wp_exif_frac2dec( $exif['FocalLength'] ); + } } if ( ! empty( $exif['ISOSpeedRatings'] ) ) { $meta['iso'] = is_array( $exif['ISOSpeedRatings'] ) ? reset( $exif['ISOSpeedRatings'] ) : $exif['ISOSpeedRatings']; $meta['iso'] = trim( $meta['iso'] ); } if ( ! empty( $exif['ExposureTime'] ) ) { - $meta['shutter_speed'] = (string) wp_exif_frac2dec( $exif['ExposureTime'] ); + $meta['shutter_speed'] = (string) $exif['ExposureTime']; + if ( is_scalar( $exif['ExposureTime'] ) ) { + $meta['shutter_speed'] = (string) wp_exif_frac2dec( $exif['ExposureTime'] ); + } } if ( ! empty( $exif['Orientation'] ) ) { $meta['orientation'] = $exif['Orientation']; diff --git a/tests/phpunit/data/images/sugarloaf-mountain.jpg b/tests/phpunit/data/images/sugarloaf-mountain.jpg new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/phpunit/tests/image/functions.php b/tests/phpunit/tests/image/functions.php index 26863a900407a..8cd42a13b5529 100644 --- a/tests/phpunit/tests/image/functions.php +++ b/tests/phpunit/tests/image/functions.php @@ -700,4 +700,130 @@ public function test_pdf_preview_doesnt_overwrite_existing_jpeg() { unlink( $temp_dir . $size['file'] ); } } + + /** + * Test for wp_exif_frac2dec verified that it properly handles edge cases + * and always returns an int or float, or 0 for failures. + * + * @param mixed $fraction The fraction to convert. + * @param int|float $expect The expected result. + * + * @ticket 54385 + * @dataProvider data_wp_exif_frac2dec + * + * @covers ::wp_exif_frac2dec + */ + public function test_wp_exif_frac2dec( $fraction, $expect ) { + $this->assertSame( $expect, wp_exif_frac2dec( $fraction ) ); + } + + /** + * Data provider for testing `wp_exif_frac2dec()`. + * + * @return array + */ + public function data_wp_exif_frac2dec() { + return array( + 'invalid input: null' => array( + 'fraction' => null, + 'expect' => 0, + ), + 'invalid input: boolean true' => array( + 'fraction' => null, + 'expect' => 0, + ), + 'invalid input: empty array value' => array( + 'fraction' => array(), + 'expect' => 0, + ), + 'input is already integer' => array( + 'fraction' => 12, + 'expect' => 12, + ), + 'input is already float' => array( + 'fraction' => 10.123, + 'expect' => 10.123, + ), + 'string input is not a fraction - no slash, not numeric' => array( + 'fraction' => '123notafraction', + 'expect' => 0, + ), + 'string input is not a fraction - no slash, numeric integer' => array( + 'fraction' => '48', + 'expect' => 48.0, + ), + 'string input is not a fraction - no slash, numeric integer (integer 0)' => array( + 'fraction' => '0', + 'expect' => 0.0, + ), + 'string input is not a fraction - no slash, octal numeric integer' => array( + 'fraction' => '010', + 'expect' => 10.0, + ), + 'string input is not a fraction - no slash, numeric float (float 0)' => array( + 'fraction' => '0.0', + 'expect' => 0.0, + ), + 'string input is not a fraction - no slash, numeric float (typical fnumber)' => array( + 'fraction' => '4.8', + 'expect' => 4.8, + ), + 'string input is not a fraction - more than 1 slash with text' => array( + 'fraction' => 'path/to/file', + 'expect' => 0, + ), + 'string input is not a fraction - more than 1 slash with numbers' => array( + 'fraction' => '1/2/3', + 'expect' => 0, + ), + 'string input is not a fraction - only a slash' => array( + 'fraction' => '/', + 'expect' => 0, + ), + 'string input is not a fraction - only slashes' => array( + 'fraction' => '///', + 'expect' => 0, + ), + 'string input is not a fraction - left/right is not numeric' => array( + 'fraction' => 'path/to', + 'expect' => 0, + ), + 'string input is not a fraction - left is not numeric' => array( + 'fraction' => 'path/10', + 'expect' => 0, + ), + 'string input is not a fraction - right is not numeric' => array( + 'fraction' => '0/abc', + 'expect' => 0, + ), + 'division by zero is prevented 1' => array( + 'fraction' => '0/0', + 'expect' => 0, + ), + 'division by zero is prevented 2' => array( + 'fraction' => '100/0.0', + 'expect' => 0, + ), + 'typical focal length' => array( + 'fraction' => '37 mm', + 'expect' => 0, + ), + 'typical exposure time' => array( + 'fraction' => '1/350', + 'expect' => 0.002857142857142857, + ), + 'valid fraction 1' => array( + 'fraction' => '50/100', + 'expect' => 0.5, + ), + 'valid fraction 2' => array( + 'fraction' => '25/100', + 'expect' => .25, + ), + 'valid fraction 3' => array( + 'fraction' => '4/2', + 'expect' => 2, + ), + ); + } } diff --git a/tests/phpunit/tests/image/meta.php b/tests/phpunit/tests/image/meta.php index 39e68b33c3ce8..b362cf1f20472 100644 --- a/tests/phpunit/tests/image/meta.php +++ b/tests/phpunit/tests/image/meta.php @@ -32,32 +32,32 @@ public function test_exif_d70() { // Exif from a Nikon D70. $out = wp_read_image_metadata( DIR_TESTDATA . '/images/2004-07-22-DSC_0008.jpg' ); - $this->assertEquals( 6.3, $out['aperture'] ); - $this->assertSame( '', $out['credit'] ); - $this->assertSame( 'NIKON D70', $out['camera'] ); - $this->assertSame( '', $out['caption'] ); - $this->assertEquals( strtotime( '2004-07-22 17:14:59' ), $out['created_timestamp'] ); - $this->assertSame( '', $out['copyright'] ); - $this->assertEquals( 27, $out['focal_length'] ); - $this->assertEquals( 400, $out['iso'] ); - $this->assertEquals( 1 / 40, $out['shutter_speed'] ); - $this->assertSame( '', $out['title'] ); + $this->assertEquals( 6.3, $out['aperture'], 'Aperture value not equivalent' ); + $this->assertSame( '', $out['credit'], 'Credit value not the same' ); + $this->assertSame( 'NIKON D70', $out['camera'], 'Camera value not the same' ); + $this->assertSame( '', $out['caption'], 'Caption value not the same' ); + $this->assertEquals( strtotime( '2004-07-22 17:14:59' ), $out['created_timestamp'], 'Timestamp value not equivalent' ); + $this->assertSame( '', $out['copyright'], 'Copyright value not the same' ); + $this->assertEquals( 27, $out['focal_length'], 'Focal length value not equivalent' ); + $this->assertEquals( 400, $out['iso'], 'Iso value not equivalent' ); + $this->assertEquals( 1 / 40, $out['shutter_speed'], 'Shutter speed value not equivalent' ); + $this->assertSame( '', $out['title'], 'Title value not the same' ); } public function test_exif_d70_mf() { // Exif from a Nikon D70 - manual focus lens, so some data is unavailable. $out = wp_read_image_metadata( DIR_TESTDATA . '/images/2007-06-17DSC_4173.JPG' ); - $this->assertSame( '0', $out['aperture'] ); - $this->assertSame( '', $out['credit'] ); - $this->assertSame( 'NIKON D70', $out['camera'] ); - $this->assertSame( '', $out['caption'] ); - $this->assertEquals( strtotime( '2007-06-17 21:18:00' ), $out['created_timestamp'] ); - $this->assertSame( '', $out['copyright'] ); - $this->assertEquals( 0, $out['focal_length'] ); - $this->assertEquals( 0, $out['iso'] ); // Interesting - a Nikon bug? - $this->assertEquals( 1 / 500, $out['shutter_speed'] ); - $this->assertSame( '', $out['title'] ); + $this->assertSame( '0', $out['aperture'], 'Aperture value not the same' ); + $this->assertSame( '', $out['credit'], 'Credit value not the same' ); + $this->assertSame( 'NIKON D70', $out['camera'], 'Camera value not the same' ); + $this->assertSame( '', $out['caption'], 'Caption value not the same' ); + $this->assertEquals( strtotime( '2007-06-17 21:18:00' ), $out['created_timestamp'], 'Timestamp value not equivalent' ); + $this->assertSame( '', $out['copyright'], 'Copyright value not the same' ); + $this->assertEquals( 0, $out['focal_length'], 'Focal length value not equivalent' ); + $this->assertEquals( 0, $out['iso'], 'Iso value not equivalent' ); // Interesting - a Nikon bug? + $this->assertEquals( 1 / 500, $out['shutter_speed'], 'Shutter speed value not equivalent' ); + $this->assertSame( '', $out['title'], 'Title value not the same' ); // $this->assertSame( array( 'Flowers' ), $out['keywords'] ); } @@ -65,33 +65,32 @@ public function test_exif_d70_iptc() { // Exif from a Nikon D70 with IPTC data added later. $out = wp_read_image_metadata( DIR_TESTDATA . '/images/2004-07-22-DSC_0007.jpg' ); - $this->assertEquals( 6.3, $out['aperture'] ); - $this->assertSame( 'IPTC Creator', $out['credit'] ); - $this->assertSame( 'NIKON D70', $out['camera'] ); - $this->assertSame( 'IPTC Caption', $out['caption'] ); - $this->assertEquals( strtotime( '2004-07-22 17:14:35' ), $out['created_timestamp'] ); - $this->assertSame( 'IPTC Copyright', $out['copyright'] ); - $this->assertEquals( 18, $out['focal_length'] ); - $this->assertEquals( 200, $out['iso'] ); - $this->assertEquals( 1 / 25, $out['shutter_speed'] ); - $this->assertSame( 'IPTC Headline', $out['title'] ); + $this->assertEquals( 6.3, $out['aperture'], 'Aperture value not equivalent' ); + $this->assertSame( 'IPTC Creator', $out['credit'], 'Credit value not the same' ); + $this->assertSame( 'NIKON D70', $out['camera'], 'Camera value not the same' ); + $this->assertSame( 'IPTC Caption', $out['caption'], 'Caption value not the same' ); + $this->assertEquals( strtotime( '2004-07-22 17:14:35' ), $out['created_timestamp'], 'Timestamp value not equivalent' ); + $this->assertSame( 'IPTC Copyright', $out['copyright'], 'Copyright value not the same' ); + $this->assertEquals( 18, $out['focal_length'], 'Focal length value not equivalent' ); + $this->assertEquals( 200, $out['iso'], 'Iso value not equivalent' ); + $this->assertEquals( 1 / 25, $out['shutter_speed'], 'Shutter speed value not equivalent' ); + $this->assertSame( 'IPTC Headline', $out['title'], 'Title value not the same' ); } public function test_exif_fuji() { // Exif from a Fuji FinePix S5600 (thanks Mark). $out = wp_read_image_metadata( DIR_TESTDATA . '/images/a2-small.jpg' ); - $this->assertEquals( 4.5, $out['aperture'] ); - $this->assertSame( '', $out['credit'] ); - $this->assertSame( 'FinePix S5600', $out['camera'] ); - $this->assertSame( '', $out['caption'] ); - $this->assertEquals( strtotime( '2007-09-03 10:17:03' ), $out['created_timestamp'] ); - $this->assertSame( '', $out['copyright'] ); - $this->assertEquals( 6.3, $out['focal_length'] ); - $this->assertEquals( 64, $out['iso'] ); - $this->assertEquals( 1 / 320, $out['shutter_speed'] ); - $this->assertSame( '', $out['title'] ); - + $this->assertEquals( 4.5, $out['aperture'], 'Aperture value not equivalent' ); + $this->assertSame( '', $out['credit'], 'Credit value not the same' ); + $this->assertSame( 'FinePix S5600', $out['camera'], 'Camera value not the same' ); + $this->assertSame( '', $out['caption'], 'Caption value not the same' ); + $this->assertEquals( strtotime( '2007-09-03 10:17:03' ), $out['created_timestamp'], 'Timestamp value not equivalent' ); + $this->assertSame( '', $out['copyright'], 'Copyright value not the same' ); + $this->assertEquals( 6.3, $out['focal_length'], 'Focal length value not equivalent' ); + $this->assertEquals( 64, $out['iso'], 'Iso value not equivalent' ); + $this->assertEquals( 1 / 320, $out['shutter_speed'], 'Shutter speed value not equivalent' ); + $this->assertSame( '', $out['title'], 'Title value not the same' ); } /** @@ -102,32 +101,32 @@ public function test_exif_error() { // This triggers a warning mesage when reading the Exif block. $out = wp_read_image_metadata( DIR_TESTDATA . '/images/waffles.jpg' ); - $this->assertEquals( 0, $out['aperture'] ); - $this->assertSame( '', $out['credit'] ); - $this->assertSame( '', $out['camera'] ); - $this->assertSame( '', $out['caption'] ); - $this->assertEquals( 0, $out['created_timestamp'] ); - $this->assertSame( '', $out['copyright'] ); - $this->assertEquals( 0, $out['focal_length'] ); - $this->assertEquals( 0, $out['iso'] ); - $this->assertEquals( 0, $out['shutter_speed'] ); - $this->assertSame( '', $out['title'] ); + $this->assertEquals( 0, $out['aperture'], 'Aperture value not equivalent' ); + $this->assertSame( '', $out['credit'], 'Credit value not the same' ); + $this->assertSame( '', $out['camera'], 'Camera value not the same' ); + $this->assertSame( '', $out['caption'], 'Caption value not the same' ); + $this->assertEquals( 0, $out['created_timestamp'], 'Timestamp value not equivalent' ); + $this->assertSame( '', $out['copyright'], 'Copyright value not the same' ); + $this->assertEquals( 0, $out['focal_length'], 'Focal length value not equivalent' ); + $this->assertEquals( 0, $out['iso'], 'Iso value not equivalent' ); + $this->assertEquals( 0, $out['shutter_speed'], 'Shutter speed value not equivalent' ); + $this->assertSame( '', $out['title'], 'Title value not the same' ); } public function test_exif_no_data() { // No Exif data in this image (from burningwell.org). $out = wp_read_image_metadata( DIR_TESTDATA . '/images/canola.jpg' ); - $this->assertEquals( 0, $out['aperture'] ); - $this->assertSame( '', $out['credit'] ); - $this->assertSame( '', $out['camera'] ); - $this->assertSame( '', $out['caption'] ); - $this->assertEquals( 0, $out['created_timestamp'] ); - $this->assertSame( '', $out['copyright'] ); - $this->assertEquals( 0, $out['focal_length'] ); - $this->assertEquals( 0, $out['iso'] ); - $this->assertEquals( 0, $out['shutter_speed'] ); - $this->assertSame( '', $out['title'] ); + $this->assertEquals( 0, $out['aperture'], 'Aperture value not equivalent' ); + $this->assertSame( '', $out['credit'], 'Credit value not the same' ); + $this->assertSame( '', $out['camera'], 'Camera value not the same' ); + $this->assertSame( '', $out['caption'], 'Caption value not the same' ); + $this->assertEquals( 0, $out['created_timestamp'], 'Timestamp value not equivalent' ); + $this->assertSame( '', $out['copyright'], 'Copyright value not the same' ); + $this->assertEquals( 0, $out['focal_length'], 'Focal length value not equivalent' ); + $this->assertEquals( 0, $out['iso'], 'Iso value not equivalent' ); + $this->assertEquals( 0, $out['shutter_speed'], 'Shutter speed value not equivalent' ); + $this->assertSame( '', $out['title'], 'Title value not the same' ); } /** @@ -155,18 +154,18 @@ public function test_missing_image_file() { public function test_exif_keywords() { $out = wp_read_image_metadata( DIR_TESTDATA . '/images/33772.jpg' ); - $this->assertSame( '8', $out['aperture'] ); - $this->assertSame( 'Photoshop Author', $out['credit'] ); - $this->assertSame( 'DMC-LX2', $out['camera'] ); - $this->assertSame( 'Photoshop Description', $out['caption'] ); - $this->assertEquals( 1306315327, $out['created_timestamp'] ); - $this->assertSame( 'Photoshop Copyrright Notice', $out['copyright'] ); - $this->assertSame( '6.3', $out['focal_length'] ); - $this->assertSame( '100', $out['iso'] ); - $this->assertSame( '0.0025', $out['shutter_speed'] ); - $this->assertSame( 'Photoshop Document Ttitle', $out['title'] ); - $this->assertEquals( 1, $out['orientation'] ); - $this->assertSame( array( 'beach', 'baywatch', 'LA', 'sunset' ), $out['keywords'] ); + $this->assertSame( '8', $out['aperture'], 'Aperture value not the same' ); + $this->assertSame( 'Photoshop Author', $out['credit'], 'Credit value not the same' ); + $this->assertSame( 'DMC-LX2', $out['camera'], 'Camera value not the same' ); + $this->assertSame( 'Photoshop Description', $out['caption'], 'Caption value not the same' ); + $this->assertEquals( 1306315327, $out['created_timestamp'], 'Timestamp value not equivalent' ); + $this->assertSame( 'Photoshop Copyrright Notice', $out['copyright'], 'Copyright value not the same' ); + $this->assertSame( '6.3', $out['focal_length'], 'Focal length value not the same' ); + $this->assertSame( '100', $out['iso'], 'Iso value not the same' ); + $this->assertSame( '0.0025', $out['shutter_speed'], 'Shutter speed value not the same' ); + $this->assertSame( 'Photoshop Document Ttitle', $out['title'], 'Title value not the same' ); + $this->assertEquals( 1, $out['orientation'], 'Orientation value not equivalent' ); + $this->assertSame( array( 'beach', 'baywatch', 'LA', 'sunset' ), $out['keywords'], 'Keywords not the same' ); } /** @@ -239,4 +238,23 @@ public function data_stream() { ), ); } + + /** + * @ticket 54385 + */ + public function test_exif_unexpected_data() { + // Unexpected Exif data: FNumber is "0/0", aperture should be 0. + $out = wp_read_image_metadata( DIR_TESTDATA . '/images/sugarloaf-mountain.jpg' ); + + $this->assertEquals( 0, $out['aperture'], 'Aperture value not equivalent' ); + $this->assertSame( '', $out['credit'], 'Credit value not the same' ); + $this->assertSame( 'X-T1', $out['camera'], 'Camera value not the same' ); + $this->assertSame( '', $out['caption'], 'Caption value not the same' ); + $this->assertEquals( 0, $out['created_timestamp'], 'Timestamp value not equivalent' ); + $this->assertSame( '', $out['copyright'], 'Copyright value not the same' ); + $this->assertEquals( 50, $out['focal_length'], 'Focal length value not equivalent' ); + $this->assertEquals( 200, $out['iso'], 'Iso value not equivalent' ); + $this->assertEquals( 2, $out['shutter_speed'], 'Shutter speed value not equivalent' ); + $this->assertSame( 'Sugarloaf Panorama', $out['title'], 'Title value not the same' ); + } }