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

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

[Anton Khirnov]

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.20.1

[PATCH 2/2] python/notmuch2: add bindings for the database config strings

[Anton Khirnov]

---
 bindings/python-cffi/notmuch2/_build.py    | 17 +++++
 bindings/python-cffi/notmuch2/_config.py   | 84 ++++++++++++++++++++++
 bindings/python-cffi/notmuch2/_database.py | 23 ++++++
 3 files changed, 124 insertions(+)
 create mode 100644 bindings/python-cffi/notmuch2/_config.py

diff --git a/bindings/python-cffi/notmuch2/_build.py b/bindings/python-cffi/notmuch2/_build.py
index 5e1fcac1..f269f2a1 100644
--- a/bindings/python-cffi/notmuch2/_build.py
+++ b/bindings/python-cffi/notmuch2/_build.py
@@ -314,6 +314,23 @@ ffibuilder.cdef(
     notmuch_indexopts_get_decrypt_policy (const notmuch_indexopts_t *indexopts);
     void
     notmuch_indexopts_destroy (notmuch_indexopts_t *options);
+
+    notmuch_status_t
+    notmuch_database_set_config (notmuch_database_t *db, const char *key, const char *value);
+    notmuch_status_t
+    notmuch_database_get_config (notmuch_database_t *db, const char *key, char **value);
+    notmuch_status_t
+    notmuch_database_get_config_list (notmuch_database_t *db, const char *prefix, notmuch_config_list_t **out);
+    notmuch_bool_t
+    notmuch_config_list_valid (notmuch_config_list_t *config_list);
+    const char *
+    notmuch_config_list_key (notmuch_config_list_t *config_list);
+    const char *
+    notmuch_config_list_value (notmuch_config_list_t *config_list);
+    void
+    notmuch_config_list_move_to_next (notmuch_config_list_t *config_list);
+    void
+    notmuch_config_list_destroy (notmuch_config_list_t *config_list);
     """
 )
 
diff --git a/bindings/python-cffi/notmuch2/_config.py b/bindings/python-cffi/notmuch2/_config.py
new file mode 100644
index 00000000..58383c16
--- /dev/null
+++ b/bindings/python-cffi/notmuch2/_config.py
@@ -0,0 +1,84 @@
+import collections.abc
+
+import notmuch2._base as base
+import notmuch2._capi as capi
+import notmuch2._errors as errors
+
+__all__ = ['ConfigMapping']
+
+class ConfigIter(base.NotmuchIter):
+    def __init__(self, parent, iter_p):
+        super().__init__(
+            parent, iter_p,
+            fn_destroy=capi.lib.notmuch_config_list_destroy,
+            fn_valid=capi.lib.notmuch_config_list_valid,
+            fn_get=capi.lib.notmuch_config_list_key,
+            fn_next=capi.lib.notmuch_config_list_move_to_next)
+
+    def __next__(self):
+        item = super().__next__()
+        return base.BinString.from_cffi(item)
+
+class ConfigMapping(base.NotmuchObject, collections.abc.MutableMapping):
+    """The config key/value pairs stored in the database.
+
+    The entries are exposed as a :class:`collections.abc.MutableMapping` object.
+    Note that setting a value to an empty string is the same as deleting it.
+
+    :param parent: the parent object
+    :param ptr_name: the name of the attribute on the parent which will
+       return the memory pointer.  This allows this object to
+       access the pointer via the parent's descriptor and thus
+       trigger :class:`MemoryPointer`'s memory safety.
+    """
+
+    def __init__(self, parent, ptr_name):
+        self._parent = parent
+        self._ptr = lambda: getattr(parent, ptr_name)
+
+    @property
+    def alive(self):
+        return self._parent.alive
+
+    def _destroy(self):
+        pass
+
+    def __getitem__(self, key):
+        if isinstance(key, str):
+            key = key.encode('utf-8')
+        val_pp = capi.ffi.new('char**')
+        ret = capi.lib.notmuch_database_get_config(self._ptr(), key, val_pp)
+        if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
+            raise errors.NotmuchError(ret)
+        if val_pp[0] == "":
+            capi.lib.free(val_pp[0])
+            raise KeyError
+        val = base.BinString.from_cffi(val_pp[0])
+        capi.lib.free(val_pp[0])
+        return val
+
+    def __setitem__(self, key, val):
+        if isinstance(key, str):
+            key = key.encode('utf-8')
+        if isinstance(val, str):
+            val = val.encode('utf-8')
+        ret = capi.lib.notmuch_database_set_config(self._ptr(), key, val)
+        if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
+            raise errors.NotmuchError(ret)
+
+    def __delitem__(self, key):
+        self[key] = ""
+
+    def __iter__(self):
+        """Return an iterator over the config items.
+
+        :raises NullPointerError: If the iterator can not be created.
+        """
+        configlist_pp = capi.ffi.new('notmuch_config_list_t**')
+        ret = capi.lib.notmuch_database_get_config_list(self._ptr(), b'', configlist_pp)
+        if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
+            raise errors.NotmuchError(ret)
+        return ConfigIter(self._parent, configlist_pp[0])
+
+    def __len__(self):
+        return sum(1 for t in self)
diff --git a/bindings/python-cffi/notmuch2/_database.py b/bindings/python-cffi/notmuch2/_database.py
index f14eac78..fc55fea8 100644
--- a/bindings/python-cffi/notmuch2/_database.py
+++ b/bindings/python-cffi/notmuch2/_database.py
@@ -7,6 +7,7 @@ import pathlib
 import weakref
 
 import notmuch2._base as base
+import notmuch2._config as config
 import notmuch2._capi as capi
 import notmuch2._errors as errors
 import notmuch2._message as message
@@ -536,6 +537,28 @@ class Database(base.NotmuchObject):
             self._cached_tagset = weakref.ref(tagset)
         return tagset
 
+    @property
+    def config(self):
+        """Return a mutable mapping with the settings stored in this database.
+
+        This returns an mutable dict-like object implementing the
+        collections.abc.MutableMapping Abstract Base Class.
+
+        :rtype: Config
+
+        :raises ObjectDestroyedError: if used after destroyed.
+        """
+        try:
+            ref = self._cached_config
+        except AttributeError:
+            config_mapping = None
+        else:
+            config_mapping = ref()
+        if config_mapping is None:
+            config_mapping = config.ConfigMapping(self, '_db_p')
+            self._cached_config = weakref.ref(config_mapping)
+        return config_mapping
+
     def _create_query(self, query, *,
                       omit_excluded=EXCLUDE.TRUE,
                       sort=SORT.UNSORTED,  # Check this default
-- 
2.20.1

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

[Anton Khirnov]

ping

-- 
Anton Khirnov

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

[David Bremner]

Anton Khirnov <anton@khirnov.net> writes:

> ping

I'm hoping Floris will find a chance to review your patches.

d

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

[Floris Bruynooghe]

Hi Anton,

Thanks for improving the bindings!  Any my apologies for the late
response, I failed to spot this mail the first time round.

Also, this is a pretty serious bug, thanks for finding it.

This looks pretty solid, a few small style comments that aren't very
important notwithstanding.  Though I do think you should be able to add
a test for this in the pytest tests.  I think just triggering the
use-after-free you demonstrate in the commit message is fine as it will
work with your patch.  The fact it will crash rather then fail without
is unfortunate but probably fine.

On Sat 09 May 2020 at 07:05 +0200, Anton Khirnov wrote:

> 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

I feel like some more description from the commit message could live
here to explain why this isn't destroying the message rather than having
to find it in the commit message.

>  
>      @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):

Minor style thing, between top-level code one usually leaves 2 blank
lines per PEP8.  Nowadays perhaps we should think about adopting an
automatic formatter but hey.

(This applies to lots of places in both patches btw)

> +    """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.
> +    """

Likewise I'd be tempted to add here that this message is destroyed as
soon as it is garbage collected pointing out that this is not the case
for Message.  And probably update the Message docstring with the inverse.

> +    @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."""

Re: [PATCH 2/2] python/notmuch2: add bindings for the database config strings

[Floris Bruynooghe]

Thanks for adding more of the API!

This mostly is fine as well, again I'd mainly ask to add tests however.
At least the things which are implemented directly I guess: setitem,
getitem, iter and len.


On Sat 09 May 2020 at 07:05 +0200, Anton Khirnov wrote:

> ---
>  bindings/python-cffi/notmuch2/_build.py    | 17 +++++
>  bindings/python-cffi/notmuch2/_config.py   | 84 ++++++++++++++++++++++
>  bindings/python-cffi/notmuch2/_database.py | 23 ++++++
>  3 files changed, 124 insertions(+)
>  create mode 100644 bindings/python-cffi/notmuch2/_config.py
>
> diff --git a/bindings/python-cffi/notmuch2/_build.py b/bindings/python-cffi/notmuch2/_build.py
> index 5e1fcac1..f269f2a1 100644
> --- a/bindings/python-cffi/notmuch2/_build.py
> +++ b/bindings/python-cffi/notmuch2/_build.py
> @@ -314,6 +314,23 @@ ffibuilder.cdef(
>      notmuch_indexopts_get_decrypt_policy (const notmuch_indexopts_t *indexopts);
>      void
>      notmuch_indexopts_destroy (notmuch_indexopts_t *options);
> +
> +    notmuch_status_t
> +    notmuch_database_set_config (notmuch_database_t *db, const char *key, const char *value);
> +    notmuch_status_t
> +    notmuch_database_get_config (notmuch_database_t *db, const char *key, char **value);
> +    notmuch_status_t
> +    notmuch_database_get_config_list (notmuch_database_t *db, const char *prefix, notmuch_config_list_t **out);
> +    notmuch_bool_t
> +    notmuch_config_list_valid (notmuch_config_list_t *config_list);
> +    const char *
> +    notmuch_config_list_key (notmuch_config_list_t *config_list);
> +    const char *
> +    notmuch_config_list_value (notmuch_config_list_t *config_list);
> +    void
> +    notmuch_config_list_move_to_next (notmuch_config_list_t *config_list);
> +    void
> +    notmuch_config_list_destroy (notmuch_config_list_t *config_list);
>      """
>  )
>  
> diff --git a/bindings/python-cffi/notmuch2/_config.py b/bindings/python-cffi/notmuch2/_config.py
> new file mode 100644
> index 00000000..58383c16
> --- /dev/null
> +++ b/bindings/python-cffi/notmuch2/_config.py
> @@ -0,0 +1,84 @@
> +import collections.abc
> +
> +import notmuch2._base as base
> +import notmuch2._capi as capi
> +import notmuch2._errors as errors
> +
> +__all__ = ['ConfigMapping']
> +

Same style thing about 2 blank lines

> +class ConfigIter(base.NotmuchIter):
> +    def __init__(self, parent, iter_p):
> +        super().__init__(
> +            parent, iter_p,
> +            fn_destroy=capi.lib.notmuch_config_list_destroy,
> +            fn_valid=capi.lib.notmuch_config_list_valid,
> +            fn_get=capi.lib.notmuch_config_list_key,
> +            fn_next=capi.lib.notmuch_config_list_move_to_next)
> +
> +    def __next__(self):
> +        item = super().__next__()
> +        return base.BinString.from_cffi(item)
> +
> +class ConfigMapping(base.NotmuchObject, collections.abc.MutableMapping):
> +    """The config key/value pairs stored in the database.
> +
> +    The entries are exposed as a :class:`collections.abc.MutableMapping` object.
> +    Note that setting a value to an empty string is the same as deleting it.
> +
> +    :param parent: the parent object
> +    :param ptr_name: the name of the attribute on the parent which will
> +       return the memory pointer.  This allows this object to
> +       access the pointer via the parent's descriptor and thus
> +       trigger :class:`MemoryPointer`'s memory safety.
> +    """
> +
> +    def __init__(self, parent, ptr_name):
> +        self._parent = parent
> +        self._ptr = lambda: getattr(parent, ptr_name)
> +
> +    @property
> +    def alive(self):
> +        return self._parent.alive
> +
> +    def _destroy(self):
> +        pass
> +
> +    def __getitem__(self, key):
> +        if isinstance(key, str):
> +            key = key.encode('utf-8')
> +        val_pp = capi.ffi.new('char**')
> +        ret = capi.lib.notmuch_database_get_config(self._ptr(), key, val_pp)
> +        if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
> +            raise errors.NotmuchError(ret)
> +        if val_pp[0] == "":
> +            capi.lib.free(val_pp[0])
> +            raise KeyError
> +        val = base.BinString.from_cffi(val_pp[0])
> +        capi.lib.free(val_pp[0])
> +        return val
> +
> +    def __setitem__(self, key, val):
> +        if isinstance(key, str):
> +            key = key.encode('utf-8')
> +        if isinstance(val, str):
> +            val = val.encode('utf-8')
> +        ret = capi.lib.notmuch_database_set_config(self._ptr(), key, val)
> +        if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
> +            raise errors.NotmuchError(ret)
> +
> +    def __delitem__(self, key):
> +        self[key] = ""
> +
> +    def __iter__(self):
> +        """Return an iterator over the config items.
> +
> +        :raises NullPointerError: If the iterator can not be created.
> +        """
> +        configlist_pp = capi.ffi.new('notmuch_config_list_t**')
> +        ret = capi.lib.notmuch_database_get_config_list(self._ptr(), b'', configlist_pp)
> +        if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
> +            raise errors.NotmuchError(ret)
> +        return ConfigIter(self._parent, configlist_pp[0])
> +
> +    def __len__(self):
> +        return sum(1 for t in self)
> diff --git a/bindings/python-cffi/notmuch2/_database.py b/bindings/python-cffi/notmuch2/_database.py
> index f14eac78..fc55fea8 100644
> --- a/bindings/python-cffi/notmuch2/_database.py
> +++ b/bindings/python-cffi/notmuch2/_database.py
> @@ -7,6 +7,7 @@ import pathlib
>  import weakref
>  
>  import notmuch2._base as base
> +import notmuch2._config as config
>  import notmuch2._capi as capi
>  import notmuch2._errors as errors
>  import notmuch2._message as message
> @@ -536,6 +537,28 @@ class Database(base.NotmuchObject):
>              self._cached_tagset = weakref.ref(tagset)
>          return tagset
>  
> +    @property
> +    def config(self):
> +        """Return a mutable mapping with the settings stored in this database.
> +
> +        This returns an mutable dict-like object implementing the
> +        collections.abc.MutableMapping Abstract Base Class.
> +
> +        :rtype: Config
> +
> +        :raises ObjectDestroyedError: if used after destroyed.
> +        """
> +        try:
> +            ref = self._cached_config
> +        except AttributeError:
> +            config_mapping = None
> +        else:
> +            config_mapping = ref()
> +        if config_mapping is None:
> +            config_mapping = config.ConfigMapping(self, '_db_p')
> +            self._cached_config = weakref.ref(config_mapping)
> +        return config_mapping
> +
>      def _create_query(self, query, *,
>                        omit_excluded=EXCLUDE.TRUE,
>                        sort=SORT.UNSORTED,  # Check this default

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

[Tomi Ollila]

Hi Anton,

On Thu, May 21 2020, Anton Khirnov wrote:

> ping

Great (an important) stuff in this series !

...just that...

Floris did good review on your changes the next day.

What I remember was there (looked 30 mins ago)

- style: somewhat important (I press more there). Easy to get better,
         good and consistent style makes it better

- docs in code: like Floris mentioned, the docs in commit messages are
                are not seen by user. The relevant documentation is 
                to be with the code. commit message tells what was done
                and why

- tests: since pytest is used to test the current notmuch2 cffi python
         tests, all new code that can be tested pytest code to do so
         is to be added

And these have to be done now -- postponing such a things into future
will just mean that it never happens, and the messines and entropy of
the notmuch code gets even greater in such a case...

>
> -- 
> Anton Khirnov

Tomi

[PATCH] wip test for id:20200509050526.23148-1-anton@khirnov.net

[David Bremner]

---

I'm not the best person to write a good commit message here; probably
some Anton's patch 1/2 message should be moved here.

That patch should apply after this one, and remote the
test_subtest_known_broken. Of course someone is welcome to improve the
test and inline comments.


 bindings/python-cffi/tests/test_database.py | 7 +++++++
 test/T391-python-cffi.sh                    | 1 +
 2 files changed, 8 insertions(+)

diff --git a/bindings/python-cffi/tests/test_database.py b/bindings/python-cffi/tests/test_database.py
index e3a8344d..df504daa 100644
--- a/bindings/python-cffi/tests/test_database.py
+++ b/bindings/python-cffi/tests/test_database.py
@@ -324,3 +324,10 @@ class TestQuery:
         threads = db.threads('*')
         thread = next(threads)
         assert isinstance(thread, notmuch2.Thread)
+
+    def test_memory_bug(self, db):
+        # check early destroy bug reported in id:20200509050526.23148-1-anton@khirnov.net
+        t    = next(db.threads('*'))
+        msgs = list(zip(t.toplevel(), t.toplevel()))
+        msgs = list(zip(t.toplevel(), t.toplevel()))
+        assert len(msgs) != 0
diff --git a/test/T391-python-cffi.sh b/test/T391-python-cffi.sh
index f961069b..9407cc81 100755
--- a/test/T391-python-cffi.sh
+++ b/test/T391-python-cffi.sh
@@ -8,6 +8,7 @@ fi
 
 
 test_begin_subtest "python cffi tests"
+test_subtest_known_broken
 pytest_dir=$NOTMUCH_BUILDDIR/bindings/python-cffi/build/stage
 printf "[pytest]\nminversion = 3.0\naddopts = -ra\n" > $pytest_dir/pytest.ini
 test_expect_success "(cd $pytest_dir && ${NOTMUCH_PYTHON} -m pytest --log-file=$TMP_DIRECTORY/test.output)"
-- 
2.27.0