Call avifFixImageXMP() during jpg,png reading

CHANGELOG.md

@@ -65,6 +65,8 @@ List of incompatible ABI changes in this release:
 * At decoding, avifIOStats now returns the same values as at encoding.
 * avifRGBImageAllocatePixels() now returns avifResult instead of void to report
   memory allocation failures.
+* avifReadImage(), avifJPEGRead() and avifPNGRead() now remove the trailing zero
+  byte from read XMP chunks, if any. See avifImageFixXMP().
 
 ## [0.11.1] - 2022-10-19
 

apps/shared/avifjpeg.c

@@ -551,6 +551,7 @@ avifBool avifJPEGRead(const char * inputFilename,
         } else if (standardXMPData) {
             avifImageSetMetadataXMP(avif, standardXMPData, standardXMPSize);
         }
+        avifImageFixXMP(avif); // Remove one trailing null character if any.
     }
     jpeg_finish_decompress(&cinfo);
     ret = AVIF_TRUE;

apps/shared/avifpng.c

@@ -193,6 +193,10 @@ static avifBool avifExtractExifAndXMP(png_structp png, png_infop info, avifBool
             *ignoreXMP = AVIF_TRUE; // Ignore any other XMP chunk.
         }
     }
+    // The iTXt XMP payload may not contain a zero byte according to section 4.2.3.3 of
+    // the PNG specification, version 1.2. Still remove one trailing null character if any,
+    // in case libpng does not strictly enforce that at decoding.
+    avifImageFixXMP(avif);
     return AVIF_TRUE;
 }
 
@@ -511,26 +515,23 @@ avifBool avifPNGWrite(const char * outputFilename, const avifImage * avif, uint3
     if (avif->xmp.data && (avif->xmp.size > 0)) {
         // The iTXt XMP payload may not contain a zero byte according to section 4.2.3.3 of
         // the PNG specification, version 1.2.
-        if (memchr(avif->xmp.data, '\0', avif->xmp.size)) {
-            fprintf(stderr, "Error writing PNG: XMP metadata contains an invalid null character\n");
+        // The chunk is given to libpng as is. Bytes after a zero byte may be stripped.
+
+        // Providing the length through png_text.itxt_length does not work.
+        // The given png_text.text string must end with a zero byte.
+        if (avif->xmp.size >= SIZE_MAX) {
+            fprintf(stderr, "Error writing PNG: XMP metadata is too big\n");
             goto cleanup;
-        } else {
-            // Providing the length through png_text.itxt_length does not work.
-            // The given png_text.text string must end with a zero byte.
-            if (avif->xmp.size >= SIZE_MAX) {
-                fprintf(stderr, "Error writing PNG: XMP metadata is too big\n");
-                goto cleanup;
-            }
-            avifRWDataRealloc(&xmp, avif->xmp.size + 1);
-            memcpy(xmp.data, avif->xmp.data, avif->xmp.size);
-            xmp.data[avif->xmp.size] = '\0';
-            png_text * text = &texts[numTextMetadataChunks++];
-            memset(text, 0, sizeof(*text));
-            text->compression = PNG_ITXT_COMPRESSION_NONE;
-            text->key = "XML:com.adobe.xmp";
-            text->text = (char *)xmp.data;
-            text->itxt_length = xmp.size;
         }
+        avifRWDataRealloc(&xmp, avif->xmp.size + 1);
+        memcpy(xmp.data, avif->xmp.data, avif->xmp.size);
+        xmp.data[avif->xmp.size] = '\0';
+        png_text * text = &texts[numTextMetadataChunks++];
+        memset(text, 0, sizeof(*text));
+        text->compression = PNG_ITXT_COMPRESSION_NONE;
+        text->key = "XML:com.adobe.xmp";
+        text->text = (char *)xmp.data;
+        text->itxt_length = xmp.size;
     }
     if (numTextMetadataChunks != 0) {
         png_set_text(png, info, texts, numTextMetadataChunks);

apps/shared/avifutil.c

@@ -277,6 +277,22 @@ avifAppFileFormat avifReadImage(const char * filename,
     return format;
 }
 
+void avifImageFixXMP(avifImage * image)
+{
+    // Zero bytes are forbidden in UTF-8 XML: https://en.wikipedia.org/wiki/Valid_characters_in_XML
+    // Keeping zero bytes in XMP may lead to issues at encoding or decoding.
+    // For example, the PNG specification forbids null characters in XMP. See avifPNGWrite().
+    // The XMP Specification Part 3 says "When XMP is encoded as UTF-8,
+    // there are no zero bytes in the XMP packet" for GIF.
+
+    // Consider a single trailing null character following a non-null character
+    // as a programming error. Leave other null characters as is.
+    // See the discussion at https://github.com/AOMediaCodec/libavif/issues/1333.
+    if (image->xmp.size >= 2 && image->xmp.data[image->xmp.size - 1] == '\0' && image->xmp.data[image->xmp.size - 2] != '\0') {
+        --image->xmp.size;
+    }
+}
+
 void avifDumpDiagnostics(const avifDiagnostics * diag)
 {
     if (!*diag->error) {

apps/shared/avifutil.h

@@ -72,6 +72,9 @@ avifAppFileFormat avifReadImage(const char * filename,
                                 avifAppSourceTiming * sourceTiming,
                                 struct y4mFrameIterator ** frameIter);
 
+// Removes a single trailing null character from the image->xmp, if there is exactly one.
+void avifImageFixXMP(avifImage * image);
+
 // Used by image decoders when the user doesn't explicitly choose a format with --yuv
 // This must match the cited fallback for "--yuv auto" in avifenc.c's syntax() function.
 #define AVIF_APP_DEFAULT_PIXEL_FORMAT AVIF_PIXEL_FORMAT_YUV444

tests/data/README.md

@@ -106,6 +106,22 @@ The goal is to have a large XMP blob so that it can only be stored as multiple e
 |   136627 | 0xffe1 APP1 |   4791 | http://ns.adobe.com/xmp/extensio |
 |          |             |        | ...                              |
 
+### File [paris_xmp_trailing_null.jpg](paris_xmp_trailing_null.jpg)
+
+![](paris_xmp_trailing_null.jpg)
+
+License: [same as libavif](https://github.com/AOMediaCodec/libavif/blob/main/LICENSE)
+
+Source: `paris_exif_xmp_icc.jpg` loaded with `avifReadImage()`, stripped of ICC and Exif, a zero byte appended to XMP,
+then written with `avifJPEGWrite()` with quality 0 (without calling `avifImageFixXMP()`).
+
+| address | marker      | length | data                                         |
+|--------:|-------------|-------:|----------------------------------------------|
+|       0 | 0xffd8 SOI  |        |                                              |
+|       2 | 0xffe0 APP0 |     16 | `JFIF.........`                              |
+|      20 | 0xffe1 APP1 |   3930 | `http://ns.adobe.com/xap/1.0/.<?x`           |
+|         |             |        | ...                                          |
+
 ### File [paris_icc_exif_xmp.png](paris_icc_exif_xmp.png)
 
 ![](paris_icc_exif_xmp.png)
@@ -148,7 +164,7 @@ metadata this way).
 
 License: [same as libavif](https://github.com/AOMediaCodec/libavif/blob/main/LICENSE)
 
-Source: `paris_exif_xmp_icc.jpg` stripped from all metadata with `exiftool -all=` and Exif orientation added
+Source: `paris_exif_xmp_icc.jpg` stripped of all metadata with `exiftool -all=` and Exif orientation added
 with `exiv2 -k -M "set Exif.Image.Orientation 5"`
 
 | address | marker      |  length | data                                 |

tests/gtest/avifmetadatatest.cc

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: BSD-2-Clause
 
 #include <array>
+#include <cstring>
 #include <tuple>
 
 #include "avif/avif.h"
@@ -413,6 +414,32 @@ TEST(MetadataTest, MultipleExtendedXMPAndAlternativeGUIDTag) {
 
 //------------------------------------------------------------------------------
 
+// Regression test for https://github.com/AOMediaCodec/libavif/issues/1333.
+// Coverage for avifImageFixXMP().
+TEST(MetadataTest, XMPWithTrailingNullCharacter) {
+  testutil::AvifImagePtr jpg =
+      testutil::ReadImage(data_path, "paris_xmp_trailing_null.jpg");
+  ASSERT_NE(jpg, nullptr);
+  ASSERT_NE(jpg->xmp.size, 0u);
+  // avifJPEGRead() should strip the trailing null character.
+  ASSERT_EQ(std::memchr(jpg->xmp.data, '\0', jpg->xmp.size), nullptr);
+
+  // Append a zero byte to see what happens when encoded with libpng.
+  avifRWDataRealloc(&jpg->xmp, jpg->xmp.size + 1);
+  jpg->xmp.data[jpg->xmp.size - 1] = '\0';
+  testutil::WriteImage(jpg.get(),
+                       (testing::TempDir() + "xmp_trailing_null.png").c_str());
+  const testutil::AvifImagePtr png =
+      testutil::ReadImage(testing::TempDir().c_str(), "xmp_trailing_null.png");
+  ASSERT_NE(png, nullptr);
+  ASSERT_NE(png->xmp.size, 0u);
+  // avifPNGRead() should strip the trailing null character, but the libpng
+  // export during testutil::WriteImage() probably took care of that anyway.
+  ASSERT_EQ(std::memchr(png->xmp.data, '\0', png->xmp.size), nullptr);
+}
+
+//------------------------------------------------------------------------------
+
 }  // namespace
 }  // namespace libavif