diff --git a/CHANGELOG.md b/CHANGELOG.md index 3688d14941..8ece0ee1b1 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/apps/shared/avifjpeg.c b/apps/shared/avifjpeg.c index b599b3e2b7..1e4d668ddb 100644 --- a/apps/shared/avifjpeg.c +++ b/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; diff --git a/apps/shared/avifpng.c b/apps/shared/avifpng.c index bb18ebca8a..e1226be471 100644 --- a/apps/shared/avifpng.c +++ b/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); diff --git a/apps/shared/avifutil.c b/apps/shared/avifutil.c index 8e200902ba..0f8381fa22 100644 --- a/apps/shared/avifutil.c +++ b/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) { diff --git a/apps/shared/avifutil.h b/apps/shared/avifutil.h index be0b2b8f8e..16d7a53d7c 100644 --- a/apps/shared/avifutil.h +++ b/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 diff --git a/tests/data/README.md b/tests/data/README.md index 933f2e375c..a093986a6e 100644 --- a/tests/data/README.md +++ b/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/. +#include #include #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