From a6cb9797a695b0b57888edf7f9a5677aa57727f6 Mon Sep 17 00:00:00 2001 From: Yannis Guyon Date: Fri, 12 May 2023 10:39:41 +0200 Subject: [PATCH 1/3] Call avifFixImageXMP() during jpg,png reading Strip one null character from XMP chunks extracted from JPG and PNG files. Add regression test in avifmetadatatest. --- CHANGELOG.md | 2 ++ apps/shared/avifjpeg.c | 1 + apps/shared/avifpng.c | 37 +++++++++++++------------ apps/shared/avifutil.c | 20 +++++++++++++ apps/shared/avifutil.h | 3 ++ tests/data/README.md | 16 +++++++++++ tests/data/paris_xmp_trailing_null.jpg | Bin 0 -> 6862 bytes tests/gtest/avifmetadatatest.cc | 27 ++++++++++++++++++ 8 files changed, 88 insertions(+), 18 deletions(-) create mode 100644 tests/data/paris_xmp_trailing_null.jpg diff --git a/CHANGELOG.md b/CHANGELOG.md index 3688d14941..0bd3ba2cc6 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 avifFixImageXMP(). ## [0.11.1] - 2022-10-19 diff --git a/apps/shared/avifjpeg.c b/apps/shared/avifjpeg.c index b599b3e2b7..468c0f24ad 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); } + avifFixImageXMP(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..ec4bf82038 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. + avifFixImageXMP(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..fa1852c31a 100644 --- a/apps/shared/avifutil.c +++ b/apps/shared/avifutil.c @@ -277,6 +277,26 @@ avifAppFileFormat avifReadImage(const char * filename, return format; } +void avifFixImageXMP(avifImage * image) +{ + if (!image->xmp.data) { + return; + } + + // 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..3f3a6ff089 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 avifFixImageXMP(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..e7b7325255 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 from ICC and Exif, a zero byte appended to XMP, +then written with `avifJPEGWrite()` with quality 0 (without calling `avifFixImageXMP()`). + +| address | marker | length | data | +|--------:|-------------|-------:|----------------------------------------------| +| 0 | 0xffd8 SOI | | | +| 2 | 0xffe0 APP0 | 16 | `JFIF.........` | +| 20 | 0xffe1 APP1 | 3930 | `http://ns.adobe.com/xap/1.0/.atxB}0@dV!y~$dzBuA1j z$qdEel7c1=#ZjFQ9x$>TXlHn!6)kBW3MpwRGoFb9r9fYbTf${JQ)o(CdcpnA*_Gmk z^g`0NTK2Aw&i>~=|Ns5p_aDhiZ!G-^t&8?XdJ#n-M8OYQdix8>bg?)R@_4d^4oS_) zT8EO$cqXI~57)tZ&`sMWMkM7Pt!S6EAwApW_}veGT^Z@U=9A>EF^)CPSMqnG30`3fcJGW zK3VcI0vFJjU@8z~6fFf4TB=j#`MRG-wosI^iiZ7w=MEj5NIWO_0usykc}-)4Ku}`> zLEg{sDQ|!cbovxIpw!pjme+xA((v+lupbrt1i4OC3VIbr@CH(hz<0`w>{nPuRaKu? zmQ=6clO1+d3H9dk83_ueXQUy`bN7fg?6L_D%dij89al4i;0OPPLj_vRo zsD!%nnpDi?2XndME(hMujvteZL;au%RVqq7Qc;6tEYFA7K!_Ix@xTwiUt(cUH_oYg zYVUtC#X2N4-KiH~+r6fy2DClqXjr!sMu#K?>!Zt2kj6CCVNNP(DLbTr(t$*VnSOzq zrZ}(a3#b7`P&AGafd0`!%j-w$d1#VB4m$g;9uN(rE8gNK(CzdywD%i{Oo zL&1=Q=X|x0XUh%{pdFclQwa|nhuqEfmDdUaD+sJ0u!6t}0xJmo{~>VhYqFMwPt4=+ zB^WLJ5_O}cOMQV8>Qa3yy@1-*P#x4EE9F3zHp<#YExm-Uh5rHADDpz)i?UdoY^zqU zX>Mr+gXh*E3uU!hnyfZkQxjO<3(rwgn{EAOZu_b$;?ioz9-80(=#y)eCzGSU;pNvGw;6l{s$lZ{_Gz<{^Oti{K;Q%UKFw#X~Eq{ z?7Y0%ATLW(leK9z&Wo~)!)0x2vTf#8t=}GBE$z9&!S7!~?|AgdXJ2gI;!S*brJP@A z*>Iikw(}#L8p-UxOYHuO%B+#txxC&&msu$&p0y2aLrGWV$o(bPjOCv$w~Imf#|FVeSjS+K__k*=+Ye%>uz)fro5s)XD+PTie0 z_J=`xIk6C~10soZ%w8!ayVp;HZPM*B7A2zuuVe5CgR$(IIxHZZ0J$O}V(4-V0<2#( zREEr`-n0P_&JKfl83xjH3Ls2PPqqLg_{qZt#2Dt!UQu^?rfH5;$2HSbnVy0|?2d;| zfY)`kTDb;MI1v;Cu1L65bke9+PMm^pNieS=jk#!4kWz`GR8Dq z#xoetnfhcg16qJRsERohK0}Icw4n#g(iemey+~VGYIn@_#?i1+)nBt&S%rK}$ACR6vk|1^8wY@&!G|WU4 zDrM92aNo6X6kzc7rlHf!Zv+v-5Rz*2nppwW5-C%4BsDCObGYG^x_+FP#lx+A{qyvJ ztwOmxu&$QEVl@=hz>Io+maf&%k?P5#%b=^7(}|e%y?Gyy!5|#u3+c2T-Tl{B^+LGy^~Z^L;^9^4c{*k}GgWd! z_6&RbfK0&5PT?Arr<0h-4`a#`%8Ls)K!JR2TZjkN!hLueFslj77xY` z_4ebIq(T_G6UDjeJfiP=;+;>)glkqfK4U$4tDbkgJdbV=5@&!L7zV=N8m=DhB9Xo@ zNZkpY{j-g&*-!vj(21DkG;~4S6;Lf6*=fVj0)>vW_8rA3>xED6yAjUR*A75gtMkka zG-^q@*FvRmV*yBgB{&pGhX%<35#hY5!VyXQ;G@E3=rjr^80jrhB38U$-2xnA2pN$1t4tWQ^GP8k(0$HO>qeBMGhb{ndi`Tvj^h-i0usK9Y=^z@yT!nB^fMBm4S2bPT9BLQ6XaM220qyU0{EQAdqegv*#9b8 UJ}ktj=(cB^PG~|Inp%4E9~PivBme*a literal 0 HcmV?d00001 diff --git a/tests/gtest/avifmetadatatest.cc b/tests/gtest/avifmetadatatest.cc index 507b573578..7740e87965 100644 --- a/tests/gtest/avifmetadatatest.cc +++ b/tests/gtest/avifmetadatatest.cc @@ -2,6 +2,7 @@ // SPDX-License-Identifier: BSD-2-Clause #include +#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 avifFixImageXMP(). +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] = '\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 From d78b19125d7275f6e6eb189f35e0af942d868283 Mon Sep 17 00:00:00 2001 From: Yannis Guyon Date: Mon, 22 May 2023 13:54:34 +0200 Subject: [PATCH 2/3] Rename avifFixImageXMP() to avifImageFixXMP() --- CHANGELOG.md | 2 +- apps/shared/avifjpeg.c | 2 +- apps/shared/avifpng.c | 2 +- apps/shared/avifutil.c | 2 +- apps/shared/avifutil.h | 2 +- tests/data/README.md | 2 +- tests/gtest/avifmetadatatest.cc | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bd3ba2cc6..8ece0ee1b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,7 +66,7 @@ List of incompatible ABI changes in this release: * 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 avifFixImageXMP(). + 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 468c0f24ad..1e4d668ddb 100644 --- a/apps/shared/avifjpeg.c +++ b/apps/shared/avifjpeg.c @@ -551,7 +551,7 @@ avifBool avifJPEGRead(const char * inputFilename, } else if (standardXMPData) { avifImageSetMetadataXMP(avif, standardXMPData, standardXMPSize); } - avifFixImageXMP(avif); // Remove one trailing null character if any. + 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 ec4bf82038..e1226be471 100644 --- a/apps/shared/avifpng.c +++ b/apps/shared/avifpng.c @@ -196,7 +196,7 @@ static avifBool avifExtractExifAndXMP(png_structp png, png_infop info, avifBool // 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. - avifFixImageXMP(avif); + avifImageFixXMP(avif); return AVIF_TRUE; } diff --git a/apps/shared/avifutil.c b/apps/shared/avifutil.c index fa1852c31a..a570c47d93 100644 --- a/apps/shared/avifutil.c +++ b/apps/shared/avifutil.c @@ -277,7 +277,7 @@ avifAppFileFormat avifReadImage(const char * filename, return format; } -void avifFixImageXMP(avifImage * image) +void avifImageFixXMP(avifImage * image) { if (!image->xmp.data) { return; diff --git a/apps/shared/avifutil.h b/apps/shared/avifutil.h index 3f3a6ff089..16d7a53d7c 100644 --- a/apps/shared/avifutil.h +++ b/apps/shared/avifutil.h @@ -73,7 +73,7 @@ avifAppFileFormat avifReadImage(const char * filename, struct y4mFrameIterator ** frameIter); // Removes a single trailing null character from the image->xmp, if there is exactly one. -void avifFixImageXMP(avifImage * image); +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. diff --git a/tests/data/README.md b/tests/data/README.md index e7b7325255..4af9e0a071 100644 --- a/tests/data/README.md +++ b/tests/data/README.md @@ -113,7 +113,7 @@ The goal is to have a large XMP blob so that it can only be stored as multiple e License: [same as libavif](https://github.com/AOMediaCodec/libavif/blob/main/LICENSE) Source: `paris_exif_xmp_icc.jpg` loaded with `avifReadImage()`, stripped from ICC and Exif, a zero byte appended to XMP, -then written with `avifJPEGWrite()` with quality 0 (without calling `avifFixImageXMP()`). +then written with `avifJPEGWrite()` with quality 0 (without calling `avifImageFixXMP()`). | address | marker | length | data | |--------:|-------------|-------:|----------------------------------------------| diff --git a/tests/gtest/avifmetadatatest.cc b/tests/gtest/avifmetadatatest.cc index 7740e87965..4f7554f8b9 100644 --- a/tests/gtest/avifmetadatatest.cc +++ b/tests/gtest/avifmetadatatest.cc @@ -415,7 +415,7 @@ TEST(MetadataTest, MultipleExtendedXMPAndAlternativeGUIDTag) { //------------------------------------------------------------------------------ // Regression test for https://github.com/AOMediaCodec/libavif/issues/1333. -// Coverage for avifFixImageXMP(). +// Coverage for avifImageFixXMP(). TEST(MetadataTest, XMPWithTrailingNullCharacter) { testutil::AvifImagePtr jpg = testutil::ReadImage(data_path, "paris_xmp_trailing_null.jpg"); From 9de4a3e3d825252f9c9f3922ea7c895f900ad876 Mon Sep 17 00:00:00 2001 From: Yannis Guyon Date: Mon, 22 May 2023 13:57:55 +0200 Subject: [PATCH 3/3] Fix avifmetadatatest --- apps/shared/avifutil.c | 4 ---- tests/data/README.md | 4 ++-- tests/gtest/avifmetadatatest.cc | 2 +- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/apps/shared/avifutil.c b/apps/shared/avifutil.c index a570c47d93..0f8381fa22 100644 --- a/apps/shared/avifutil.c +++ b/apps/shared/avifutil.c @@ -279,10 +279,6 @@ avifAppFileFormat avifReadImage(const char * filename, void avifImageFixXMP(avifImage * image) { - if (!image->xmp.data) { - return; - } - // 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(). diff --git a/tests/data/README.md b/tests/data/README.md index 4af9e0a071..a093986a6e 100644 --- a/tests/data/README.md +++ b/tests/data/README.md @@ -112,7 +112,7 @@ The goal is to have a large XMP blob so that it can only be stored as multiple e License: [same as libavif](https://github.com/AOMediaCodec/libavif/blob/main/LICENSE) -Source: `paris_exif_xmp_icc.jpg` loaded with `avifReadImage()`, stripped from ICC and Exif, a zero byte appended to XMP, +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 | @@ -164,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 | diff --git a/tests/gtest/avifmetadatatest.cc b/tests/gtest/avifmetadatatest.cc index 4f7554f8b9..bb6c7b729d 100644 --- a/tests/gtest/avifmetadatatest.cc +++ b/tests/gtest/avifmetadatatest.cc @@ -426,7 +426,7 @@ TEST(MetadataTest, XMPWithTrailingNullCharacter) { // 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] = '\0'; + jpg->xmp.data[jpg->xmp.size - 1] = '\0'; testutil::WriteImage(jpg.get(), (testing::TempDir() + "xmp_trailing_null.png").c_str()); const testutil::AvifImagePtr png =