From b00e460d765b75934d87ebfb142944358529687f Mon Sep 17 00:00:00 2001 From: Luis Diaz Date: Mon, 4 Apr 2022 17:10:26 +0200 Subject: [PATCH 1/9] Move static functions to anonymous namespace --- src/jpgimage.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index 2f2ef859..cb018a9e 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -27,13 +27,15 @@ // class member definitions namespace Exiv2 { -static inline bool inRange(int lo, int value, int hi) { +namespace { +inline bool inRange(int lo, int value, int hi) { return lo <= value && value <= hi; } -static inline bool inRange2(int value, int lo1, int hi1, int lo2, int hi2) { +inline bool inRange2(int value, int lo1, int hi1, int lo2, int hi2) { return inRange(lo1, value, hi1) || inRange(lo2, value, hi2); } +} // namespace bool Photoshop::isIrb(const byte* pPsData) { if (pPsData == nullptr) { From 23fe743d4d43cca72ea07267ca5e28c59bb803fa Mon Sep 17 00:00:00 2001 From: Luis Diaz Date: Mon, 4 Apr 2022 17:21:53 +0200 Subject: [PATCH 2/9] Move private constants to .cpp --- include/exiv2/jpgimage.hpp | 42 ++------------------------------------ src/jpgimage.cpp | 41 ++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 41 deletions(-) diff --git a/include/exiv2/jpgimage.hpp b/include/exiv2/jpgimage.hpp index 168031cb..0b36d719 100644 --- a/include/exiv2/jpgimage.hpp +++ b/include/exiv2/jpgimage.hpp @@ -150,43 +150,6 @@ class EXIV2API JpegBase : public Image { virtual int writeHeader(BasicIo& oIo) const = 0; //@} - // Constant Data - static constexpr byte dht_ = 0xc4; //!< JPEG DHT marker - static constexpr byte dqt_ = 0xdb; //!< JPEG DQT marker - static constexpr byte dri_ = 0xdd; //!< JPEG DRI marker - static constexpr byte sos_ = 0xda; //!< JPEG SOS marker - static constexpr byte eoi_ = 0xd9; //!< JPEG EOI marker - static constexpr byte app0_ = 0xe0; //!< JPEG APP0 marker - static constexpr byte app1_ = 0xe1; //!< JPEG APP1 marker - static constexpr byte app2_ = 0xe2; //!< JPEG APP2 marker - static constexpr byte app13_ = 0xed; //!< JPEG APP13 marker - static constexpr byte com_ = 0xfe; //!< JPEG Comment marker - - // Start of Frame markers, nondifferential Huffman-coding frames - static constexpr byte sof0_ = 0xc0; //!< JPEG Start-Of-Frame marker - static constexpr byte sof1_ = 0xc1; //!< JPEG Start-Of-Frame marker - static constexpr byte sof2_ = 0xc2; //!< JPEG Start-Of-Frame marker - static constexpr byte sof3_ = 0xc3; //!< JPEG Start-Of-Frame marker - - // Start of Frame markers, differential Huffman-coding frames - static constexpr byte sof5_ = 0xc5; //!< JPEG Start-Of-Frame marker - static constexpr byte sof6_ = 0xc6; //!< JPEG Start-Of-Frame marker - static constexpr byte sof7_ = 0xc7; //!< JPEG Start-Of-Frame marker - - // Start of Frame markers, nondifferential arithmetic-coding frames - static constexpr byte sof9_ = 0xc9; //!< JPEG Start-Of-Frame marker - static constexpr byte sof10_ = 0xca; //!< JPEG Start-Of-Frame marker - static constexpr byte sof11_ = 0xcb; //!< JPEG Start-Of-Frame marker - - // Start of Frame markers, differential arithmetic-coding frames - static constexpr byte sof13_ = 0xcd; //!< JPEG Start-Of-Frame marker - static constexpr byte sof14_ = 0xce; //!< JPEG Start-Of-Frame marker - static constexpr byte sof15_ = 0xcf; //!< JPEG Start-Of-Frame marker - - static constexpr auto exifId_ = "Exif\0\0"; //!< Exif identifier - static constexpr auto jfifId_ = "JFIF\0"; //!< JFIF identifier - static constexpr auto xmpId_ = "http://ns.adobe.com/xap/1.0/\0"; //!< XMP packet identifier - static constexpr auto iccId_ = "ICC_PROFILE\0"; //!< ICC profile identifier private: //! @name Manipulators @@ -290,9 +253,8 @@ class EXIV2API JpegImage : public JpegBase { private: // Constant data - static constexpr byte soi_ = 0xd8; // SOI marker - static const byte blank_[]; // Minimal Jpeg image -}; // class JpegImage + static const byte blank_[]; ///< Minimal Jpeg image +}; //! Helper class to access %Exiv2 files class EXIV2API ExvImage : public JpegBase { diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index cb018a9e..727181cb 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -28,6 +28,45 @@ namespace Exiv2 { namespace { +// JPEG Segment markers (The first byte is always 0xFF, the value of these constants correspond to the 2nd byte) +constexpr byte dht_ = 0xc4; //!< JPEG DHT marker: Define Huffman Table(s) +constexpr byte dqt_ = 0xdb; //!< JPEG DQT marker: Define Quantization Table(s) +constexpr byte dri_ = 0xdd; //!< JPEG DRI marker +constexpr byte sos_ = 0xda; //!< JPEG SOS marker +constexpr byte eoi_ = 0xd9; //!< JPEG EOI marker +constexpr byte app0_ = 0xe0; //!< JPEG APP0 marker +constexpr byte app1_ = 0xe1; //!< JPEG APP1 marker +constexpr byte app2_ = 0xe2; //!< JPEG APP2 marker +constexpr byte app13_ = 0xed; //!< JPEG APP13 marker +constexpr byte com_ = 0xfe; //!< JPEG Comment marker +constexpr byte soi_ = 0xd8; ///!< SOI marker + +// Start of Frame markers, nondifferential Huffman-coding frames +constexpr byte sof0_ = 0xc0; //!< JPEG Start-Of-Frame marker +constexpr byte sof1_ = 0xc1; //!< JPEG Start-Of-Frame marker +constexpr byte sof2_ = 0xc2; //!< JPEG Start-Of-Frame marker +constexpr byte sof3_ = 0xc3; //!< JPEG Start-Of-Frame marker + +// Start of Frame markers, differential Huffman-coding frames +constexpr byte sof5_ = 0xc5; //!< JPEG Start-Of-Frame marker +constexpr byte sof6_ = 0xc6; //!< JPEG Start-Of-Frame marker +constexpr byte sof7_ = 0xc7; //!< JPEG Start-Of-Frame marker + +// Start of Frame markers, nondifferential arithmetic-coding frames +constexpr byte sof9_ = 0xc9; //!< JPEG Start-Of-Frame marker +constexpr byte sof10_ = 0xca; //!< JPEG Start-Of-Frame marker +constexpr byte sof11_ = 0xcb; //!< JPEG Start-Of-Frame marker + +// Start of Frame markers, differential arithmetic-coding frames +constexpr byte sof13_ = 0xcd; //!< JPEG Start-Of-Frame marker +constexpr byte sof14_ = 0xce; //!< JPEG Start-Of-Frame marker +constexpr byte sof15_ = 0xcf; //!< JPEG Start-Of-Frame marker + +constexpr auto exifId_ = "Exif\0\0"; //!< Exif identifier +constexpr auto jfifId_ = "JFIF\0"; //!< JFIF identifier +constexpr auto xmpId_ = "http://ns.adobe.com/xap/1.0/\0"; //!< XMP packet identifier +constexpr auto iccId_ = "ICC_PROFILE\0"; //!< ICC profile identifier + inline bool inRange(int lo, int value, int hi) { return lo <= value && value <= hi; } @@ -1162,7 +1201,7 @@ bool isJpegType(BasicIo& iIo, bool advance) { if (iIo.error() || iIo.eof()) return false; - if (0xff != tmpBuf[0] || JpegImage::soi_ != tmpBuf[1]) { + if (0xff != tmpBuf[0] || soi_ != tmpBuf[1]) { result = false; } if (!advance || !result) From 24d2a7b8f0933b08b1dc8073aee33907631b52f3 Mon Sep 17 00:00:00 2001 From: Luis Diaz Date: Mon, 4 Apr 2022 17:29:10 +0200 Subject: [PATCH 3/9] JpegBase::markerHasLength moved to implementation details --- include/exiv2/jpgimage.hpp | 9 +-------- src/jpgimage.cpp | 18 ++++++++++++------ 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/include/exiv2/jpgimage.hpp b/include/exiv2/jpgimage.hpp index 0b36d719..05cbc6a8 100644 --- a/include/exiv2/jpgimage.hpp +++ b/include/exiv2/jpgimage.hpp @@ -187,14 +187,7 @@ class EXIV2API JpegBase : public Image { //@} DataBuf readNextSegment(byte marker); - - /*! - @brief Is the marker followed by a non-zero payload? - @param marker The marker at the start of a segment - @return true if the marker is followed by a non-zero payload - */ - static bool markerHasLength(byte marker); -}; // class JpegBase +}; /*! @brief Class to access JPEG images diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index 727181cb..2b695087 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -39,7 +39,7 @@ constexpr byte app1_ = 0xe1; //!< JPEG APP1 marker constexpr byte app2_ = 0xe2; //!< JPEG APP2 marker constexpr byte app13_ = 0xed; //!< JPEG APP13 marker constexpr byte com_ = 0xfe; //!< JPEG Comment marker -constexpr byte soi_ = 0xd8; ///!< SOI marker +constexpr byte soi_ = 0xd8; ///!< SOI marker // Start of Frame markers, nondifferential Huffman-coding frames constexpr byte sof0_ = 0xc0; //!< JPEG Start-Of-Frame marker @@ -74,6 +74,16 @@ inline bool inRange(int lo, int value, int hi) { inline bool inRange2(int value, int lo1, int hi1, int lo2, int hi2) { return inRange(lo1, value, hi1) || inRange(lo2, value, hi2); } + +/// @brief has the segment a non-zero payload? +/// @param marker The marker at the start of a segment +/// @return true if the segment has a length field/payload +bool markerHasLength(byte marker) { + /// \todo there are less markers without payload. Maybe we should revert the logic. + return (marker >= sof0_ && marker <= sof15_) || (marker >= app0_ && marker <= (app0_ | 0x0F)) || marker == dht_ || + marker == dqt_ || marker == dri_ || marker == com_ || marker == sos_; +} + } // namespace bool Photoshop::isIrb(const byte* pPsData) { @@ -250,11 +260,6 @@ DataBuf Photoshop::setIptcIrb(const byte* pPsData, size_t sizePsData, const Iptc return rc; } -bool JpegBase::markerHasLength(byte marker) { - return (marker >= sof0_ && marker <= sof15_) || (marker >= app0_ && marker <= (app0_ | 0x0F)) || marker == dht_ || - marker == dqt_ || marker == dri_ || marker == com_ || marker == sos_; -} - JpegBase::JpegBase(ImageType type, BasicIo::UniquePtr io, bool create, const byte initData[], size_t dataSize) : Image(type, mdExif | mdIptc | mdXmp | mdComment, std::move(io)) { if (create) { @@ -314,6 +319,7 @@ void JpegBase::readMetadata() { byte marker = advanceToMarker(ErrorCode::kerNotAJpeg); while (marker != sos_ && marker != eoi_ && search > 0) { + /// @todo the block to read the size of the segment is repeated in 3 places. Factor-out // 2-byte buffer for reading the size. std::array sizebuf; uint16_t size = 0; // Size of the segment, including the 2-byte size field From 400632f27b410b89df70d1c37c972aad22caf7fa Mon Sep 17 00:00:00 2001 From: Luis Diaz Date: Mon, 4 Apr 2022 17:35:15 +0200 Subject: [PATCH 4/9] Factor out function readSegmentSize() --- src/jpgimage.cpp | 35 +++++++++++++---------------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index 2b695087..384a33df 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -84,6 +84,14 @@ bool markerHasLength(byte marker) { marker == dqt_ || marker == dri_ || marker == com_ || marker == sos_; } +void readSegmentSize(const byte marker, BasicIo& io, std::array& buf, std::uint16_t& size) { + if (markerHasLength(marker)) { + io.readOrThrow(buf.data(), buf.size(), ErrorCode::kerFailedToReadImageData); + size = getUShort(buf.data(), bigEndian); + enforce(size >= 2, ErrorCode::kerFailedToReadImageData); + } +} + } // namespace bool Photoshop::isIrb(const byte* pPsData) { @@ -320,14 +328,9 @@ void JpegBase::readMetadata() { while (marker != sos_ && marker != eoi_ && search > 0) { /// @todo the block to read the size of the segment is repeated in 3 places. Factor-out - // 2-byte buffer for reading the size. - std::array sizebuf; - uint16_t size = 0; // Size of the segment, including the 2-byte size field - if (markerHasLength(marker)) { - io_->readOrThrow(sizebuf.data(), sizebuf.size(), ErrorCode::kerFailedToReadImageData); - size = getUShort(sizebuf.data(), bigEndian); - enforce(size >= 2, ErrorCode::kerFailedToReadImageData); - } + std::array sizebuf; // 2-byte buffer for reading the size. + uint16_t size = 0; // Size of the segment, including the 2-byte size field + readSegmentSize(marker, *io_, sizebuf, size); // Read the rest of the segment. DataBuf buf(size); @@ -530,13 +533,7 @@ void JpegBase::printStructure(std::ostream& out, PrintStructureOption option, in // 2-byte buffer for reading the size. std::array sizebuf; uint16_t size = 0; - if (markerHasLength(marker)) { - io_->readOrThrow(sizebuf.data(), sizebuf.size(), ErrorCode::kerFailedToReadImageData); - size = getUShort(sizebuf.data(), bigEndian); - // `size` is the size of the segment, including the 2-byte size field - // that we just read. - enforce(size >= 2, ErrorCode::kerFailedToReadImageData); - } + readSegmentSize(marker, *io_, sizebuf, size); // Read the rest of the segment. DataBuf buf(size); @@ -783,13 +780,7 @@ DataBuf JpegBase::readNextSegment(byte marker) { // 2-byte buffer for reading the size. std::array sizebuf; uint16_t size = 0; - if (markerHasLength(marker)) { - io_->readOrThrow(sizebuf.data(), sizebuf.size(), ErrorCode::kerFailedToReadImageData); - size = getUShort(sizebuf.data(), bigEndian); - // `size` is the size of the segment, including the 2-byte size field - // that we just read. - enforce(size >= 2, ErrorCode::kerFailedToReadImageData); - } + readSegmentSize(marker, *io_, sizebuf, size); // Read the rest of the segment. DataBuf buf(size); From 047f6b733e41553987a978607b451927d4b894f6 Mon Sep 17 00:00:00 2001 From: Luis Diaz Date: Mon, 4 Apr 2022 17:59:31 +0200 Subject: [PATCH 5/9] Change logic to determine if segment has size Note that the failing tests that had to be adapted were bad formed files from FUZZERs. We should not consider invalid markers like 0x00 or 0x52 but only undefined APPn markers. --- include/exiv2/jpgimage.hpp | 3 +- src/jpgimage.cpp | 35 ++++++++---------------- tests/bugfixes/github/test_issue_1833.py | 6 ++-- tests/bugfixes/github/test_issue_1881.py | 11 +++++--- tests/bugfixes/github/test_issue_756.py | 13 ++++----- 5 files changed, 29 insertions(+), 39 deletions(-) diff --git a/include/exiv2/jpgimage.hpp b/include/exiv2/jpgimage.hpp index 05cbc6a8..b352df98 100644 --- a/include/exiv2/jpgimage.hpp +++ b/include/exiv2/jpgimage.hpp @@ -150,7 +150,6 @@ class EXIV2API JpegBase : public Image { virtual int writeHeader(BasicIo& oIo) const = 0; //@} - private: //! @name Manipulators //@{ @@ -246,7 +245,7 @@ class EXIV2API JpegImage : public JpegBase { private: // Constant data - static const byte blank_[]; ///< Minimal Jpeg image + static const byte blank_[]; ///< Minimal Jpeg image }; //! Helper class to access %Exiv2 files diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index 384a33df..9f677d6b 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -29,41 +29,30 @@ namespace Exiv2 { namespace { // JPEG Segment markers (The first byte is always 0xFF, the value of these constants correspond to the 2nd byte) -constexpr byte dht_ = 0xc4; //!< JPEG DHT marker: Define Huffman Table(s) -constexpr byte dqt_ = 0xdb; //!< JPEG DQT marker: Define Quantization Table(s) -constexpr byte dri_ = 0xdd; //!< JPEG DRI marker constexpr byte sos_ = 0xda; //!< JPEG SOS marker -constexpr byte eoi_ = 0xd9; //!< JPEG EOI marker constexpr byte app0_ = 0xe0; //!< JPEG APP0 marker constexpr byte app1_ = 0xe1; //!< JPEG APP1 marker constexpr byte app2_ = 0xe2; //!< JPEG APP2 marker constexpr byte app13_ = 0xed; //!< JPEG APP13 marker constexpr byte com_ = 0xfe; //!< JPEG Comment marker -constexpr byte soi_ = 0xd8; ///!< SOI marker + +// Markers without payload +constexpr byte soi_ = 0xd8; ///!< SOI marker +constexpr byte eoi_ = 0xd9; //!< JPEG EOI marker +constexpr byte rst1_ = 0xd0; //!< JPEG Restart 0 Marker (from 0xD0 to 0xD7 there might be 8 of these markers) // Start of Frame markers, nondifferential Huffman-coding frames constexpr byte sof0_ = 0xc0; //!< JPEG Start-Of-Frame marker -constexpr byte sof1_ = 0xc1; //!< JPEG Start-Of-Frame marker -constexpr byte sof2_ = 0xc2; //!< JPEG Start-Of-Frame marker constexpr byte sof3_ = 0xc3; //!< JPEG Start-Of-Frame marker // Start of Frame markers, differential Huffman-coding frames constexpr byte sof5_ = 0xc5; //!< JPEG Start-Of-Frame marker -constexpr byte sof6_ = 0xc6; //!< JPEG Start-Of-Frame marker -constexpr byte sof7_ = 0xc7; //!< JPEG Start-Of-Frame marker - -// Start of Frame markers, nondifferential arithmetic-coding frames -constexpr byte sof9_ = 0xc9; //!< JPEG Start-Of-Frame marker -constexpr byte sof10_ = 0xca; //!< JPEG Start-Of-Frame marker -constexpr byte sof11_ = 0xcb; //!< JPEG Start-Of-Frame marker // Start of Frame markers, differential arithmetic-coding frames -constexpr byte sof13_ = 0xcd; //!< JPEG Start-Of-Frame marker -constexpr byte sof14_ = 0xce; //!< JPEG Start-Of-Frame marker constexpr byte sof15_ = 0xcf; //!< JPEG Start-Of-Frame marker -constexpr auto exifId_ = "Exif\0\0"; //!< Exif identifier -constexpr auto jfifId_ = "JFIF\0"; //!< JFIF identifier +constexpr auto exifId_ = "Exif\0\0"; //!< Exif identifier +// constexpr auto jfifId_ = "JFIF\0"; //!< JFIF identifier constexpr auto xmpId_ = "http://ns.adobe.com/xap/1.0/\0"; //!< XMP packet identifier constexpr auto iccId_ = "ICC_PROFILE\0"; //!< ICC profile identifier @@ -76,12 +65,11 @@ inline bool inRange2(int value, int lo1, int hi1, int lo2, int hi2) { } /// @brief has the segment a non-zero payload? -/// @param marker The marker at the start of a segment +/// @param m The marker at the start of a segment /// @return true if the segment has a length field/payload -bool markerHasLength(byte marker) { - /// \todo there are less markers without payload. Maybe we should revert the logic. - return (marker >= sof0_ && marker <= sof15_) || (marker >= app0_ && marker <= (app0_ | 0x0F)) || marker == dht_ || - marker == dqt_ || marker == dri_ || marker == com_ || marker == sos_; +bool markerHasLength(byte m) { + bool markerWithoutLength = m >= rst1_ && m <= eoi_; + return !markerWithoutLength; } void readSegmentSize(const byte marker, BasicIo& io, std::array& buf, std::uint16_t& size) { @@ -294,6 +282,7 @@ byte JpegBase::advanceToMarker(ErrorCode err) const { throw Error(err); } + /// \todo should we check for validity of the marker? 0x59 does not look fine // Markers can start with any number of 0xff while ((c = io_->getb()) == 0xff) { } diff --git a/tests/bugfixes/github/test_issue_1833.py b/tests/bugfixes/github/test_issue_1833.py index 5508e08f..25582729 100644 --- a/tests/bugfixes/github/test_issue_1833.py +++ b/tests/bugfixes/github/test_issue_1833.py @@ -11,7 +11,9 @@ class TiffMnEntryDoCountInvalidTiffType(metaclass=CaseMeta): filename = path("$data_path/issue_1833_poc.jpg") commands = ["$exiv2 -pS $filename"] - stderr = [""] - retval = [0] + stderr = ["""$exiv2_exception_message """ + filename + """: +$kerFailedToReadImageData +"""] + retval = [1] compare_stdout = check_no_ASAN_UBSAN_errors diff --git a/tests/bugfixes/github/test_issue_1881.py b/tests/bugfixes/github/test_issue_1881.py index bd8b5394..db79a6d2 100644 --- a/tests/bugfixes/github/test_issue_1881.py +++ b/tests/bugfixes/github/test_issue_1881.py @@ -1,6 +1,6 @@ # -*- coding: utf-8 -*- -from system_tests import CaseMeta, CopyTmpFiles, path +from system_tests import CaseMeta, CopyTmpFiles, path, check_no_ASAN_UBSAN_errors @CopyTmpFiles("$data_path/issue_1881_poc.jpg", "$data_path/issue_1881_coverage.jpg") class SonyPreviewImageLargeAllocation(metaclass=CaseMeta): @@ -13,6 +13,9 @@ class SonyPreviewImageLargeAllocation(metaclass=CaseMeta): filename1 = path("$tmp_path/issue_1881_poc.jpg") filename2 = path("$tmp_path/issue_1881_coverage.jpg") commands = ["$exiv2 -q -d I rm $filename1", "$exiv2 -q -d I rm $filename2"] - stdout = ["",""] - stderr = ["",""] - retval = [0,0] + stderr = ["""$exception_in_erase """ + filename1 + """: +$kerFailedToReadImageData +""", ""] + retval = [1,0] + + compare_stdout = check_no_ASAN_UBSAN_errors diff --git a/tests/bugfixes/github/test_issue_756.py b/tests/bugfixes/github/test_issue_756.py index 464a69ce..3987da7b 100644 --- a/tests/bugfixes/github/test_issue_756.py +++ b/tests/bugfixes/github/test_issue_756.py @@ -1,6 +1,5 @@ import system_tests - class BufferOverReadInNikon1MakerNotePrint0x0088( metaclass=system_tests.CaseMeta): @@ -9,12 +8,10 @@ class BufferOverReadInNikon1MakerNotePrint0x0088( filename = system_tests.path( "$data_path/NikonMakerNotePrint0x088_overread" ) - commands = ["$exiv2 -pt --grep AFFocusPos $filename"] - stdout = [ - """Exif.Nikon1.AFFocusPos Undefined 4 Invalid value; Center -""" - ] - stderr = [""] - retval = [0] + commands = ["$exiv2 -q -pt --grep AFFocusPos $filename"] + stderr = ["""$exiv2_exception_message """ + filename + """: +$kerFailedToReadImageData +"""] + retval = [1] compare_stderr = system_tests.check_no_ASAN_UBSAN_errors From f942ba89bd67d15af1db55975a7de480a980dfa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20D=C3=ADaz=20M=C3=A1s?= Date: Thu, 7 Apr 2022 16:42:16 +0200 Subject: [PATCH 6/9] Move Photoshopb class to internal namespace --- app/exiv2.cpp | 1 + include/exiv2/exiv2.hpp | 1 + include/exiv2/jpgimage.hpp | 56 ----------- include/exiv2/photoshop.hpp | 70 ++++++++++++++ src/CMakeLists.txt | 2 + src/image.cpp | 1 + src/jpgimage.cpp | 180 +--------------------------------- src/photoshop.cpp | 182 +++++++++++++++++++++++++++++++++++ src/pngchunk_int.cpp | 2 + src/pngimage.cpp | 2 + src/preview.cpp | 1 + src/psdimage.cpp | 1 + src/tiffvisitor_int.cpp | 1 + unitTests/test_Photoshop.cpp | 5 +- 14 files changed, 271 insertions(+), 234 deletions(-) create mode 100644 include/exiv2/photoshop.hpp create mode 100644 src/photoshop.cpp diff --git a/app/exiv2.cpp b/app/exiv2.cpp index 18654f3b..84459f60 100644 --- a/app/exiv2.cpp +++ b/app/exiv2.cpp @@ -13,6 +13,7 @@ #include "xmp_exiv2.hpp" #include +#include #include #include #include diff --git a/include/exiv2/exiv2.hpp b/include/exiv2/exiv2.hpp index 66d74d5d..2798f7d5 100644 --- a/include/exiv2/exiv2.hpp +++ b/include/exiv2/exiv2.hpp @@ -29,6 +29,7 @@ #include "exiv2/mrwimage.hpp" #include "exiv2/orfimage.hpp" #include "exiv2/pgfimage.hpp" +#include "exiv2/photoshop.hpp" #ifdef EXV_HAVE_LIBZ #include "exiv2/pngimage.hpp" diff --git a/include/exiv2/jpgimage.hpp b/include/exiv2/jpgimage.hpp index b352df98..128903d7 100644 --- a/include/exiv2/jpgimage.hpp +++ b/include/exiv2/jpgimage.hpp @@ -3,11 +3,8 @@ #ifndef JPGIMAGE_HPP_ #define JPGIMAGE_HPP_ -// ***************************************************************************** #include "exiv2lib_export.h" -#include - // included header files #include "error.hpp" #include "image.hpp" @@ -18,59 +15,6 @@ namespace Exiv2 { // ***************************************************************************** // class definitions -/// @brief Helper class, has methods to deal with %Photoshop "Information Resource Blocks" (IRBs). -struct EXIV2API Photoshop { - // Todo: Public for now - static constexpr std::array irbId_{"8BIM", "AgHg", "DCSR", "PHUT"}; //!< %Photoshop IRB markers - static constexpr auto ps3Id_ = "Photoshop 3.0\0"; //!< %Photoshop marker - static constexpr uint16_t iptc_ = 0x0404; //!< %Photoshop IPTC marker - static constexpr uint16_t preview_ = 0x040c; //!< %Photoshop preview marker - - /// @brief Checks an IRB - /// @param pPsData Existing IRB buffer. It is expected to be of size 4. - /// @return true if the IRB marker is known - /// @todo This should be an implementation detail and not exposed in the API. An attacker could try to pass - /// a smaller buffer or null pointer. - static bool isIrb(const byte* pPsData); - - /// @brief Validates all IRBs - /// @param pPsData Existing IRB buffer - /// @param sizePsData Size of the IRB buffer, may be 0 - /// @return true if all IRBs are valid;
false otherwise - static bool valid(const byte* pPsData, size_t sizePsData); - - /// @brief Locates the data for a %Photoshop tag in a %Photoshop formated memory buffer. - /// Operates on raw data to simplify reuse. - /// @param pPsData Pointer to buffer containing entire payload of %Photoshop formated data (from APP13 Jpeg segment) - /// @param sizePsData Size in bytes of pPsData. - /// @param psTag %Tag number of the block to look for. - /// @param record Output value that is set to the start of the data block within pPsData (may not be null). - /// @param sizeHdr Output value that is set to the size of the header within the data block pointed to by record - /// (may not be null). - /// @param sizeData Output value that is set to the size of the actual data within the data block pointed to by record - /// (may not be null). - /// @return 0 if successful;
- /// 3 if no data for psTag was found in pPsData;
- /// -2 if the pPsData buffer does not contain valid data. - static int locateIrb(const byte* pPsData, size_t sizePsData, uint16_t psTag, const byte** record, - uint32_t* const sizeHdr, uint32_t* const sizeData); - - /// @brief Forwards to locateIrb() with \em psTag = \em iptc_ - static int locateIptcIrb(const byte* pPsData, size_t sizePsData, const byte** record, uint32_t* const sizeHdr, - uint32_t* const sizeData); - - /// @brief Forwards to locatePreviewIrb() with \em psTag = \em preview_ - static int locatePreviewIrb(const byte* pPsData, size_t sizePsData, const byte** record, uint32_t* const sizeHdr, - uint32_t* const sizeData); - - /// @brief Set the new IPTC IRB, keeps existing IRBs but removes the IPTC block if there is no new IPTC data to write. - /// @param pPsData Existing IRB buffer - /// @param sizePsData Size of the IRB buffer, may be 0 - /// @param iptcData Iptc data to embed, may be empty - /// @return A data buffer containing the new IRB buffer, may have 0 size - static DataBuf setIptcIrb(const byte* pPsData, size_t sizePsData, const IptcData& iptcData); -}; - /*! @brief Abstract helper base class to access JPEG images. */ diff --git a/include/exiv2/photoshop.hpp b/include/exiv2/photoshop.hpp new file mode 100644 index 00000000..c0c3ae3c --- /dev/null +++ b/include/exiv2/photoshop.hpp @@ -0,0 +1,70 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +#ifndef PHOTOSHOP_INT_HPP +#define PHOTOSHOP_INT_HPP + +#include "exiv2lib_export.h" + +#include "types.hpp" + +#include + +namespace Exiv2 { +// Forward declarations +class IptcData; + +/// @brief Helper class, has methods to deal with %Photoshop "Information Resource Blocks" (IRBs). +struct EXIV2API Photoshop { + // Todo: Public for now + static constexpr std::array irbId_{"8BIM", "AgHg", "DCSR", "PHUT"}; //!< %Photoshop IRB markers + static constexpr auto ps3Id_ = "Photoshop 3.0\0"; //!< %Photoshop marker + static constexpr uint16_t iptc_ = 0x0404; //!< %Photoshop IPTC marker + static constexpr uint16_t preview_ = 0x040c; //!< %Photoshop preview marker + + /// @brief Checks an IRB + /// @param pPsData Existing IRB buffer. It is expected to be of size 4. + /// @return true if the IRB marker is known + /// @todo This should be an implementation detail and not exposed in the API. An attacker could try to pass + /// a smaller buffer or null pointer. + static bool isIrb(const byte* pPsData); + + /// @brief Validates all IRBs + /// @param pPsData Existing IRB buffer + /// @param sizePsData Size of the IRB buffer, may be 0 + /// @return true if all IRBs are valid;
false otherwise + static bool valid(const byte* pPsData, size_t sizePsData); + + /// @brief Locates the data for a %Photoshop tag in a %Photoshop formated memory buffer. + /// Operates on raw data to simplify reuse. + /// @param pPsData Pointer to buffer containing entire payload of %Photoshop formated data (from APP13 Jpeg segment) + /// @param sizePsData Size in bytes of pPsData. + /// @param psTag %Tag number of the block to look for. + /// @param record Output value that is set to the start of the data block within pPsData (may not be null). + /// @param sizeHdr Output value that is set to the size of the header within the data block pointed to by record + /// (may not be null). + /// @param sizeData Output value that is set to the size of the actual data within the data block pointed to by record + /// (may not be null). + /// @return 0 if successful;
+ /// 3 if no data for psTag was found in pPsData;
+ /// -2 if the pPsData buffer does not contain valid data. + static int locateIrb(const byte* pPsData, size_t sizePsData, uint16_t psTag, const byte** record, + uint32_t* const sizeHdr, uint32_t* const sizeData); + + /// @brief Forwards to locateIrb() with \em psTag = \em iptc_ + static int locateIptcIrb(const byte* pPsData, size_t sizePsData, const byte** record, uint32_t* const sizeHdr, + uint32_t* const sizeData); + + /// @brief Forwards to locatePreviewIrb() with \em psTag = \em preview_ + static int locatePreviewIrb(const byte* pPsData, size_t sizePsData, const byte** record, uint32_t* const sizeHdr, + uint32_t* const sizeData); + + /// @brief Set the new IPTC IRB, keeps existing IRBs but removes the IPTC block if there is no new IPTC data to write. + /// @param pPsData Existing IRB buffer + /// @param sizePsData Size of the IRB buffer, may be 0 + /// @param iptcData Iptc data to embed, may be empty + /// @return A data buffer containing the new IRB buffer, may have 0 size + static DataBuf setIptcIrb(const byte* pPsData, size_t sizePsData, const IptcData& iptcData); +}; +} // namespace Exiv2 + +#endif // PHOTOSHOP_INT_HPP diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 01bfe5df..92126f57 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -65,6 +65,7 @@ set(PUBLIC_HEADERS ../include/exiv2/mrwimage.hpp ../include/exiv2/orfimage.hpp ../include/exiv2/pgfimage.hpp + ../include/exiv2/photoshop.hpp ../include/exiv2/preview.hpp ../include/exiv2/properties.hpp ../include/exiv2/psdimage.hpp @@ -107,6 +108,7 @@ add_library( exiv2lib mrwimage.cpp orfimage.cpp pgfimage.cpp + photoshop.cpp preview.cpp properties.cpp psdimage.cpp diff --git a/src/image.cpp b/src/image.cpp index 30c75d9c..ad8c2dcc 100644 --- a/src/image.cpp +++ b/src/image.cpp @@ -38,6 +38,7 @@ #include "xmpsidecar.hpp" // + standard includes +#include #include #include #include diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index 9f677d6b..023f4c56 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -9,6 +9,7 @@ #include "helper_functions.hpp" #include "image_int.hpp" #include "jpgimage.hpp" +#include "photoshop.hpp" #include "safe_op.hpp" #ifdef WIN32 @@ -21,12 +22,14 @@ #include "fff.h" +#include #include // ***************************************************************************** // class member definitions namespace Exiv2 { + namespace { // JPEG Segment markers (The first byte is always 0xFF, the value of these constants correspond to the 2nd byte) constexpr byte sos_ = 0xda; //!< JPEG SOS marker @@ -79,183 +82,8 @@ void readSegmentSize(const byte marker, BasicIo& io, std::array& buf, s enforce(size >= 2, ErrorCode::kerFailedToReadImageData); } } - } // namespace -bool Photoshop::isIrb(const byte* pPsData) { - if (pPsData == nullptr) { - return false; - } - /// \todo check if direct array comparison is faster than a call to memcmp - return std::any_of(irbId_.begin(), irbId_.end(), [pPsData](auto id) { return memcmp(pPsData, id, 4) == 0; }); -} - -bool Photoshop::valid(const byte* pPsData, size_t sizePsData) { - const byte* record = nullptr; - uint32_t sizeIptc = 0; - uint32_t sizeHdr = 0; - const byte* pCur = pPsData; - const byte* pEnd = pPsData + sizePsData; - int ret = 0; - while (pCur < pEnd && 0 == (ret = Photoshop::locateIptcIrb(pCur, (pEnd - pCur), &record, &sizeHdr, &sizeIptc))) { - pCur = record + sizeHdr + sizeIptc + (sizeIptc & 1); - } - return ret >= 0; -} - -// Todo: Generalised from JpegBase::locateIptcData without really understanding -// the format (in particular the header). So it remains to be confirmed -// if this also makes sense for psTag != Photoshop::iptc -int Photoshop::locateIrb(const byte* pPsData, size_t sizePsData, uint16_t psTag, const byte** record, - uint32_t* const sizeHdr, uint32_t* const sizeData) { - if (sizePsData < 12) { - return 3; - } - - // Used for error checking - size_t position = 0; -#ifdef EXIV2_DEBUG_MESSAGES - std::cerr << "Photoshop::locateIrb: "; -#endif - // Data should follow Photoshop format, if not exit - while (position <= (sizePsData - 12) && isIrb(pPsData + position)) { - const byte* hrd = pPsData + position; - position += 4; - uint16_t type = getUShort(pPsData + position, bigEndian); - position += 2; -#ifdef EXIV2_DEBUG_MESSAGES - std::cerr << "0x" << std::hex << type << std::dec << " "; -#endif - // Pascal string is padded to have an even size (including size byte) - byte psSize = pPsData[position] + 1; - psSize += (psSize & 1); - position += psSize; - if (position + 4 > sizePsData) { -#ifdef EXIV2_DEBUG_MESSAGES - std::cerr << "Warning: " - << "Invalid or extended Photoshop IRB\n"; -#endif - return -2; - } - uint32_t dataSize = getULong(pPsData + position, bigEndian); - position += 4; - if (dataSize > (sizePsData - position)) { -#ifdef EXIV2_DEBUG_MESSAGES - std::cerr << "Warning: " - << "Invalid Photoshop IRB data size " << dataSize << " or extended Photoshop IRB\n"; -#endif - return -2; - } -#ifdef EXIV2_DEBUG_MESSAGES - if ((dataSize & 1) && position + dataSize == sizePsData) { - std::cerr << "Warning: " - << "Photoshop IRB data is not padded to even size\n"; - } -#endif - if (type == psTag) { -#ifdef EXIV2_DEBUG_MESSAGES - std::cerr << "ok\n"; -#endif - *sizeData = dataSize; - *sizeHdr = psSize + 10; - *record = hrd; - return 0; - } - // Data size is also padded to be even - position += dataSize + (dataSize & 1); - } -#ifdef EXIV2_DEBUG_MESSAGES - std::cerr << "pPsData doesn't start with '8BIM'\n"; -#endif - if (position < sizePsData) { -#ifdef EXIV2_DEBUG_MESSAGES - std::cerr << "Warning: " - << "Invalid or extended Photoshop IRB\n"; -#endif - return -2; - } - return 3; -} - -int Photoshop::locateIptcIrb(const byte* pPsData, size_t sizePsData, const byte** record, uint32_t* const sizeHdr, - uint32_t* const sizeData) { - return locateIrb(pPsData, sizePsData, iptc_, record, sizeHdr, sizeData); -} - -int Photoshop::locatePreviewIrb(const byte* pPsData, size_t sizePsData, const byte** record, uint32_t* const sizeHdr, - uint32_t* const sizeData) { - return locateIrb(pPsData, sizePsData, preview_, record, sizeHdr, sizeData); -} - -DataBuf Photoshop::setIptcIrb(const byte* pPsData, size_t sizePsData, const IptcData& iptcData) { -#ifdef EXIV2_DEBUG_MESSAGES - std::cerr << "IRB block at the beginning of Photoshop::setIptcIrb\n"; - if (sizePsData == 0) - std::cerr << " None.\n"; - else - hexdump(std::cerr, pPsData, sizePsData); -#endif - const byte* record = pPsData; - uint32_t sizeIptc = 0; - uint32_t sizeHdr = 0; - DataBuf rc; - if (0 > Photoshop::locateIptcIrb(pPsData, sizePsData, &record, &sizeHdr, &sizeIptc)) { - return rc; - } - - Blob psBlob; - const auto sizeFront = static_cast(record - pPsData); - // Write data before old record. - if (sizePsData > 0 && sizeFront > 0) { - append(psBlob, pPsData, sizeFront); - } - - // Write new iptc record if we have it - DataBuf rawIptc = IptcParser::encode(iptcData); - if (!rawIptc.empty()) { - std::array tmpBuf; - std::copy_n(Photoshop::irbId_[0], 4, tmpBuf.data()); - us2Data(tmpBuf.data() + 4, iptc_, bigEndian); - tmpBuf[6] = 0; - tmpBuf[7] = 0; - ul2Data(tmpBuf.data() + 8, static_cast(rawIptc.size()), bigEndian); - append(psBlob, tmpBuf.data(), 12); - append(psBlob, rawIptc.c_data(), rawIptc.size()); - // Data is padded to be even (but not included in size) - if (rawIptc.size() & 1) - psBlob.push_back(0x00); - } - - // Write existing stuff after record, skip the current and all remaining IPTC blocks - size_t pos = sizeFront; - auto nextSizeData = Safe::add(static_cast(sizePsData), -static_cast(pos)); - enforce(nextSizeData >= 0, ErrorCode::kerCorruptedMetadata); - while (0 == Photoshop::locateIptcIrb(pPsData + pos, nextSizeData, &record, &sizeHdr, &sizeIptc)) { - const auto newPos = static_cast(record - pPsData); - if (newPos > pos) { // Copy data up to the IPTC IRB - append(psBlob, pPsData + pos, newPos - pos); - } - pos = newPos + sizeHdr + sizeIptc + (sizeIptc & 1); // Skip the IPTC IRB - nextSizeData = Safe::add(static_cast(sizePsData), -static_cast(pos)); - enforce(nextSizeData >= 0, ErrorCode::kerCorruptedMetadata); - } - if (pos < sizePsData) { - append(psBlob, pPsData + pos, sizePsData - pos); - } - - // Data is rounded to be even - if (!psBlob.empty()) - rc = DataBuf(&psBlob[0], psBlob.size()); -#ifdef EXIV2_DEBUG_MESSAGES - std::cerr << "IRB block at the end of Photoshop::setIptcIrb\n"; - if (rc.empty()) - std::cerr << " None.\n"; - else - hexdump(std::cerr, rc.c_data(), rc.size()); -#endif - return rc; -} - JpegBase::JpegBase(ImageType type, BasicIo::UniquePtr io, bool create, const byte initData[], size_t dataSize) : Image(type, mdExif | mdIptc | mdXmp | mdComment, std::move(io)) { if (create) { @@ -282,7 +110,6 @@ byte JpegBase::advanceToMarker(ErrorCode err) const { throw Error(err); } - /// \todo should we check for validity of the marker? 0x59 does not look fine // Markers can start with any number of 0xff while ((c = io_->getb()) == 0xff) { } @@ -316,7 +143,6 @@ void JpegBase::readMetadata() { byte marker = advanceToMarker(ErrorCode::kerNotAJpeg); while (marker != sos_ && marker != eoi_ && search > 0) { - /// @todo the block to read the size of the segment is repeated in 3 places. Factor-out std::array sizebuf; // 2-byte buffer for reading the size. uint16_t size = 0; // Size of the segment, including the 2-byte size field readSegmentSize(marker, *io_, sizebuf, size); diff --git a/src/photoshop.cpp b/src/photoshop.cpp new file mode 100644 index 00000000..cd7f6e52 --- /dev/null +++ b/src/photoshop.cpp @@ -0,0 +1,182 @@ +#include "photoshop.hpp" + +#include "enforce.hpp" +#include "image.hpp" +#include "safe_op.hpp" + +namespace Exiv2 { + +bool Photoshop::isIrb(const byte* data) { + if (data == nullptr) { + return false; + } + return std::any_of(irbId_.begin(), irbId_.end(), [data](auto id) { return memcmp(data, id, 4) == 0; }); +} + +bool Photoshop::valid(const byte* pPsData, size_t sizePsData) { + const byte* record = nullptr; + uint32_t sizeIptc = 0; + uint32_t sizeHdr = 0; + const byte* pCur = pPsData; + const byte* pEnd = pPsData + sizePsData; + int ret = 0; + while (pCur < pEnd && 0 == (ret = Photoshop::locateIptcIrb(pCur, (pEnd - pCur), &record, &sizeHdr, &sizeIptc))) { + pCur = record + sizeHdr + sizeIptc + (sizeIptc & 1); + } + return ret >= 0; +} + +// Todo: Generalised from JpegBase::locateIptcData without really understanding +// the format (in particular the header). So it remains to be confirmed +// if this also makes sense for psTag != Photoshop::iptc +int Photoshop::locateIrb(const byte* pPsData, size_t sizePsData, uint16_t psTag, const byte** record, + uint32_t* const sizeHdr, uint32_t* const sizeData) { + if (sizePsData < 12) { + return 3; + } + + // Used for error checking + size_t position = 0; +#ifdef EXIV2_DEBUG_MESSAGES + std::cerr << "Photoshop::locateIrb: "; +#endif + // Data should follow Photoshop format, if not exit + while (position <= (sizePsData - 12) && isIrb(pPsData + position)) { + const byte* hrd = pPsData + position; + position += 4; + uint16_t type = getUShort(pPsData + position, bigEndian); + position += 2; +#ifdef EXIV2_DEBUG_MESSAGES + std::cerr << "0x" << std::hex << type << std::dec << " "; +#endif + // Pascal string is padded to have an even size (including size byte) + byte psSize = pPsData[position] + 1; + psSize += (psSize & 1); + position += psSize; + if (position + 4 > sizePsData) { +#ifdef EXIV2_DEBUG_MESSAGES + std::cerr << "Warning: " + << "Invalid or extended Photoshop IRB\n"; +#endif + return -2; + } + uint32_t dataSize = getULong(pPsData + position, bigEndian); + position += 4; + if (dataSize > (sizePsData - position)) { +#ifdef EXIV2_DEBUG_MESSAGES + std::cerr << "Warning: " + << "Invalid Photoshop IRB data size " << dataSize << " or extended Photoshop IRB\n"; +#endif + return -2; + } +#ifdef EXIV2_DEBUG_MESSAGES + if ((dataSize & 1) && position + dataSize == sizePsData) { + std::cerr << "Warning: " + << "Photoshop IRB data is not padded to even size\n"; + } +#endif + if (type == psTag) { +#ifdef EXIV2_DEBUG_MESSAGES + std::cerr << "ok\n"; +#endif + *sizeData = dataSize; + *sizeHdr = psSize + 10; + *record = hrd; + return 0; + } + // Data size is also padded to be even + position += dataSize + (dataSize & 1); + } +#ifdef EXIV2_DEBUG_MESSAGES + std::cerr << "pPsData doesn't start with '8BIM'\n"; +#endif + if (position < sizePsData) { +#ifdef EXIV2_DEBUG_MESSAGES + std::cerr << "Warning: " + << "Invalid or extended Photoshop IRB\n"; +#endif + return -2; + } + return 3; +} + +int Photoshop::locateIptcIrb(const byte* pPsData, size_t sizePsData, const byte** record, uint32_t* const sizeHdr, + uint32_t* const sizeData) { + return locateIrb(pPsData, sizePsData, iptc_, record, sizeHdr, sizeData); +} + +int Photoshop::locatePreviewIrb(const byte* pPsData, size_t sizePsData, const byte** record, uint32_t* const sizeHdr, + uint32_t* const sizeData) { + return locateIrb(pPsData, sizePsData, preview_, record, sizeHdr, sizeData); +} + +DataBuf Photoshop::setIptcIrb(const byte* pPsData, size_t sizePsData, const IptcData& iptcData) { +#ifdef EXIV2_DEBUG_MESSAGES + std::cerr << "IRB block at the beginning of Photoshop::setIptcIrb\n"; + if (sizePsData == 0) + std::cerr << " None.\n"; + else + hexdump(std::cerr, pPsData, sizePsData); +#endif + const byte* record = pPsData; + uint32_t sizeIptc = 0; + uint32_t sizeHdr = 0; + DataBuf rc; + if (0 > Photoshop::locateIptcIrb(pPsData, sizePsData, &record, &sizeHdr, &sizeIptc)) { + return rc; + } + + Blob psBlob; + const auto sizeFront = static_cast(record - pPsData); + // Write data before old record. + if (sizePsData > 0 && sizeFront > 0) { + append(psBlob, pPsData, sizeFront); + } + + // Write new iptc record if we have it + DataBuf rawIptc = IptcParser::encode(iptcData); + if (!rawIptc.empty()) { + std::array tmpBuf; + std::copy_n(Photoshop::irbId_[0], 4, tmpBuf.data()); + us2Data(tmpBuf.data() + 4, iptc_, bigEndian); + tmpBuf[6] = 0; + tmpBuf[7] = 0; + ul2Data(tmpBuf.data() + 8, static_cast(rawIptc.size()), bigEndian); + append(psBlob, tmpBuf.data(), 12); + append(psBlob, rawIptc.c_data(), rawIptc.size()); + // Data is padded to be even (but not included in size) + if (rawIptc.size() & 1) + psBlob.push_back(0x00); + } + + // Write existing stuff after record, skip the current and all remaining IPTC blocks + size_t pos = sizeFront; + long nextSizeData = Safe::add(static_cast(sizePsData), -static_cast(pos)); + enforce(nextSizeData >= 0, ErrorCode::kerCorruptedMetadata); + while (0 == Photoshop::locateIptcIrb(pPsData + pos, nextSizeData, &record, &sizeHdr, &sizeIptc)) { + const auto newPos = static_cast(record - pPsData); + if (newPos > pos) { // Copy data up to the IPTC IRB + append(psBlob, pPsData + pos, newPos - pos); + } + pos = newPos + sizeHdr + sizeIptc + (sizeIptc & 1); // Skip the IPTC IRB + nextSizeData = Safe::add(static_cast(sizePsData), -static_cast(pos)); + enforce(nextSizeData >= 0, ErrorCode::kerCorruptedMetadata); + } + if (pos < sizePsData) { + append(psBlob, pPsData + pos, sizePsData - pos); + } + + // Data is rounded to be even + if (!psBlob.empty()) + rc = DataBuf(&psBlob[0], psBlob.size()); +#ifdef EXIV2_DEBUG_MESSAGES + std::cerr << "IRB block at the end of Photoshop::setIptcIrb\n"; + if (rc.empty()) + std::cerr << " None.\n"; + else + hexdump(std::cerr, rc.c_data(), rc.size()); +#endif + return rc; +} + +} // namespace Exiv2 diff --git a/src/pngchunk_int.cpp b/src/pngchunk_int.cpp index f41f9bf1..4e18431b 100644 --- a/src/pngchunk_int.cpp +++ b/src/pngchunk_int.cpp @@ -13,12 +13,14 @@ #include "image.hpp" #include "iptc.hpp" #include "jpgimage.hpp" +#include "photoshop.hpp" #include "pngchunk_int.hpp" #include "safe_op.hpp" #include "tiffimage.hpp" // standard includes #include +#include #include #include #include diff --git a/src/pngimage.cpp b/src/pngimage.cpp index 0743ab9c..d1c6a363 100644 --- a/src/pngimage.cpp +++ b/src/pngimage.cpp @@ -13,11 +13,13 @@ #include "image.hpp" #include "image_int.hpp" #include "jpgimage.hpp" +#include "photoshop.hpp" #include "pngchunk_int.hpp" #include "pngimage.hpp" #include "tiffimage.hpp" #include "types.hpp" +#include #include namespace { diff --git a/src/preview.cpp b/src/preview.cpp index 82d9899c..ee7e6415 100644 --- a/src/preview.cpp +++ b/src/preview.cpp @@ -8,6 +8,7 @@ #include "futils.hpp" #include "image.hpp" #include "jpgimage.hpp" +#include "photoshop.hpp" #include "safe_op.hpp" #include "tiffimage.hpp" #include "tiffimage_int.hpp" diff --git a/src/psdimage.cpp b/src/psdimage.cpp index 4df8f882..a1e91151 100644 --- a/src/psdimage.cpp +++ b/src/psdimage.cpp @@ -10,6 +10,7 @@ #include "futils.hpp" #include "image.hpp" #include "jpgimage.hpp" +#include "photoshop.hpp" #include diff --git a/src/tiffvisitor_int.cpp b/src/tiffvisitor_int.cpp index ac41f133..190b348e 100644 --- a/src/tiffvisitor_int.cpp +++ b/src/tiffvisitor_int.cpp @@ -8,6 +8,7 @@ #include "iptc.hpp" #include "jpgimage.hpp" #include "makernote_int.hpp" +#include "photoshop.hpp" #include "sonymn_int.hpp" #include "tiffcomposite_int.hpp" // Do not change the order of these 2 includes, #include "tiffimage_int.hpp" diff --git a/unitTests/test_Photoshop.cpp b/unitTests/test_Photoshop.cpp index 98b69e33..1dd31b98 100644 --- a/unitTests/test_Photoshop.cpp +++ b/unitTests/test_Photoshop.cpp @@ -1,6 +1,9 @@ // SPDX-License-Identifier: GPL-2.0-or-later -#include +#include "photoshop.hpp" + +#include "error.hpp" +#include "iptc.hpp" #include From 4ee9c35799a82d18007734790248352ed2c60e9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20D=C3=ADaz=20M=C3=A1s?= Date: Thu, 7 Apr 2022 18:03:03 +0200 Subject: [PATCH 7/9] Include missing iostream header in photoshop.cpp --- src/photoshop.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/photoshop.cpp b/src/photoshop.cpp index cd7f6e52..2aaebf30 100644 --- a/src/photoshop.cpp +++ b/src/photoshop.cpp @@ -4,6 +4,8 @@ #include "image.hpp" #include "safe_op.hpp" +#include + namespace Exiv2 { bool Photoshop::isIrb(const byte* data) { From 4d99c2aca1f2a32a6417cc7f90f468110ba597ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20D=C3=ADaz=20M=C3=A1s?= Date: Fri, 8 Apr 2022 07:50:36 +0200 Subject: [PATCH 8/9] Use std::pair to return multiple values --- src/jpgimage.cpp | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index 023f4c56..ef740cf2 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -75,12 +75,15 @@ bool markerHasLength(byte m) { return !markerWithoutLength; } -void readSegmentSize(const byte marker, BasicIo& io, std::array& buf, std::uint16_t& size) { +std::pair, uint16_t> readSegmentSize(const byte marker, BasicIo& io) { + std::array buf{0, 0}; // 2-byte buffer for reading the size. + uint16_t size{0}; // Size of the segment, including the 2-byte size field if (markerHasLength(marker)) { io.readOrThrow(buf.data(), buf.size(), ErrorCode::kerFailedToReadImageData); size = getUShort(buf.data(), bigEndian); enforce(size >= 2, ErrorCode::kerFailedToReadImageData); } + return {buf, size}; } } // namespace @@ -143,9 +146,7 @@ void JpegBase::readMetadata() { byte marker = advanceToMarker(ErrorCode::kerNotAJpeg); while (marker != sos_ && marker != eoi_ && search > 0) { - std::array sizebuf; // 2-byte buffer for reading the size. - uint16_t size = 0; // Size of the segment, including the 2-byte size field - readSegmentSize(marker, *io_, sizebuf, size); + auto [sizebuf, size] = readSegmentSize(marker, *io_); // Read the rest of the segment. DataBuf buf(size); @@ -345,10 +346,7 @@ void JpegBase::printStructure(std::ostream& out, PrintStructureOption option, in first = false; bool bLF = bPrint; - // 2-byte buffer for reading the size. - std::array sizebuf; - uint16_t size = 0; - readSegmentSize(marker, *io_, sizebuf, size); + auto [sizebuf, size] = readSegmentSize(marker, *io_); // Read the rest of the segment. DataBuf buf(size); @@ -592,10 +590,7 @@ void JpegBase::writeMetadata() { } DataBuf JpegBase::readNextSegment(byte marker) { - // 2-byte buffer for reading the size. - std::array sizebuf; - uint16_t size = 0; - readSegmentSize(marker, *io_, sizebuf, size); + auto [sizebuf, size] = readSegmentSize(marker, *io_); // Read the rest of the segment. DataBuf buf(size); From f07c88de5d2682b6312ac84593190965a1423687 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20D=C3=ADaz=20M=C3=A1s?= Date: Fri, 8 Apr 2022 07:51:01 +0200 Subject: [PATCH 9/9] Use .front() to avoid warning from static analysis --- src/photoshop.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/photoshop.cpp b/src/photoshop.cpp index 2aaebf30..a08fa014 100644 --- a/src/photoshop.cpp +++ b/src/photoshop.cpp @@ -139,7 +139,7 @@ DataBuf Photoshop::setIptcIrb(const byte* pPsData, size_t sizePsData, const Iptc DataBuf rawIptc = IptcParser::encode(iptcData); if (!rawIptc.empty()) { std::array tmpBuf; - std::copy_n(Photoshop::irbId_[0], 4, tmpBuf.data()); + std::copy_n(Photoshop::irbId_.front(), 4, tmpBuf.data()); us2Data(tmpBuf.data() + 4, iptc_, bigEndian); tmpBuf[6] = 0; tmpBuf[7] = 0;