From 9f3240c91cf4913baaf874615f58d357299f09e5 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Fri, 24 Dec 2021 12:32:48 +0000 Subject: [PATCH 1/4] Delete TiffEntryBase::isMalloced_ field and use a std::shared_ptr instead. --- src/tiffcomposite_int.cpp | 25 +++++++------------------ src/tiffcomposite_int.hpp | 21 ++++++++++++++++++--- src/tiffvisitor_int.cpp | 6 ++++-- 3 files changed, 29 insertions(+), 23 deletions(-) diff --git a/src/tiffcomposite_int.cpp b/src/tiffcomposite_int.cpp index 943f33bb..66434733 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,18 +312,15 @@ 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) { - if (isMalloced_) { - delete[] pData_; - } pData_ = pData; size_ = size; if (pData_ == nullptr) @@ -344,7 +333,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_); diff --git a/src/tiffcomposite_int.hpp b/src/tiffcomposite_int.hpp index 1db4f520..74b7a6e5 100644 --- a/src/tiffcomposite_int.hpp +++ b/src/tiffcomposite_int.hpp @@ -438,8 +438,12 @@ namespace Exiv2 { 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 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. @@ -545,11 +549,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..a4a833b1 100644 --- a/src/tiffvisitor_int.cpp +++ b/src/tiffvisitor_int.cpp @@ -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(); From 29f7d0ccf0ae996b46a13d1839d262a6ec671eb6 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Fri, 24 Dec 2021 12:35:46 +0000 Subject: [PATCH 2/4] Remove DataBuf::release() which is no longer used. --- include/exiv2/types.hpp | 7 ------- src/types.cpp | 8 -------- 2 files changed, 15 deletions(-) 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/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_; From b6a4fd1c672d811b206ab254db32a3f395d39270 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Fri, 24 Dec 2021 12:54:35 +0000 Subject: [PATCH 3/4] Add storage parameter to setData. --- src/tiffcomposite_int.cpp | 6 ++++-- src/tiffcomposite_int.hpp | 4 +++- src/tiffvisitor_int.cpp | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/tiffcomposite_int.cpp b/src/tiffcomposite_int.cpp index 66434733..0d27b8a6 100644 --- a/src/tiffcomposite_int.cpp +++ b/src/tiffcomposite_int.cpp @@ -319,10 +319,12 @@ namespace Exiv2 { size_ = buf->size(); } - void TiffEntryBase::setData(byte* pData, int32_t size) + void TiffEntryBase::setData(byte* pData, int32_t size, + const std::shared_ptr& storage) { pData_ = pData; size_ = size; + storage_ = storage; if (pData_ == nullptr) size_ = 0; } @@ -579,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 74b7a6e5..d5a5ebe9 100644 --- a/src/tiffcomposite_int.hpp +++ b/src/tiffcomposite_int.hpp @@ -437,7 +437,7 @@ namespace Exiv2 { //! 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); + 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 @@ -533,6 +533,8 @@ namespace Exiv2 { TiffType tiffType, ByteOrder byteOrder); + const std::shared_ptr& storage() { return storage_; } + private: //! @name NOT implemented //@{ diff --git a/src/tiffvisitor_int.cpp b/src/tiffvisitor_int.cpp index a4a833b1..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())); From 9268279b47e8c2caab81102a75ddee01bcdbdac4 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Fri, 24 Dec 2021 15:04:14 +0000 Subject: [PATCH 4/4] Add comments. --- src/tiffcomposite_int.hpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/tiffcomposite_int.hpp b/src/tiffcomposite_int.hpp index d5a5ebe9..3b48ae9d 100644 --- a/src/tiffcomposite_int.hpp +++ b/src/tiffcomposite_int.hpp @@ -436,7 +436,21 @@ 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). + /*! + @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 @@ -533,6 +547,7 @@ 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: