diff --git a/include/exiv2/types.hpp b/include/exiv2/types.hpp index 32725694..c6b1afe2 100644 --- a/include/exiv2/types.hpp +++ b/include/exiv2/types.hpp @@ -205,13 +205,6 @@ namespace Exiv2 { */ 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 - buffer. - */ - EXV_WARN_UNUSED_RESULT std::pair release(); - //! Reset value void reset(); //@} diff --git a/src/tiffcomposite_int.cpp b/src/tiffcomposite_int.cpp index 943f33bb..0d27b8a6 100644 --- a/src/tiffcomposite_int.cpp +++ b/src/tiffcomposite_int.cpp @@ -97,7 +97,6 @@ namespace Exiv2 { offset_(0), size_(0), pData_(nullptr), - isMalloced_(false), idx_(0), pValue_(nullptr) { @@ -188,9 +187,6 @@ namespace Exiv2 { TiffEntryBase::~TiffEntryBase() { - if (isMalloced_) { - delete[] pData_; - } delete pValue_; } @@ -218,14 +214,10 @@ namespace Exiv2 { offset_(rhs.offset_), size_(rhs.size_), pData_(rhs.pData_), - isMalloced_(rhs.isMalloced_), idx_(rhs.idx_), - pValue_(rhs.pValue_ ? rhs.pValue_->clone().release() : nullptr) + pValue_(rhs.pValue_ ? rhs.pValue_->clone().release() : nullptr), + storage_(rhs.storage_) { - if (rhs.isMalloced_) { - pData_ = new byte[rhs.size_]; - memcpy(pData_, rhs.pData_, rhs.size_); - } } TiffDirectory::TiffDirectory(const TiffDirectory& rhs) : TiffComponent(rhs), hasNext_(rhs.hasNext_), pNext_(nullptr) @@ -320,20 +312,19 @@ namespace Exiv2 { return idx_; } - void TiffEntryBase::setData(DataBuf buf) + void TiffEntryBase::setData(const std::shared_ptr& buf) { - std::pair p = buf.release(); - setData(p.first, p.second); - isMalloced_ = true; + storage_ = buf; + pData_ = buf->data(); + size_ = buf->size(); } - void TiffEntryBase::setData(byte* pData, int32_t size) + void TiffEntryBase::setData(byte* pData, int32_t size, + const std::shared_ptr& storage) { - if (isMalloced_) { - delete[] pData_; - } pData_ = pData; size_ = size; + storage_ = storage; if (pData_ == nullptr) size_ = 0; } @@ -344,7 +335,7 @@ namespace Exiv2 { return; uint32_t newSize = value->size(); if (newSize > size_) { - setData(DataBuf(newSize)); + setData(std::make_shared(newSize)); } if (pData_ != nullptr) { memset(pData_, 0x0, size_); @@ -590,7 +581,7 @@ namespace Exiv2 { // the TIFF structure table (TiffCreator::tiffTreeStruct_) assert(tp); tp->setStart(pData() + idx); - tp->setData(const_cast(pData() + idx), sz); + tp->setData(const_cast(pData() + idx), sz, storage()); tp->setElDef(def); tp->setElByteOrder(cfg()->byteOrder_); addChild(std::move(tc)); diff --git a/src/tiffcomposite_int.hpp b/src/tiffcomposite_int.hpp index 1db4f520..3b48ae9d 100644 --- a/src/tiffcomposite_int.hpp +++ b/src/tiffcomposite_int.hpp @@ -436,10 +436,28 @@ namespace Exiv2 { void encode(TiffEncoder& encoder, const Exifdatum* datum); //! Set the offset void setOffset(int32_t offset) { offset_ = offset; } - //! Set pointer and size of the entry's data (not taking ownership of the data). - void setData(byte* pData, int32_t size); - //! Set the entry's data buffer, taking ownership of the data buffer passed in. - void setData(DataBuf buf); + /*! + @brief Set pointer and size of the entry's data (not taking ownership of the data). + + @param storage Usually, pData is a pointer into a copy of the image file, which + means that it points to memory which is guaranteed to live longer + than this class. However, sometimes pData is pointer into a + DataBuf that was allocated by another node in the component tree. + If so, we need to make sure that the DataBuf doesn't get freed too + early. We use a std::shared_ptr to hold a reference to the DataBuf + to ensure that it will be kept alive. The storage parameter is + assigned to the storage_ field. In the more common scenario where + pData points to a copy of the image, rather than a DataBuf, then + you should pass std::shared_ptr(), which is essentially + a nullptr. + */ + void setData(byte* pData, int32_t size, const std::shared_ptr& storage); + /*! + @brief Set the entry's data buffer. A shared_ptr is used to manage the DataBuf + because TiffEntryBase has a clone method so it is possible (in theory) for + the DataBuf to have multiple owners. + */ + void setData(const std::shared_ptr& buf); /*! @brief Update the value. Takes ownership of the pointer passed in. @@ -529,6 +547,9 @@ namespace Exiv2 { TiffType tiffType, ByteOrder byteOrder); + //! Used (internally) to create another reference to the DataBuf reference by storage_. + const std::shared_ptr& storage() { return storage_; } + private: //! @name NOT implemented //@{ @@ -545,11 +566,22 @@ namespace Exiv2 { minimum size. */ uint32_t size_; + + // Notes on the ownership model of pData_: pData_ is a always a + // pointer to a buffer owned by somebody else. Usually it is a + // pointer into a copy of the image file, but if + // TiffEntryBase::setData is used then it is a pointer into the + // storage_ DataBuf below. byte* pData_; //!< Pointer to the data area - bool isMalloced_; //!< True if this entry owns the value data + int idx_; //!< Unique id of the entry in the image Value* pValue_; //!< Converted data value + // This DataBuf is only used when TiffEntryBase::setData is called. + // Otherwise, it remains empty. It is wrapped in a shared_ptr because + // TiffEntryBase has a clone method, which could lead to the DataBuf + // having multiple owners. + std::shared_ptr storage_; }; // class TiffEntryBase /*! diff --git a/src/tiffvisitor_int.cpp b/src/tiffvisitor_int.cpp index 1540cda3..3de05e91 100644 --- a/src/tiffvisitor_int.cpp +++ b/src/tiffvisitor_int.cpp @@ -1609,7 +1609,7 @@ namespace Exiv2 { v->read(pData, size, byteOrder()); object->setValue(std::move(v)); - object->setData(pData, size); + object->setData(pData, size, std::shared_ptr()); object->setOffset(offset); object->setIdx(nextIdx(object->group())); @@ -1653,8 +1653,10 @@ namespace Exiv2 { if (cryptFct != nullptr) { 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(std::move(buf)); + std::shared_ptr buf = std::make_shared( + cryptFct(object->tag(), pData, size, pRoot_) + ); + if (buf->size() > 0) object->setData(buf); } const ArrayDef* defs = object->def(); diff --git a/src/types.cpp b/src/types.cpp index f7aba2cb..dc7981a9 100644 --- a/src/types.cpp +++ b/src/types.cpp @@ -183,14 +183,6 @@ namespace Exiv2 { size_ = size; } - EXV_WARN_UNUSED_RESULT std::pair DataBuf::release() - { - std::pair p = {pData_, size_}; - pData_ = nullptr; - size_ = 0; - return p; - } - void DataBuf::reset() { delete[] pData_;