From 81cfc162c1b5e3bde0e222efaff8b7e257db3fc8 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 10 Oct 2021 23:56:00 +0100 Subject: [PATCH 1/2] Use std::move to transfer ownership of DataBufs. --- include/exiv2/image.hpp | 5 +++-- include/exiv2/preview.hpp | 2 +- include/exiv2/types.hpp | 38 +++++++++----------------------------- src/actions.cpp | 14 +++++++------- src/actions.hpp | 2 +- src/bmffimage.cpp | 4 ++-- src/crwimage_int.cpp | 24 ++++++++++++------------ src/crwimage_int.hpp | 4 ++-- src/image.cpp | 8 ++++---- src/jp2image.cpp | 2 +- src/jpgimage.cpp | 2 +- src/preview.cpp | 6 +++--- src/tiffvisitor_int.cpp | 4 ++-- src/types.cpp | 32 +++++++++++++++----------------- src/webpimage.cpp | 2 +- 15 files changed, 64 insertions(+), 85 deletions(-) diff --git a/include/exiv2/image.hpp b/include/exiv2/image.hpp index 4b85a11d..6d54ce62 100644 --- a/include/exiv2/image.hpp +++ b/include/exiv2/image.hpp @@ -222,7 +222,7 @@ namespace Exiv2 { @param iccProfile DataBuf containing profile (binary) @param bTestValid - tests that iccProfile contains credible data */ - virtual void setIccProfile(DataBuf& iccProfile,bool bTestValid=true); + virtual void setIccProfile(DataBuf&& iccProfile,bool bTestValid=true); /*! @brief Erase iccProfile. the profile is not removed from the actual image until the writeMetadata() method is called. @@ -240,7 +240,8 @@ namespace Exiv2 { /*! @brief return iccProfile */ - virtual DataBuf* iccProfile() { return &iccProfile_; } + virtual const DataBuf& iccProfile() const { return iccProfile_; } + /*! @brief Copy all existing metadata from source Image. The data is copied into internal buffers and is not written to the image diff --git a/include/exiv2/preview.hpp b/include/exiv2/preview.hpp index 98b92848..6f794466 100644 --- a/include/exiv2/preview.hpp +++ b/include/exiv2/preview.hpp @@ -145,7 +145,7 @@ namespace Exiv2 { private: //! Private constructor - PreviewImage(PreviewProperties properties, DataBuf data); + PreviewImage(PreviewProperties properties, DataBuf&& data); PreviewProperties properties_; //!< Preview image properties DataBuf preview_; //!< Preview image data diff --git a/include/exiv2/types.hpp b/include/exiv2/types.hpp index cc84392a..026c7cc3 100644 --- a/include/exiv2/types.hpp +++ b/include/exiv2/types.hpp @@ -153,18 +153,6 @@ namespace Exiv2 { }; - /*! - @brief Auxiliary type to enable copies and assignments, similar to - std::unique_ptr_ref. See http://www.josuttis.com/libbook/auto_ptr.html - for a discussion. - */ - struct EXIV2API DataBufRef { - //! Constructor - explicit DataBufRef(std::pair rhs) : p(rhs) {} - //! Pointer to a byte array and its size - std::pair p; - }; - /*! @brief Utility class containing a character array. All it does is to take care of memory allocation and deletion. Its primary use is meant to @@ -181,11 +169,15 @@ namespace Exiv2 { //! Constructor, copies an existing buffer DataBuf(const byte* pData, long size); /*! - @brief Copy constructor. Transfers the buffer to the newly created + @brief Copy constructor. Copies an existing DataBuf. + */ + DataBuf(const DataBuf& rhs); + /*! + @brief Move constructor. Transfers the buffer to the newly created object similar to std::unique_ptr, i.e., the original object is modified. */ - DataBuf(DataBuf& rhs); + DataBuf(DataBuf&& rhs); //! Destructor, deletes the allocated buffer ~DataBuf(); //@} @@ -197,7 +189,7 @@ namespace Exiv2 { buffer at the original object similar to std::unique_ptr, i.e., the original object is modified. */ - DataBuf& operator=(DataBuf& rhs); + DataBuf& operator=(DataBuf&& rhs); /*! @brief Allocate a data buffer of at least the given size. Note that if the requested \em size is less than the current buffer size, no @@ -208,6 +200,7 @@ namespace Exiv2 { @brief Resize the buffer. Existing data is preserved (like std::realloc()). */ void resize(long size); + /*! @brief Release ownership of the buffer to the caller. Returns the buffer as a data pointer and size pair, resets the internal @@ -216,25 +209,12 @@ namespace Exiv2 { EXV_WARN_UNUSED_RESULT std::pair release(); //! Reset value - void reset(std::pair = {nullptr, long(0)}); + void reset(); //@} //! Fill the buffer with zeros. void clear(); - /*! - @name Conversions - - Special conversions with auxiliary type to enable copies - and assignments, similar to those used for std::unique_ptr. - See http://www.josuttis.com/libbook/auto_ptr.html for a discussion. - */ - //@{ - DataBuf(const DataBufRef& rhs); - DataBuf& operator=(DataBufRef rhs); - operator DataBufRef(); - //@} - long size() const { return size_; } uint8_t read_uint8(size_t offset) const; diff --git a/src/actions.cpp b/src/actions.cpp index 5366c1a9..38a7b937 100644 --- a/src/actions.cpp +++ b/src/actions.cpp @@ -1025,15 +1025,15 @@ namespace Action { } else { if ( bStdout ) { // -eC- - std::cout.write(image->iccProfile()->c_str(), - image->iccProfile()->size()); + std::cout.write(image->iccProfile().c_str(), + image->iccProfile().size()); } else { if (Params::instance().verbose_) { std::cout << _("Writing iccProfile: ") << target << std::endl; } Exiv2::FileIo iccFile(target); iccFile.open("wb") ; - iccFile.write(image->iccProfile()->c_data(),image->iccProfile()->size()); + iccFile.write(image->iccProfile().c_data(),image->iccProfile().size()); iccFile.close(); } } @@ -1180,7 +1180,7 @@ namespace Action { if ( iccPath == "-" ) { Exiv2::DataBuf iccProfile ; Params::instance().getStdin(iccProfile); - rc = insertIccProfile(path,iccProfile); + rc = insertIccProfile(path,std::move(iccProfile)); } else { if (!Exiv2::fileExists(iccProfilePath, true)) { std::cerr << iccProfilePath @@ -1188,13 +1188,13 @@ namespace Action { rc = -1; } else { Exiv2::DataBuf iccProfile = Exiv2::readFile(iccPath); - rc = insertIccProfile(path,iccProfile); + rc = insertIccProfile(path,std::move(iccProfile)); } } return rc; } // Insert::insertIccProfile - int Insert::insertIccProfile(const std::string& path, Exiv2::DataBuf& iccProfileBlob) + int Insert::insertIccProfile(const std::string& path, Exiv2::DataBuf&& iccProfileBlob) { int rc = 0; // test path exists @@ -1211,7 +1211,7 @@ namespace Action { // clear existing profile, assign the blob and rewrite image image->clearIccProfile(); if ( iccProfileBlob.size() ) { - image->setIccProfile(iccProfileBlob); + image->setIccProfile(std::move(iccProfileBlob)); } image->writeMetadata(); } diff --git a/src/actions.hpp b/src/actions.hpp index b88ae530..09f4eeaa 100644 --- a/src/actions.hpp +++ b/src/actions.hpp @@ -360,7 +360,7 @@ namespace Action { /*! @brief Insert an ICC profile from binary DataBuf into file \em path. */ - static int insertIccProfile(const std::string& path, Exiv2::DataBuf& iccProfileBlob); + static int insertIccProfile(const std::string& path, Exiv2::DataBuf&& iccProfileBlob); private: Insert* clone_() const override; diff --git a/src/bmffimage.cpp b/src/bmffimage.cpp index b84d64ec..9e70c8db 100644 --- a/src/bmffimage.cpp +++ b/src/bmffimage.cpp @@ -411,12 +411,12 @@ namespace Exiv2 skip+=4; if ( colour_type == "rICC" || colour_type == "prof" ) { DataBuf profile(data.c_data(skip),data.size()-skip); - setIccProfile(profile); + setIccProfile(std::move(profile)); } else if ( meth == 2 && prec == 0 && approx == 0 ) { // JP2000 files have a 3 byte head // 2 0 0 icc...... skip -= 1 ; DataBuf profile(data.c_data(skip),data.size()-skip); - setIccProfile(profile); + setIccProfile(std::move(profile)); } } } break; diff --git a/src/crwimage_int.cpp b/src/crwimage_int.cpp index b0bc17b4..1a4fe7e9 100644 --- a/src/crwimage_int.cpp +++ b/src/crwimage_int.cpp @@ -553,9 +553,9 @@ namespace Exiv2 { } } // CiffDirectory::doPrint - void CiffComponent::setValue(DataBuf buf) + void CiffComponent::setValue(DataBuf&& buf) { - storage_ = buf; + storage_ = std::move(buf); pData_ = storage_.c_data(); size_ = storage_.size(); if (size_ > 8 && dataLocation() == directoryData) { @@ -626,7 +626,7 @@ namespace Exiv2 { return nullptr; } // CiffDirectory::doFindComponent - void CiffHeader::add(uint16_t crwTagId, uint16_t crwDir, DataBuf buf) + void CiffHeader::add(uint16_t crwTagId, uint16_t crwDir, DataBuf&& buf) { CrwDirs crwDirs; CrwMap::loadStack(crwDirs, crwDir); @@ -639,7 +639,7 @@ namespace Exiv2 { } CiffComponent* child = pRootDir_->add(crwDirs, crwTagId); if (child) { - child->setValue(buf); + child->setValue(std::move(buf)); } } // CiffHeader::add @@ -1015,7 +1015,7 @@ namespace Exiv2 { if (ed != image.exifData().end()) { DataBuf buf(ed->size()); ed->copy(buf.data(), pHead->byteOrder()); - pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, buf); + pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, std::move(buf)); } else { pHead->remove(pCrwMapping->crwTagId_, pCrwMapping->crwDir_); @@ -1039,14 +1039,14 @@ namespace Exiv2 { DataBuf buf(size); buf.clear(); buf.copyBytes(0, comment.data(), comment.size()); - pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, buf); + pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, std::move(buf)); } else { if (cc) { // Just delete the value, do not remove the tag DataBuf buf(cc->size()); buf.clear(); - cc->setValue(buf); + cc->setValue(std::move(buf)); } } } // CrwMap::encode0x0805 @@ -1079,7 +1079,7 @@ namespace Exiv2 { pos += ed2->size(); } assert(pos == size); - pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, buf); + pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, std::move(buf)); } else { pHead->remove(pCrwMapping->crwTagId_, pCrwMapping->crwDir_); @@ -1109,7 +1109,7 @@ namespace Exiv2 { if (buf.size() > 0) { // Write the number of shorts to the beginning of buf buf.write_uint16(0, static_cast(buf.size()), pHead->byteOrder()); - pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, buf); + pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, std::move(buf)); } else { pHead->remove(pCrwMapping->crwTagId_, pCrwMapping->crwDir_); @@ -1137,7 +1137,7 @@ namespace Exiv2 { DataBuf buf(12); buf.clear(); buf.write_uint32(0, static_cast(t), pHead->byteOrder()); - pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, buf); + pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, std::move(buf)); } else { pHead->remove(pCrwMapping->crwTagId_, pCrwMapping->crwDir_); @@ -1183,7 +1183,7 @@ namespace Exiv2 { d = RotationMap::degrees(static_cast(edO->toLong())); } buf.write_uint32(12, d, pHead->byteOrder()); - pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, buf); + pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, std::move(buf)); } else { pHead->remove(pCrwMapping->crwTagId_, pCrwMapping->crwDir_); @@ -1200,7 +1200,7 @@ namespace Exiv2 { ExifThumbC exifThumb(image.exifData()); DataBuf buf = exifThumb.copy(); if (buf.size() != 0) { - pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, buf); + pHead->add(pCrwMapping->crwTagId_, pCrwMapping->crwDir_, std::move(buf)); } else { pHead->remove(pCrwMapping->crwTagId_, pCrwMapping->crwDir_); diff --git a/src/crwimage_int.hpp b/src/crwimage_int.hpp index a5528933..c3ef56b9 100644 --- a/src/crwimage_int.hpp +++ b/src/crwimage_int.hpp @@ -164,7 +164,7 @@ namespace Exiv2 { //! Set the directory tag for this component. void setDir(uint16_t dir) { dir_ = dir; } //! Set the data value of the entry. - void setValue(DataBuf buf); + void setValue(DataBuf&& buf); //@} //! Return the type id for a tag @@ -446,7 +446,7 @@ namespace Exiv2 { @param crwDir Parent directory of the tag. @param buf Value to be set. */ - void add(uint16_t crwTagId, uint16_t crwDir, DataBuf buf); + void add(uint16_t crwTagId, uint16_t crwDir, DataBuf&& buf); /*! @brief Remove entry \em crwTagId in directory \em crwDir from the parse tree. If it's the last entry in the directory, the directory is diff --git a/src/image.cpp b/src/image.cpp index 7135177f..db6f7a9d 100644 --- a/src/image.cpp +++ b/src/image.cpp @@ -590,8 +590,8 @@ namespace Exiv2 { if (checkMode(mdIptc) & amWrite) { setIptcData(image.iptcData()); } - if (checkMode(mdIccProfile) & amWrite && iccProfile()) { - setIccProfile(*iccProfile()); + if (checkMode(mdIccProfile) & amWrite) { + setIccProfile(DataBuf(image.iccProfile())); } if (checkMode(mdXmp) & amWrite) { setXmpPacket(image.xmpPacket()); @@ -668,7 +668,7 @@ namespace Exiv2 { comment_ = comment; } - void Image::setIccProfile(Exiv2::DataBuf& iccProfile,bool bTestValid) + void Image::setIccProfile(Exiv2::DataBuf&& iccProfile,bool bTestValid) { if ( bTestValid ) { if (iccProfile.size() < static_cast(sizeof(long))) { @@ -679,7 +679,7 @@ namespace Exiv2 { throw Error(kerInvalidIccProfile); } } - iccProfile_ = iccProfile; + iccProfile_ = std::move(iccProfile); } void Image::clearIccProfile() diff --git a/src/jp2image.cpp b/src/jp2image.cpp index 5fa12f92..d03aaebc 100644 --- a/src/jp2image.cpp +++ b/src/jp2image.cpp @@ -303,7 +303,7 @@ static void boxes_check(size_t b,size_t m) } std::cout << "Exiv2::Jp2Image::readMetadata: wrote iccProfile " << icc.size() << " bytes to " << iccPath << std::endl ; #endif - setIccProfile(icc); + setIccProfile(std::move(icc)); } if( subBox.type == kJp2BoxTypeImageHeader) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index db6b9dfa..f07b48fb 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -496,7 +496,7 @@ namespace Exiv2 { profile.copyBytes(0, iccProfile_.c_data(), iccProfile_.size()); } profile.copyBytes(iccProfile_.size(), buf.c_data(2+14), icc_size); - setIccProfile(profile,chunk==chunks); + setIccProfile(std::move(profile),chunk==chunks); } else if ( pixelHeight_ == 0 && inRange2(marker,sof0_,sof3_,sof5_,sof15_) ) { // We hit a SOFn (start-of-frame) marker diff --git a/src/preview.cpp b/src/preview.cpp index 39a3023b..19c09d63 100644 --- a/src/preview.cpp +++ b/src/preview.cpp @@ -1037,8 +1037,8 @@ namespace { // ***************************************************************************** // class member definitions namespace Exiv2 { - PreviewImage::PreviewImage(PreviewProperties properties, DataBuf data) - : properties_(std::move(properties)), preview_(data) + PreviewImage::PreviewImage(PreviewProperties properties, DataBuf&& data) + : properties_(std::move(properties)), preview_(std::move(data)) {} PreviewImage::PreviewImage(const PreviewImage &rhs) @@ -1148,6 +1148,6 @@ namespace Exiv2 { buf = loader->getData(); } - return PreviewImage(properties, buf); + return PreviewImage(properties, std::move(buf)); } } // namespace Exiv2 diff --git a/src/tiffvisitor_int.cpp b/src/tiffvisitor_int.cpp index ab568748..7e0febc3 100644 --- a/src/tiffvisitor_int.cpp +++ b/src/tiffvisitor_int.cpp @@ -627,7 +627,7 @@ namespace Exiv2 { buf.copyBytes(0, rawIptc.c_data(), rawIptc.size()); } else { - buf = rawIptc; // Note: This resets rawIptc + buf = std::move(rawIptc); // Note: This resets rawIptc } value->read(buf.data(), buf.size(), byteOrder_); Exifdatum iptcDatum(iptcNaaKey, value.get()); @@ -1654,7 +1654,7 @@ namespace Exiv2 { const byte* pData = object->pData(); int32_t size = object->TiffEntryBase::doSize(); DataBuf buf = cryptFct(object->tag(), pData, size, pRoot_); - if (buf.size() > 0) object->setData(buf); + if (buf.size() > 0) object->setData(std::move(buf)); } const ArrayDef* defs = object->def(); diff --git a/src/types.cpp b/src/types.cpp index 026cc4c3..f7aba2cb 100644 --- a/src/types.cpp +++ b/src/types.cpp @@ -121,11 +121,11 @@ namespace Exiv2 { return tit->size_; } - DataBuf::DataBuf(DataBuf& rhs) + DataBuf::DataBuf(DataBuf&& rhs) : pData_(rhs.pData_), size_(rhs.size_) { - std::pair ret = rhs.release(); - UNUSED(ret); + rhs.pData_ = nullptr; + rhs.size_ = 0; } DataBuf::~DataBuf() @@ -146,10 +146,16 @@ namespace Exiv2 { } } - DataBuf& DataBuf::operator=(DataBuf& rhs) + DataBuf::DataBuf(const DataBuf& rhs) + : DataBuf(rhs.pData_, rhs.size_) + {} + + DataBuf& DataBuf::operator=(DataBuf&& rhs) { if (this == &rhs) return *this; - reset(rhs.release()); + reset(); + std::swap(pData_, rhs.pData_); + std::swap(size_, rhs.size_); return *this; } @@ -185,25 +191,17 @@ namespace Exiv2 { return p; } - void DataBuf::reset(std::pair p) + void DataBuf::reset() { - if (pData_ != p.first) { - delete[] pData_; - pData_ = p.first; - } - size_ = p.second; + delete[] pData_; + pData_ = nullptr; + size_ = 0; } void DataBuf::clear() { memset(pData_, 0, size_); } - DataBuf::DataBuf(const DataBufRef &rhs) : pData_(rhs.p.first), size_(rhs.p.second) {} - - DataBuf &DataBuf::operator=(DataBufRef rhs) { reset(rhs.p); return *this; } - - Exiv2::DataBuf::operator DataBufRef() { return DataBufRef(release()); } - uint8_t Exiv2::DataBuf::read_uint8(size_t offset) const { if (offset >= static_cast(size_)) { throw std::overflow_error("Overflow in Exiv2::DataBuf::read_uint8"); diff --git a/src/webpimage.cpp b/src/webpimage.cpp index 9ae77786..f8a1887b 100644 --- a/src/webpimage.cpp +++ b/src/webpimage.cpp @@ -635,7 +635,7 @@ namespace Exiv2 { pixelHeight_ = Exiv2::getULong(size_buf, littleEndian) + 1; } else if (equalsWebPTag(chunkId, WEBP_CHUNK_HEADER_ICCP)) { readOrThrow(*io_, payload.data(), payload.size(), Exiv2::kerCorruptedMetadata); - this->setIccProfile(payload); + this->setIccProfile(std::move(payload)); } else if (equalsWebPTag(chunkId, WEBP_CHUNK_HEADER_EXIF)) { readOrThrow(*io_, payload.data(), payload.size(), Exiv2::kerCorruptedMetadata); From 94ea0c6d90f292dc3c3a84ed402a20083d729b1f Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Mon, 11 Oct 2021 21:36:37 +0100 Subject: [PATCH 2/2] Delete the copy assignment operator to avoid accidental copying. --- include/exiv2/types.hpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/exiv2/types.hpp b/include/exiv2/types.hpp index 026c7cc3..32725694 100644 --- a/include/exiv2/types.hpp +++ b/include/exiv2/types.hpp @@ -190,6 +190,10 @@ namespace Exiv2 { the original object is modified. */ DataBuf& operator=(DataBuf&& rhs); + + // No copy assignment. + DataBuf& operator=(const DataBuf&) = delete; + /*! @brief Allocate a data buffer of at least the given size. Note that if the requested \em size is less than the current buffer size, no