use after free in python notmuch2 bindings

use after free in python notmuch2 bindings

[David Bremner]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]


I've been attempting to port nmweb to the new bindings, but I got stuck
on a bug that segfaults python. I attached a reduced version that
reproduces the problem for me. It uses recent messages from the notmuch
list; it others can't reproduce let me know and I will try to make
something more self contained including a message set.

It's a bit tricky to get ASAN working but I managed with

% env ASAN_OPTIONS=alloc_dealloc_mismatch=0 LD_PRELOAD="libasan.so.6 libstdc++.so.6" LD_LIBRARY_PATH=../../lib python3 ~/test.py

You can see in the attached output that one of the notmuch messages
structs is used after being freed. I suspect it has something to do with
the iterator code in the bindings, but I have not examined it in detail.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: reproducer for use after free --]
[-- Type: text/x-python, Size: 487 bytes --]

from notmuch2 import Database

def mailto_addrs(msg,header_name):
    hdr = msg.header(header_name)
    return 

def show_msgs(msgs):
  print("show msgs" + str(msgs))
  for msg in msgs:
      print("\t",msg.messageid)
      frm = mailto_addrs(msg,'From')
      rs = show_msgs(msg.replies())
  return 

db = Database(config=Database.CONFIG.SEARCH)
msg=db.find("87fsqijx7u.fsf@metapensiero.it")
threads = db.threads(query="thread:"+msg.threadid)
thread = next (threads)

show_msgs(thread)

[-- Attachment #3: asan.out --]
[-- Type: application/octet-stream, Size: 3820 bytes --]

=================================================================
==571087==ERROR: AddressSanitizer: heap-use-after-free on address 0x6120000021b0 at pc 0x7f2500d36671 bp 0x7ffdb9fa8670 sp 0x7ffdb9fa8668
READ of size 8 at 0x6120000021b0 thread T0
    #0 0x7f2500d36670 in notmuch_message_get_message_id lib/message.cc:525
    #1 0x7f2501612b76 in _cffi_f_notmuch_message_get_message_id build/temp.linux-x86_64-3.9/notmuch2._capi.c:3079
    #2 0x524dbc  (/usr/bin/python3.9+0x524dbc)
    #3 0x514653 in _PyEval_EvalFrameDefault (/usr/bin/python3.9+0x514653)
    #4 0x525912 in _PyFunction_Vectorcall (/usr/bin/python3.9+0x525912)
    #5 0x5342b2  (/usr/bin/python3.9+0x5342b2)
    #6 0x523f57 in _PyObject_GenericGetAttrWithDict (/usr/bin/python3.9+0x523f57)
    #7 0x50f634 in _PyEval_EvalFrameDefault (/usr/bin/python3.9+0x50f634)
    #8 0x525912 in _PyFunction_Vectorcall (/usr/bin/python3.9+0x525912)
    #9 0x50f71e in _PyEval_EvalFrameDefault (/usr/bin/python3.9+0x50f71e)
    #10 0x525912 in _PyFunction_Vectorcall (/usr/bin/python3.9+0x525912)
    #11 0x50f71e in _PyEval_EvalFrameDefault (/usr/bin/python3.9+0x50f71e)
    #12 0x50deb0  (/usr/bin/python3.9+0x50deb0)
    #13 0x50dc26 in _PyEval_EvalCodeWithName (/usr/bin/python3.9+0x50dc26)
    #14 0x50dbd2 in PyEval_EvalCode (/usr/bin/python3.9+0x50dbd2)
    #15 0x629bc6  (/usr/bin/python3.9+0x629bc6)
    #16 0x626b6f  (/usr/bin/python3.9+0x626b6f)
    #17 0x6295e8  (/usr/bin/python3.9+0x6295e8)
    #18 0x629262 in PyRun_SimpleFileExFlags (/usr/bin/python3.9+0x629262)
    #19 0x620d61 in Py_RunMain (/usr/bin/python3.9+0x620d61)
    #20 0x608bf8 in Py_BytesMain (/usr/bin/python3.9+0x608bf8)
    #21 0x7f25046e07ec in __libc_start_main ../csu/libc-start.c:332
    #22 0x608af9 in _start (/usr/bin/python3.9+0x608af9)

0x6120000021b0 is located 112 bytes inside of 272-byte region [0x612000002140,0x612000002250)
freed by thread T0 here:
    #0 0x7f2504cfb4d7 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:127
    #1 0x7f2501411353 in _tc_free_internal ../../talloc.c:1222

previously allocated by thread T0 here:
    #0 0x7f2504cfb7cf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x7f25014137ef in __talloc_with_prefix ../../talloc.c:783

SUMMARY: AddressSanitizer: heap-use-after-free lib/message.cc:525 in notmuch_message_get_message_id
Shadow bytes around the buggy address:
  0x0c247fff83e0: fd fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa
  0x0c247fff83f0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c247fff8400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c247fff8410: 00 00 00 00 00 00 00 00 00 00 fa fa fa fa fa fa
  0x0c247fff8420: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
=>0x0c247fff8430: fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd
  0x0c247fff8440: fd fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa
  0x0c247fff8450: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c247fff8460: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c247fff8470: 00 00 00 00 00 00 00 00 00 00 fa fa fa fa fa fa
  0x0c247fff8480: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==571087==ABORTING

[-- Attachment #4: Type: text/plain, Size: 0 bytes --]

Re: use after free in python notmuch2 bindings

[David Bremner]

[-- Attachment #1: Type: text/plain, Size: 1992 bytes --]

David Bremner <david@tethera.net> writes:

> I've been attempting to port nmweb to the new bindings, but I got stuck
> on a bug that segfaults python. I attached a reduced version that
> reproduces the problem for me. It uses recent messages from the notmuch
> list; it others can't reproduce let me know and I will try to make
> something more self contained including a message set.
>

Attached is a slightly simpler (and more informative) reproducer

It produces the the following output for me

7f23164b6cd0 <notmuch2.Thread object at 0x7f23164b6cd0>
   87fsqijx7u.fsf@metapensiero.it <cdata 'struct _notmuch_message *' 0x137a000>
     7f23164b6a90 <NotmuchIter>
       87lf0anoiv.fsf@tethera.net <cdata 'struct _notmuch_message *' 0x13636f0>
         7f23164b6910 <NotmuchIter>
           87bl16jezh.fsf@metapensiero.it <cdata 'struct _notmuch_message *' 0x139a6b0>
             7f23164c3070 <NotmuchIter>
           87bl0vlbys.fsf@powell.devork.be <cdata 'struct _notmuch_message *' 0x139b8e0>
             7f23164c30d0 <NotmuchIter>
   87lf0anoiv.fsf@tethera.net <cdata 'struct _notmuch_message *' 0x13636f0>
     7f23164b68e0 <NotmuchIter>
       87bl16jezh.fsf@metapensiero.it <cdata 'struct _notmuch_message *' 0x139a6b0>
         7f23164b6a00 <NotmuchIter>
       87bl0vlbys.fsf@powell.devork.be <cdata 'struct _notmuch_message *' 0x139b8e0>
zsh: IOT instruction  python3 test.py

The IOT instruction is actually talloc aborting. If I leave in the call
to msg.header, it segfaults as before.

I noticed that the message struct 0x139b8e0 is visited twice, once as
part of the thread and once as part of reply-to-reply-to-reply.

I think the issue here is that bindings destroy the iterator for
replies, but the library docs say

"
 * The returned list will be destroyed when the thread is
 * destroyed.
"

Perhaps that needs to be worded more strongly, to forbid the user from
calling notmuch_messages_destroy. I still need to untangle the intended
ownership semantics to be sure.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: test.py --]
[-- Type: text/x-python, Size: 501 bytes --]

from notmuch2 import Database

def show_msgs(msgs, level):
    print('{:s} {:x} {:s}'.format(' ' * level*4, id(msgs), str(msgs)))
    for msg in msgs:
        print('{:s} {:s} {:s}'.format(' ' * (level*4+2), msg.messageid, str(msg._msg_p)))
        replies=msg.replies()
        show_msgs(replies, level+1)
    

db = Database(config=Database.CONFIG.SEARCH)
msg=db.find("87fsqijx7u.fsf@metapensiero.it")
threads = db.threads(query="thread:"+msg.threadid)
thread = next (threads)

show_msgs(thread, 0)

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

[PATCH 1/2] test: add known broken tests for recursive traversal of replies.

[David Bremner]

This reproduces the bug reported at [1]. The second test hints at the
solution, making reply return OwnedMessage objects.

[1]: id:87sfu6utxg.fsf@tethera.net
---
 test/T392-python-cffi-notmuch.sh | 50 ++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100755 test/T392-python-cffi-notmuch.sh

diff --git a/test/T392-python-cffi-notmuch.sh b/test/T392-python-cffi-notmuch.sh
new file mode 100755
index 00000000..50012c55
--- /dev/null
+++ b/test/T392-python-cffi-notmuch.sh
@@ -0,0 +1,50 @@
+#!/usr/bin/env bash
+test_description="python bindings (notmuch test suite)"
+. $(dirname "$0")/test-lib.sh || exit 1
+
+if [ $NOTMUCH_HAVE_PYTHON3_CFFI -eq 0 -o $NOTMUCH_HAVE_PYTHON3_PYTEST -eq 0 ]; then
+    test_done
+fi
+
+add_email_corpus
+
+cat <<EOF > recurse.py
+from notmuch2 import Database
+def show_msgs(msgs, level):
+    print('{:s} {:s}'.format(' ' * level*4, type(msgs).__name__))
+    for msg in msgs:
+        print('{:s} {:s}'.format(' ' * (level*4+2), type(msg).__name__))
+        replies=msg.replies()
+        show_msgs(replies, level+1)
+db = Database(config=Database.CONFIG.SEARCH)
+msg=db.find("87ocn0qh6d.fsf@yoom.home.cworth.org")
+threads = db.threads(query="thread:"+msg.threadid)
+thread = next (threads)
+show_msgs(thread, 0)
+EOF
+
+test_begin_subtest "recursive traversal of replies (no crash)"
+test_subtest_known_broken
+test_python < recurse.py
+error=$?
+test_expect_equal "${error}" 0
+
+test_begin_subtest "recursive traversal of replies (output)"
+test_subtest_known_broken
+test_python < recurse.py
+tail -n 10 < OUTPUT > OUTPUT.sample
+cat <<EOF > EXPECTED
+   OwnedMessage
+     MessageIter
+   OwnedMessage
+     MessageIter
+       OwnedMessage
+         MessageIter
+   OwnedMessage
+     MessageIter
+   OwnedMessage
+     MessageIter
+EOF
+test_expect_equal_file EXPECTED OUTPUT.sample
+
+test_done
-- 
2.34.1

[PATCH 2/2] python-cffi: returned OwnedMessage objects from Message.replies

[David Bremner]

If we return regular Message objects, python will try to destroy them,
and the underlying notmuch object, causing e.g. the crash [1].

[1]: id:87sfu6utxg.fsf@tethera.net
---
 bindings/python-cffi/notmuch2/_message.py | 4 ++--
 test/T392-python-cffi-notmuch.sh          | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/bindings/python-cffi/notmuch2/_message.py b/bindings/python-cffi/notmuch2/_message.py
index a460d8c1..aa1cb875 100644
--- a/bindings/python-cffi/notmuch2/_message.py
+++ b/bindings/python-cffi/notmuch2/_message.py
@@ -371,14 +371,14 @@ class Message(base.NotmuchObject):
         This method will only work if the message was created from a
         thread.  Otherwise it will yield no results.
 
-        :returns: An iterator yielding :class:`Message` instances.
+        :returns: An iterator yielding :class:`OwnedMessage` instances.
         :rtype: MessageIter
         """
         # The notmuch_messages_valid call accepts NULL and this will
         # become an empty iterator, raising StopIteration immediately.
         # Hence no return value checking here.
         msgs_p = capi.lib.notmuch_message_get_replies(self._msg_p)
-        return MessageIter(self, msgs_p, db=self._db)
+        return MessageIter(self, msgs_p, db=self._db, msg_cls=OwnedMessage)
 
     def __hash__(self):
         return hash(self.messageid)
diff --git a/test/T392-python-cffi-notmuch.sh b/test/T392-python-cffi-notmuch.sh
index 50012c55..15c8fc6b 100755
--- a/test/T392-python-cffi-notmuch.sh
+++ b/test/T392-python-cffi-notmuch.sh
@@ -24,13 +24,11 @@ show_msgs(thread, 0)
 EOF
 
 test_begin_subtest "recursive traversal of replies (no crash)"
-test_subtest_known_broken
 test_python < recurse.py
 error=$?
 test_expect_equal "${error}" 0
 
 test_begin_subtest "recursive traversal of replies (output)"
-test_subtest_known_broken
 test_python < recurse.py
 tail -n 10 < OUTPUT > OUTPUT.sample
 cat <<EOF > EXPECTED
-- 
2.34.1

Re: [PATCH 2/2] python-cffi: returned OwnedMessage objects from Message.replies

[Floris Bruynooghe]

On Sat 08 Jan 2022 at 10:03 -0400, David Bremner wrote:

> If we return regular Message objects, python will try to destroy them,
> and the underlying notmuch object, causing e.g. the crash [1].
>
> [1]: id:87sfu6utxg.fsf@tethera.net
> ---
>  bindings/python-cffi/notmuch2/_message.py | 4 ++--
>  test/T392-python-cffi-notmuch.sh          | 2 --
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/bindings/python-cffi/notmuch2/_message.py b/bindings/python-cffi/notmuch2/_message.py
> index a460d8c1..aa1cb875 100644
> --- a/bindings/python-cffi/notmuch2/_message.py
> +++ b/bindings/python-cffi/notmuch2/_message.py
> @@ -371,14 +371,14 @@ class Message(base.NotmuchObject):
>          This method will only work if the message was created from a
>          thread.  Otherwise it will yield no results.
>  
> -        :returns: An iterator yielding :class:`Message` instances.
> +        :returns: An iterator yielding :class:`OwnedMessage` instances.
>          :rtype: MessageIter
>          """
>          # The notmuch_messages_valid call accepts NULL and this will
>          # become an empty iterator, raising StopIteration immediately.
>          # Hence no return value checking here.
>          msgs_p = capi.lib.notmuch_message_get_replies(self._msg_p)
> -        return MessageIter(self, msgs_p, db=self._db)
> +        return MessageIter(self, msgs_p, db=self._db, msg_cls=OwnedMessage)
>  
>      def __hash__(self):
>          return hash(self.messageid)
> diff --git a/test/T392-python-cffi-notmuch.sh b/test/T392-python-cffi-notmuch.sh
> index 50012c55..15c8fc6b 100755
> --- a/test/T392-python-cffi-notmuch.sh
> +++ b/test/T392-python-cffi-notmuch.sh
> @@ -24,13 +24,11 @@ show_msgs(thread, 0)
>  EOF
>  
>  test_begin_subtest "recursive traversal of replies (no crash)"
> -test_subtest_known_broken
>  test_python < recurse.py
>  error=$?
>  test_expect_equal "${error}" 0
>  
>  test_begin_subtest "recursive traversal of replies (output)"
> -test_subtest_known_broken
>  test_python < recurse.py
>  tail -n 10 < OUTPUT > OUTPUT.sample
>  cat <<EOF > EXPECTED

oh, nice debugging!  And yes, this seems like the right fix, glad the
mechanism was at least already in place.

With respect to the docs, they seem clear enough.  Probably I missed
this or I simply didn't realise that the replies iter is basically a
thread owning it.

Only thing I'd do different is write a pytest test for this as well, but
than I wouldn't have written the notmuch tests. So I don't think this
matters that much.

Cheers,
Floris

Re: [PATCH 2/2] python-cffi: returned OwnedMessage objects from Message.replies

[David Bremner]

Floris Bruynooghe <flub@devork.be> writes:

> oh, nice debugging!  And yes, this seems like the right fix, glad the
> mechanism was at least already in place.
>
> With respect to the docs, they seem clear enough.  Probably I missed
> this or I simply didn't realise that the replies iter is basically a
> thread owning it.
>

Thanks for the review.

> Only thing I'd do different is write a pytest test for this as well, but
> than I wouldn't have written the notmuch tests. So I don't think this
> matters that much.

We can always add a pytest test later. One thing I would like to think
about is the length of time it takes to run the pytests. It is not
currently the bottleneck in running the parallel tests for me, but it is
among the slower T*.sh. So it might be nice at some point to split into
several invocations for pytest, assuming multiple invocations of pytest
can run in parallel.

d

Re: [PATCH 1/2] test: add known broken tests for recursive traversal of replies.

[David Bremner]

David Bremner <david@tethera.net> writes:

> This reproduces the bug reported at [1]. The second test hints at the
> solution, making reply return OwnedMessage objects.
>
> [1]: id:87sfu6utxg.fsf@tethera.net

Series applied to release and master.

d

Re: [PATCH 2/2] python-cffi: returned OwnedMessage objects from Message.replies

[Floris Bruynooghe]

On Sun 09 Jan 2022 at 09:26 -0400, David Bremner wrote:
> One thing I would like to think
> about is the length of time it takes to run the pytests. It is not
> currently the bottleneck in running the parallel tests for me, but it is
> among the slower T*.sh. So it might be nice at some point to split into
> several invocations for pytest, assuming multiple invocations of pytest
> can run in parallel.

There is the pytest-xdist plugin which is widely used and probably will
work fine on the notmuch testsuite (I even contributed to the plugin at
some point).  It will split the workload between however many processes
you choose, running them in parallel.

Re: [PATCH 2/2] python-cffi: returned OwnedMessage objects from Message.replies

[David Bremner]

Floris Bruynooghe <flub@devork.be> writes:

> On Sun 09 Jan 2022 at 09:26 -0400, David Bremner wrote:
>> One thing I would like to think
>> about is the length of time it takes to run the pytests. It is not
>> currently the bottleneck in running the parallel tests for me, but it is
>> among the slower T*.sh. So it might be nice at some point to split into
>> several invocations for pytest, assuming multiple invocations of pytest
>> can run in parallel.
>
> There is the pytest-xdist plugin which is widely used and probably will
> work fine on the notmuch testsuite (I even contributed to the plugin at
> some point).  It will split the workload between however many processes
> you choose, running them in parallel.

It sounds like it has potential, but I'm not sure how it would interact
with the existing scheme controlled by make. I suspect we'd end up with
too many processes/threads in total.

d