Invalidate metadata pointers when data is deleted

(Requires swig >= 4.4) This removes another potential source of
segfaults.

Author: jim-easterbrook

src/interface/exif.i

@@ -26,7 +26,6 @@
 %include "shared/preamble.i"
 %include "shared/buffers.i"
 %include "shared/containers.i"
-%include "shared/data_iterator.i"
 %include "shared/metadatum_wrappers.i"
 %include "shared/keep_reference.i"
 %include "shared/windows.i"
@@ -55,8 +54,6 @@ INPUT_BUFFER_RO(const Exiv2::byte* buf, BUFLEN_T size)
 
 EXTEND_METADATUM(Exifdatum)
 
-DATA_ITERATOR(ExifData, Exifdatum)
-
 METADATUM_WRAPPERS(ExifData, Exifdatum)
 
 // Get the current (or default if not set) type id of a datum

src/interface/image.i

@@ -65,6 +65,9 @@ RELEASE_BUFFER(void writeMetadata)
 // Convert path encoding on Windows
 WINDOWS_PATH(const std::string& path)
 
+// Declare metadatum wrapper classes
+DECLARE_METADATUM_WRAPPERS(ExifData, Exifdatum)
+
 // Simplify handling of default parameters
 %typemap(default) bool useCurl {$1 = true;}
 %ignore Exiv2::ImageFactory::createIo(std::string const &);

src/interface/iptc.i

@@ -25,7 +25,6 @@
 
 %include "shared/preamble.i"
 %include "shared/containers.i"
-%include "shared/data_iterator.i"
 %include "shared/metadatum_wrappers.i"
 
 %include "stdint.i"
@@ -41,8 +40,6 @@ EXCEPTION()
 
 EXTEND_METADATUM(Iptcdatum)
 
-DATA_ITERATOR(IptcData, Iptcdatum)
-
 METADATUM_WRAPPERS(IptcData, Iptcdatum)
 
 // Get the current (or default if not set) type id of a datum

src/interface/preview.i

@@ -59,6 +59,9 @@ EXV_ENABLE_FILESYSTEM_FUNCTION(Exiv2::PreviewImage::writeFile)
 WINDOWS_PATH(const std::string& path)
 WINDOWS_PATH_OUT(extension)
 
+// Declare metadatum wrapper classes
+DECLARE_METADATUM_WRAPPERS(ExifData, Exifdatum)
+
 // Convert getPreviewProperties result to a Python tuple
 %template() std::vector<Exiv2::PreviewProperties>;
 

src/interface/shared/containers.i

@@ -71,7 +71,7 @@
             return NULL;
         }
 #if SWIG_VERSION >= 0x040400
-        invalidate_iterators(py_self, pos);
+        invalidate_pointers(py_self, pos);
 #endif
         $self->erase(pos);
         return SWIG_Py_Void();

src/interface/shared/metadatum_wrappers.i

@@ -20,11 +20,25 @@
 
 
 %include "shared/keep_reference.i"
+%include "shared/pointer_store.i"
+
+
+// Macro to declare metadatum iterators and pointer wrappers
+%define DECLARE_METADATUM_WRAPPERS(container_type, datum_type)
+%{
+class datum_type##_pointer;
+class container_type##_iterator;
+class datum_type##_reference;
+%}
+%enddef // DECLARE_METADATUM_WRAPPERS
 
 
 // Macro to wrap metadatum iterators and pointers
 %define METADATUM_WRAPPERS(container_type, datum_type)
 
+// Invalidate pointers when data is deleted
+POINTER_STORE(container_type, datum_type)
+
 // Base class of pointer wrappers
 %feature("python:slot", "tp_str", functype="reprfunc")
     datum_type##_pointer::__str__;
@@ -50,10 +64,10 @@ public:
     }
     std::string __str__() {
         if (invalidated)
-            return "invalid " + name;
+            return name + "<deleted data>";
         Exiv2::datum_type* ptr = **this;
         if (!ptr)
-            return name + "<end>";
+            return name + "<data end>";
         return name + "<" + ptr->key() + ": " + ptr->print() + ">";
     }
     // Provide size() C++ method for buffer size check
@@ -67,6 +81,12 @@ public:
     }
     // Invalidate iterator unilaterally
     void _invalidate() { invalidated = true; }
+    // Invalidate iterator if what it points to has been deleted
+    bool _invalidate(Exiv2::datum_type& deleted) {
+        if (&deleted == **this)
+            invalidated = true;
+        return invalidated;
+    }
     // Dereference operator gives access to all datum methods
     Exiv2::datum_type* operator->() const {
         Exiv2::datum_type* ptr = **this;
@@ -87,7 +107,6 @@ public:
 %ignore container_type##_iterator::##container_type##_iterator;
 %ignore container_type##_iterator::operator*;
 %ignore container_type##_iterator::_ptr;
-%ignore container_type##_iterator::_invalidate;
 %feature("docstring") container_type##_iterator "
 Python wrapper for an :class:`" #container_type "` iterator. It has most of
 the methods of :class:`" #datum_type "` allowing easy access to the
@@ -131,13 +150,6 @@ public:
             return NULL;
         return &(*ptr);
     }
-    using datum_type##_pointer::_invalidate;
-    // Invalidate iterator if what it points to has been deleted
-    bool _invalidate(Exiv2::container_type::iterator deleted) {
-        if (deleted == ptr)
-            invalidated = true;
-        return invalidated;
-    }
     // Access to ptr, for use in other methods
     Exiv2::container_type::iterator _ptr() const {
         if (invalidated)
@@ -172,7 +184,7 @@ public:
         $descriptor(container_type##_iterator*), SWIG_POINTER_OWN);
 #if SWIG_VERSION >= 0x040400
     // Keep weak reference to the Python iterator
-    if (store_iterator(self, $result)) {
+    if (store_pointer(self, $result)) {
         SWIG_fail;
     }
 #endif // SWIG_VERSION
@@ -181,7 +193,6 @@ public:
 // Metadata reference wrapper
 %ignore datum_type##_reference::##datum_type##_reference;
 %ignore datum_type##_reference::operator*;
-%ignore datum_type##_reference::_invalidate;
 %feature("docstring") datum_type##_reference "
 Python wrapper for an :class:`" #datum_type "` reference. It has most of
 the methods of :class:`" #datum_type "` allowing easy access to the
@@ -201,13 +212,6 @@ public:
             throw std::runtime_error("datum_type reference is invalid");
         return ptr;
     }
-    using datum_type##_pointer::_invalidate;
-    // Invalidate pointer if what it points to has been deleted
-    bool _invalidate(Exiv2::datum_type* deleted) {
-        if (deleted == ptr)
-            invalidated = true;
-        return invalidated;
-    }
 };
 %}
 %typemap(in) const Exiv2::datum_type& {
@@ -223,6 +227,12 @@ public:
     $result = SWIG_NewPointerObj(
         SWIG_as_voidptr(new datum_type##_reference($1)),
         $descriptor(datum_type##_reference*), SWIG_POINTER_OWN);
+#if SWIG_VERSION >= 0x040400
+    // Keep weak reference to the Python result
+    if (store_pointer(self, $result)) {
+        SWIG_fail;
+    }
+#endif // SWIG_VERSION
 }
 
 %enddef // METADATUM_WRAPPERS

src/interface/shared/data_iterator.i src/interface/shared/pointer_store.isrc/interface/shared/data_iterator.i renamed to src/interface/shared/pointer_store.i

@@ -16,36 +16,35 @@
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 
-%include "shared/keep_reference.i"
 %include "shared/private_data.i"
 
 
-// Macro to wrap data iterators
-%define DATA_ITERATOR(container_type, datum_type)
+// Macro to store weak references to pointers and invalidate the
+// pointers when data is deleted
+%define POINTER_STORE(container_type, datum_type)
 
 #if SWIG_VERSION >= 0x040400
-// Functions to store weak references to iterators (swig >= v4.4)
-%fragment("iterator_store", "header", fragment="private_data") {
+// Functions to store weak references to pointers (swig >= v4.4)
+%fragment("pointer_store", "header", fragment="private_data") {
 static void _process_list(PyObject* list, bool invalidate_all,
                           Exiv2::container_type::iterator* beg,
                           Exiv2::container_type::iterator* end) {
-    PyObject* py_it = NULL;
-    container_type##_iterator* cpp_it = NULL;
+    PyObject* py_ptr = NULL;
+    datum_type##_pointer* cpp_ptr = NULL;
     for (Py_ssize_t idx = PyList_Size(list); idx > 0; idx--) {
-        py_it = PyWeakref_GetObject(PyList_GetItem(list, idx-1));
-        if (py_it == Py_None)
+        py_ptr = PyWeakref_GetObject(PyList_GetItem(list, idx-1));
+        if (py_ptr == Py_None)
             goto forget;
         if (!(invalidate_all || beg))
             continue;
-        if (SWIG_IsOK(SWIG_ConvertPtr(
-                py_it, (void**)&cpp_it,
-                $descriptor(container_type##_iterator*), 0))) {
+        if (SWIG_IsOK(SWIG_ConvertPtr(py_ptr, (void**)&cpp_ptr,
+                $descriptor(datum_type##_pointer*), 0))) {
             if (invalidate_all) {
-                cpp_it->_invalidate();
+                cpp_ptr->_invalidate();
                 goto forget;
             }
             for (Exiv2::container_type::iterator it=*beg; it!=*end; it++)
-                if (cpp_it->_invalidate(it))
+                if (cpp_ptr->_invalidate(*it))
                     goto forget;
         }
         continue;
@@ -54,44 +53,44 @@ forget:
         continue;
     }
 };
-static void purge_iterators(PyObject* list) {
+static void purge_pointers(PyObject* list) {
     _process_list(list, false, NULL, NULL);
 };
-static void invalidate_iterators(PyObject* py_self) {
-    PyObject* list = private_store_get(py_self, "iterators");
+static void invalidate_pointers(PyObject* py_self) {
+    PyObject* list = private_store_get(py_self, "pointers");
     if (list)
         _process_list(list, true, NULL, NULL);
 };
-static void invalidate_iterators(PyObject* py_self,
-                                 Exiv2::container_type::iterator pos) {
-    PyObject* list = private_store_get(py_self, "iterators");
+static void invalidate_pointers(PyObject* py_self,
+                                Exiv2::container_type::iterator pos) {
+    PyObject* list = private_store_get(py_self, "pointers");
     if (list) {
         Exiv2::container_type::iterator end = pos;
         end++;
         _process_list(list, false, &pos, &end);
     }
 };
-static void invalidate_iterators(PyObject* py_self,
-                                 Exiv2::container_type::iterator beg,
-                                 Exiv2::container_type::iterator end) {
-    PyObject* list = private_store_get(py_self, "iterators");
+static void invalidate_pointers(PyObject* py_self,
+                                Exiv2::container_type::iterator beg,
+                                Exiv2::container_type::iterator end) {
+    PyObject* list = private_store_get(py_self, "pointers");
     if (list)
         _process_list(list, false, &beg, &end);
 };
-static int store_iterator(PyObject* py_self, PyObject* iterator) {
-    PyObject* list = private_store_get(py_self, "iterators");
+static int store_pointer(PyObject* py_self, PyObject* py_ptr) {
+    PyObject* list = private_store_get(py_self, "pointers");
     if (list)
-        purge_iterators(list);
+        purge_pointers(list);
     else {
         list = PyList_New(0);
         if (!list)
             return -1;
-        int error = private_store_set(py_self, "iterators", list);
+        int error = private_store_set(py_self, "pointers", list);
         Py_DECREF(list);
         if (error)
             return -1;
     }
-    PyObject* ref = PyWeakref_NewRef(iterator, NULL);
+    PyObject* ref = PyWeakref_NewRef(py_ptr, NULL);
     if (!ref)
         return -1;
     int result = PyList_Append(list, ref);
@@ -113,24 +112,24 @@ static int store_iterator(PyObject* py_self, PyObject* iterator) {
 #endif
 
 #if SWIG_VERSION >= 0x040400
-// clear() invalidates all iterators
-%typemap(ret, fragment="iterator_store") void clear {
-    invalidate_iterators(self);
+// clear() invalidates all pointers
+%typemap(ret, fragment="pointer_store") void clear {
+    invalidate_pointers(self);
 }
-// erase() and eraseFamily() invalidate some iterators
-%typemap(check, fragment="iterator_store")
+// erase() and eraseFamily() invalidate some pointers
+%typemap(check, fragment="pointer_store")
         Exiv2::container_type::iterator pos {
-    invalidate_iterators(self, $1);
+    invalidate_pointers(self, $1);
 }
-%typemap(check, fragment="iterator_store")
+%typemap(check, fragment="pointer_store")
         (Exiv2::container_type::iterator beg,
          Exiv2::container_type::iterator end) {
-    invalidate_iterators(self, $1, $2);
+    invalidate_pointers(self, $1, $2);
 }
-%typemap(check, fragment="iterator_store")
+%typemap(check, fragment="pointer_store")
         Exiv2::container_type::iterator& pos {
-    invalidate_iterators(self, *$1, arg1->end());
+    invalidate_pointers(self, *$1, arg1->end());
 }
 #endif // SWIG_VERSION
 
-%enddef // DATA_ITERATOR
+%enddef // POINTER_STORE

src/interface/xmp.i

@@ -25,7 +25,6 @@
 
 %include "shared/preamble.i"
 %include "shared/containers.i"
-%include "shared/data_iterator.i"
 %include "shared/metadatum_wrappers.i"
 
 %include "stdint.i"
@@ -41,8 +40,6 @@ EXCEPTION()
 
 EXTEND_METADATUM(Xmpdatum)
 
-DATA_ITERATOR(XmpData, Xmpdatum)
-
 METADATUM_WRAPPERS(XmpData, Xmpdatum)
 
 // Get the current (or default if not set) type id of a datum