Merge pull request #2040 from kevinbackhouse/TiffEntryBaseStorage

Use std::shared_ptr for storage in TiffEntryBase

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<byte*, long> release();
-
         //! Reset value
         void reset();
         //@}

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<DataBuf>& buf)
     {
-        std::pair<byte*, long> p = buf.release();
+        storage_ = buf;
-        setData(p.first, p.second);
+        pData_ = buf->data();
-        isMalloced_ = true;
+        size_ = buf->size();
     }
 
-    void TiffEntryBase::setData(byte* pData, int32_t size)
+    void TiffEntryBase::setData(byte* pData, int32_t size,
+                                const std::shared_ptr<DataBuf>& 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<DataBuf>(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<byte*>(pData() + idx), sz);
+        tp->setData(const_cast<byte*>(pData() + idx), sz, storage());
         tp->setElDef(def);
         tp->setElByteOrder(cfg()->byteOrder_);
         addChild(std::move(tc));

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);
+          @brief Set pointer and size of the entry's data (not taking ownership of the data).
-        //! Set the entry's data buffer, taking ownership of the data buffer passed in.
+
-        void setData(DataBuf buf);
+          @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<DataBuf>(), which is essentially
+                         a nullptr.
+         */
+        void setData(byte* pData, int32_t size, const std::shared_ptr<DataBuf>& 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<DataBuf>& 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<DataBuf>& 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<DataBuf> storage_;
     }; // class TiffEntryBase
 
     /*!

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<DataBuf>());
         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_);
+            std::shared_ptr<DataBuf> buf = std::make_shared<DataBuf>(
-            if (buf.size() > 0) object->setData(std::move(buf));
+              cryptFct(object->tag(), pData, size, pRoot_)
+            );
+            if (buf->size() > 0) object->setData(buf);
         }
 
         const ArrayDef* defs = object->def();

src/types.cpp

@@ -183,14 +183,6 @@ namespace Exiv2 {
         size_ = size;
     }
 
-    EXV_WARN_UNUSED_RESULT std::pair<byte*, long> DataBuf::release()
-    {
-        std::pair<byte*, long> p = {pData_, size_};
-        pData_ = nullptr;
-        size_ = 0;
-        return p;
-    }
-
     void DataBuf::reset()
     {
         delete[] pData_;