python: Continuing message re-use fix

python: Continuing message re-use fix

[Floris Bruynooghe]

Hi,

This builds on the patch by Anton Khirnov to fix the message re-use
that is possible when accessing messages from a thread.  I started
with just addressing my own comments on this patch, but evolved it
into switching the logic around and leave the normal Message object
untouched.  Instead I created a new OwnedMessage which is used by
the Thread which does not free itself on __del__().  I think this
is preferable because the other iterators, mainly Database.messages(),
do not allow retrieving messages more than once since the query object
is hidden from the API.

I've left the original commit in this patch series to not alter any
contributions.

Cheers,
Floris

[PATCH 1/2] python/notmuch2: do not destroy messages owned by a query

[Floris Bruynooghe]

From: Anton Khirnov <anton@khirnov.net>

Any messages retrieved from a query - either directly via
search_messages() or indirectly via thread objects - are owned by that
query. Retrieving the same message (i.e. corresponding to the same
message ID / database object) several times will always yield the same
C object.

The caller is allowed to destroy message objects owned by a query before
the query itself - which can save memory for long-lived queries.
However, that message must then never be retrieved again from that
query.

The python-notmuch2 bindings will currently destroy every message object
in Message._destroy(), which will lead to an invalid free if the same
message is then retrieved again. E.g. the following python program leads
to libtalloc abort()ing:

import notmuch2
db   = notmuch2.Database(mode = notmuch2.Database.MODE.READ_ONLY)
t    = next(db.threads('*'))
msgs = list(zip(t.toplevel(), t.toplevel()))
msgs = list(zip(t.toplevel(), t.toplevel()))

Fix this issue by creating a subclass of Message, which is used for
"standalone" message which have to be freed by the caller. Message class
is then used only for messages descended from a query, which do not need
to be freed by the caller.
---
 bindings/python-cffi/notmuch2/_database.py |  6 ++--
 bindings/python-cffi/notmuch2/_message.py  | 42 ++++++++++++++--------
 2 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/bindings/python-cffi/notmuch2/_database.py b/bindings/python-cffi/notmuch2/_database.py
index 95f59ca0..f14eac78 100644
--- a/bindings/python-cffi/notmuch2/_database.py
+++ b/bindings/python-cffi/notmuch2/_database.py
@@ -399,7 +399,7 @@ class Database(base.NotmuchObject):
               capi.lib.NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID]
         if ret not in ok:
             raise errors.NotmuchError(ret)
-        msg = message.Message(self, msg_pp[0], db=self)
+        msg = message.StandaloneMessage(self, msg_pp[0], db=self)
         if sync_flags:
             msg.tags.from_maildir_flags()
         return self.AddedMessage(
@@ -468,7 +468,7 @@ class Database(base.NotmuchObject):
         msg_p = msg_pp[0]
         if msg_p == capi.ffi.NULL:
             raise LookupError
-        msg = message.Message(self, msg_p, db=self)
+        msg = message.StandaloneMessage(self, msg_p, db=self)
         return msg
 
     def get(self, filename):
@@ -501,7 +501,7 @@ class Database(base.NotmuchObject):
         msg_p = msg_pp[0]
         if msg_p == capi.ffi.NULL:
             raise LookupError
-        msg = message.Message(self, msg_p, db=self)
+        msg = message.StandaloneMessage(self, msg_p, db=self)
         return msg
 
     @property
diff --git a/bindings/python-cffi/notmuch2/_message.py b/bindings/python-cffi/notmuch2/_message.py
index c5fdbf6d..416ce7ca 100644
--- a/bindings/python-cffi/notmuch2/_message.py
+++ b/bindings/python-cffi/notmuch2/_message.py
@@ -14,7 +14,7 @@ __all__ = ['Message']
 
 
 class Message(base.NotmuchObject):
-    """An email message stored in the notmuch database.
+    """An email message stored in the notmuch database retrieved via a query.
 
     This should not be directly created, instead it will be returned
     by calling methods on :class:`Database`.  A message keeps a
@@ -61,22 +61,10 @@ class Message(base.NotmuchObject):
 
     @property
     def alive(self):
-        if not self._parent.alive:
-            return False
-        try:
-            self._msg_p
-        except errors.ObjectDestroyedError:
-            return False
-        else:
-            return True
-
-    def __del__(self):
-        self._destroy()
+        return self._parent.alive
 
     def _destroy(self):
-        if self.alive:
-            capi.lib.notmuch_message_destroy(self._msg_p)
-        self._msg_p = None
+        pass
 
     @property
     def messageid(self):
@@ -375,6 +363,30 @@ class Message(base.NotmuchObject):
         if isinstance(other, self.__class__):
             return self.messageid == other.messageid
 
+class StandaloneMessage(Message):
+    """An email message stored in the notmuch database.
+
+    This subclass of Message is used for messages that are retrieved from the
+    database directly and are not owned by a query.
+    """
+    @property
+    def alive(self):
+        if not self._parent.alive:
+            return False
+        try:
+            self._msg_p
+        except errors.ObjectDestroyedError:
+            return False
+        else:
+            return True
+
+    def __del__(self):
+        self._destroy()
+
+    def _destroy(self):
+        if self.alive:
+            capi.lib.notmuch_message_destroy(self._msg_p)
+        self._msg_p = None
 
 class FilenamesIter(base.NotmuchIter):
     """Iterator for binary filenames objects."""
-- 
2.27.0

[PATCH 2/2] Make messages returned by Thread objects owned

[Floris Bruynooghe]

This reverses the logic of StandaloneMessage to instead create a
OwnedMessage.  Only the Thread class allows retrieving messages more
then once so it can explicitly create such messages.

The added test fails with SIGABRT without the fix for the message
re-use in threads being present.
---
 bindings/python-cffi/notmuch2/_database.py  |  6 +--
 bindings/python-cffi/notmuch2/_message.py   | 55 ++++++++++++---------
 bindings/python-cffi/notmuch2/_thread.py    |  8 ++-
 bindings/python-cffi/tests/test_database.py | 11 +++++
 4 files changed, 51 insertions(+), 29 deletions(-)

diff --git a/bindings/python-cffi/notmuch2/_database.py b/bindings/python-cffi/notmuch2/_database.py
index f14eac78..95f59ca0 100644
--- a/bindings/python-cffi/notmuch2/_database.py
+++ b/bindings/python-cffi/notmuch2/_database.py
@@ -399,7 +399,7 @@ class Database(base.NotmuchObject):
               capi.lib.NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID]
         if ret not in ok:
             raise errors.NotmuchError(ret)
-        msg = message.StandaloneMessage(self, msg_pp[0], db=self)
+        msg = message.Message(self, msg_pp[0], db=self)
         if sync_flags:
             msg.tags.from_maildir_flags()
         return self.AddedMessage(
@@ -468,7 +468,7 @@ class Database(base.NotmuchObject):
         msg_p = msg_pp[0]
         if msg_p == capi.ffi.NULL:
             raise LookupError
-        msg = message.StandaloneMessage(self, msg_p, db=self)
+        msg = message.Message(self, msg_p, db=self)
         return msg
 
     def get(self, filename):
@@ -501,7 +501,7 @@ class Database(base.NotmuchObject):
         msg_p = msg_pp[0]
         if msg_p == capi.ffi.NULL:
             raise LookupError
-        msg = message.StandaloneMessage(self, msg_p, db=self)
+        msg = message.Message(self, msg_p, db=self)
         return msg
 
     @property
diff --git a/bindings/python-cffi/notmuch2/_message.py b/bindings/python-cffi/notmuch2/_message.py
index 416ce7ca..02de50ad 100644
--- a/bindings/python-cffi/notmuch2/_message.py
+++ b/bindings/python-cffi/notmuch2/_message.py
@@ -47,9 +47,7 @@ class Message(base.NotmuchObject):
     :type db: Database
     :param msg_p: The C pointer to the ``notmuch_message_t``.
     :type msg_p: <cdata>
-
     :param dup: Whether the message was a duplicate on insertion.
-
     :type dup: None or bool
     """
     _msg_p = base.MemoryPointer()
@@ -61,10 +59,22 @@ class Message(base.NotmuchObject):
 
     @property
     def alive(self):
-        return self._parent.alive
+        if not self._parent.alive:
+            return False
+        try:
+            self._msg_p
+        except errors.ObjectDestroyedError:
+            return False
+        else:
+            return True
+
+    def __del__(self):
+        self._destroy()
 
     def _destroy(self):
-        pass
+        if self.alive:
+            capi.lib.notmuch_message_destroy(self._msg_p)
+        self._msg_p = None
 
     @property
     def messageid(self):
@@ -363,30 +373,26 @@ class Message(base.NotmuchObject):
         if isinstance(other, self.__class__):
             return self.messageid == other.messageid
 
-class StandaloneMessage(Message):
-    """An email message stored in the notmuch database.
 
-    This subclass of Message is used for messages that are retrieved from the
-    database directly and are not owned by a query.
+class OwnedMessage(Message):
+    """An email message owned by parent thread object.
+
+    This subclass of Message is used for messages that are retrieved
+    from the notmuch database via a parent :class:`notmuch2.Thread`
+    object, which "owns" this message.  This means that when this
+    message object is destroyed, by calling :func:`del` or
+    :meth:`_destroy` directly or indirectly, the message is not freed
+    in the notmuch API and the parent :class:`notmuch2.Thread` object
+    can return the same object again when needed.
     """
+
     @property
     def alive(self):
-        if not self._parent.alive:
-            return False
-        try:
-            self._msg_p
-        except errors.ObjectDestroyedError:
-            return False
-        else:
-            return True
-
-    def __del__(self):
-        self._destroy()
+        return self._parent.alive
 
     def _destroy(self):
-        if self.alive:
-            capi.lib.notmuch_message_destroy(self._msg_p)
-        self._msg_p = None
+        pass
+
 
 class FilenamesIter(base.NotmuchIter):
     """Iterator for binary filenames objects."""
@@ -690,8 +696,9 @@ collections.abc.ValuesView.register(PropertiesValuesView)
 
 class MessageIter(base.NotmuchIter):
 
-    def __init__(self, parent, msgs_p, *, db):
+    def __init__(self, parent, msgs_p, *, db, msg_cls=Message):
         self._db = db
+        self._msg_cls = msg_cls
         super().__init__(parent, msgs_p,
                          fn_destroy=capi.lib.notmuch_messages_destroy,
                          fn_valid=capi.lib.notmuch_messages_valid,
@@ -700,4 +707,4 @@ class MessageIter(base.NotmuchIter):
 
     def __next__(self):
         msg_p = super().__next__()
-        return Message(self, msg_p, db=self._db)
+        return self._msg_cls(self, msg_p, db=self._db)
diff --git a/bindings/python-cffi/notmuch2/_thread.py b/bindings/python-cffi/notmuch2/_thread.py
index bb76f2dc..e883f308 100644
--- a/bindings/python-cffi/notmuch2/_thread.py
+++ b/bindings/python-cffi/notmuch2/_thread.py
@@ -62,7 +62,9 @@ class Thread(base.NotmuchObject, collections.abc.Iterable):
         :raises ObjectDestroyedError: if used after destroyed.
         """
         msgs_p = capi.lib.notmuch_thread_get_toplevel_messages(self._thread_p)
-        return message.MessageIter(self, msgs_p, db=self._db)
+        return message.MessageIter(self, msgs_p,
+                                   db=self._db,
+                                   msg_cls=message.OwnedMessage)
 
     def __iter__(self):
         """Return an iterator over all the messages in the thread.
@@ -72,7 +74,9 @@ class Thread(base.NotmuchObject, collections.abc.Iterable):
         :raises ObjectDestroyedError: if used after destroyed.
         """
         msgs_p = capi.lib.notmuch_thread_get_messages(self._thread_p)
-        return message.MessageIter(self, msgs_p, db=self._db)
+        return message.MessageIter(self, msgs_p,
+                                   db=self._db,
+                                   msg_cls=message.OwnedMessage)
 
     @property
     def matched(self):
diff --git a/bindings/python-cffi/tests/test_database.py b/bindings/python-cffi/tests/test_database.py
index e3a8344d..55069b5e 100644
--- a/bindings/python-cffi/tests/test_database.py
+++ b/bindings/python-cffi/tests/test_database.py
@@ -324,3 +324,14 @@ class TestQuery:
         threads = db.threads('*')
         thread = next(threads)
         assert isinstance(thread, notmuch2.Thread)
+
+    def test_use_threaded_message_twice(self, db):
+        thread = next(db.threads('*'))
+        for msg in thread.toplevel():
+            assert isinstance(msg, notmuch2.Message)
+            assert msg.alive
+            del msg
+        for msg in thread:
+            assert isinstance(msg, notmuch2.Message)
+            assert msg.alive
+            del msg
-- 
2.27.0

Re: python: Continuing message re-use fix

[David Bremner]

Floris Bruynooghe <flub@devork.be> writes:

> Hi,
>
> This builds on the patch by Anton Khirnov to fix the message re-use
> that is possible when accessing messages from a thread.  I started
> with just addressing my own comments on this patch, but evolved it
> into switching the logic around and leave the normal Message object
> untouched.  Instead I created a new OwnedMessage which is used by
> the Thread which does not free itself on __del__().  I think this
> is preferable because the other iterators, mainly Database.messages(),
> do not allow retrieving messages more than once since the query object
> is hidden from the API.
>
> I've left the original commit in this patch series to not alter any
> contributions.
>
> Cheers,
> Floris

Revised series merged to master and release.

Thanks for your help with these updates.

d