Prevent ExifInterface incorrectly copying a WebP padding byte

When skipping over the EXIF chunk in the input file (and writing a new
EXIF chunk to the output based on the internal Exif data of the class),
it's important to skip both the chunk and any trailing padding byte (if
the chunk size is odd).

Without this fix the output of saveAttributes may be invalid (not
decodable with BitmapFactory.decodeFile).

Test: Added tests to ExifInterfaceTest to check the output of
saveAttributes is always parsable with BitmapFactory.

Bug: 253622642

Change-Id: I6b7c4612b830dbf95a12c42eac5d6301dfb15104

2 files changed, 65 insertions, 2 deletions

diff --git a/media/java/android/media/ExifInterface.java b/media/java/android/media/ExifInterface.java
index e18642ce9856..74e0fc1f9677 100644
--- a/media/java/android/media/ExifInterface.java
+++ b/media/java/android/media/ExifInterface.java
@@ -3750,6 +3750,11 @@ public class ExifInterface {
                 // Skip input stream to the end of the EXIF chunk
                 totalInputStream.skipBytes(WEBP_CHUNK_TYPE_BYTE_LENGTH);
                 int exifChunkLength = totalInputStream.readInt();
+                // RIFF chunks have a single padding byte at the end if the declared chunk size is
+                // odd.
+                if (exifChunkLength % 2 != 0) {
+                    exifChunkLength++;
+                }
                 totalInputStream.skipBytes(exifChunkLength);
 
                 // Write new EXIF chunk to output stream
diff --git a/media/tests/MediaFrameworkTest/src/com/android/mediaframeworktest/unit/ExifInterfaceTest.java b/media/tests/MediaFrameworkTest/src/com/android/mediaframeworktest/unit/ExifInterfaceTest.java
index 195df78290f4..006f4e9bc0d5 100644
--- a/media/tests/MediaFrameworkTest/src/com/android/mediaframeworktest/unit/ExifInterfaceTest.java
+++ b/media/tests/MediaFrameworkTest/src/com/android/mediaframeworktest/unit/ExifInterfaceTest.java
@@ -23,6 +23,7 @@ import android.graphics.Bitmap;
 import android.graphics.BitmapFactory;
 import android.media.ExifInterface;
 import android.os.Environment;
+import android.os.FileUtils;
 import android.test.AndroidTestCase;
 import android.util.Log;
 import android.system.ErrnoException;
@@ -37,6 +38,8 @@ import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.InputStream;
 import java.io.IOException;
+import java.io.OutputStream;
+import java.util.Objects;
 
 import libcore.io.IoUtils;
 import libcore.io.Streams;
@@ -353,20 +356,23 @@ public class ExifInterfaceTest extends AndroidTestCase {
         }
     }
 
-    private void testSaveAttributes_withFileName(File imageFile, ExpectedValue expectedValue)
+    private void testSaveAttributes_withFileName(File srcFile, ExpectedValue expectedValue)
             throws IOException {
+        File imageFile = clone(srcFile);
         String verboseTag = imageFile.getName();
 
         ExifInterface exifInterface = new ExifInterface(imageFile.getAbsolutePath());
         exifInterface.saveAttributes();
         exifInterface = new ExifInterface(imageFile.getAbsolutePath());
         compareWithExpectedValue(exifInterface, expectedValue, verboseTag);
+        assertBitmapsEquivalent(srcFile, imageFile);
+        assertSecondSaveProducesSameSizeFile(imageFile);
 
         // Test for modifying one attribute.
+        exifInterface = new ExifInterface(imageFile.getAbsolutePath());
         String backupValue = exifInterface.getAttribute(ExifInterface.TAG_MAKE);
         exifInterface.setAttribute(ExifInterface.TAG_MAKE, "abc");
         exifInterface.saveAttributes();
-        exifInterface = new ExifInterface(imageFile.getAbsolutePath());
         assertEquals("abc", exifInterface.getAttribute(ExifInterface.TAG_MAKE));
         // Restore the backup value.
         exifInterface.setAttribute(ExifInterface.TAG_MAKE, backupValue);
@@ -481,4 +487,56 @@ public class ExifInterfaceTest extends AndroidTestCase {
         // Test if it is possible to parse the volantis generated JPEG smoothly.
         testExifInterfaceForJpeg(VOLANTIS_JPEG, R.array.volantis_jpg);
     }
+
+    /**
+     * Asserts that {@code expectedImageFile} and {@code actualImageFile} can be decoded by
+     * {@link BitmapFactory} and the results have the same width, height and MIME type.
+     *
+     * <p>This does not check the image itself for similarity/equality.
+     */
+    private void assertBitmapsEquivalent(File expectedImageFile, File actualImageFile) {
+        BitmapFactory.Options expectedOptions = new BitmapFactory.Options();
+        Bitmap expectedBitmap = Objects.requireNonNull(
+                BitmapFactory.decodeFile(expectedImageFile.getAbsolutePath(), expectedOptions));
+        BitmapFactory.Options actualOptions = new BitmapFactory.Options();
+        Bitmap actualBitmap = Objects.requireNonNull(
+                BitmapFactory.decodeFile(actualImageFile.getAbsolutePath(), actualOptions));
+
+        assertEquals(expectedOptions.outWidth, actualOptions.outWidth);
+        assertEquals(expectedOptions.outHeight, actualOptions.outHeight);
+        assertEquals(expectedOptions.outMimeType, actualOptions.outMimeType);
+        assertEquals(expectedBitmap.getWidth(), actualBitmap.getWidth());
+        assertEquals(expectedBitmap.getHeight(), actualBitmap.getHeight());
+    }
+
+    /**
+     * Asserts that saving the file the second time (without modifying any attributes) produces
+     * exactly the same length file as the first save. The first save (with no modifications) is
+     * expected to (possibly) change the file length because {@link ExifInterface} may move/reformat
+     * the Exif block within the file, but the second save should not make further modifications.
+     */
+    private void assertSecondSaveProducesSameSizeFile(File imageFileAfterOneSave)
+            throws IOException {
+        File imageFileAfterTwoSaves = clone(imageFileAfterOneSave);
+        ExifInterface exifInterface = new ExifInterface(imageFileAfterTwoSaves.getAbsolutePath());
+        exifInterface.saveAttributes();
+        if (imageFileAfterOneSave.getAbsolutePath().endsWith(".png")
+                || imageFileAfterOneSave.getAbsolutePath().endsWith(".webp")) {
+            // PNG and (some) WebP files are (surprisingly) modified between the first and second
+            // save (b/249097443), so we check the difference between second and third save instead.
+            File imageFileAfterThreeSaves = clone(imageFileAfterTwoSaves);
+            exifInterface = new ExifInterface(imageFileAfterThreeSaves.getAbsolutePath());
+            exifInterface.saveAttributes();
+            assertEquals(imageFileAfterTwoSaves.length(), imageFileAfterThreeSaves.length());
+        } else {
+            assertEquals(imageFileAfterOneSave.length(), imageFileAfterTwoSaves.length());
+        }
+    }
+
+    private static File clone(File original) throws IOException {
+        final File cloned =
+                File.createTempFile("tmp_", +System.nanoTime() + "_" + original.getName());
+        FileUtils.copyFileOrThrow(original, cloned);
+        return cloned;
+    }
 }