python-notmuch crash with threads

python-notmuch crash with threads

[James Westby]

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

Hi,

I've been seeing a race with python-notmuch, where it will crash due to
pointers being invalidated when threads are used.

I've attached a script which shows the problem some of the time. It's
about the smallest script I can make, but it's hampered by the fact that
making it simpler seems to make the race less likely, so it's hard to
know when it is gone.

The typical backtrace is:

Program terminated with signal 11, Segmentation fault.
#0  0x00007f7b19c34b59 in talloc_named_const () from /usr/lib/x86_64-linux-gnu/libtalloc.so.2
(gdb) up
#1  0x00007f7b1a5f78dc in notmuch_query_search_threads (query=0x14001c70) at lib/query.cc:322
322	lib/query.cc: No such file or directory.
	in lib/query.cc
(gdb) p *query
Cannot access memory at address 0x14001c70

Where something is invalidating the pointer between creation in
db.create_query() and calling it in query.search_threads()

I've seen other similar things when using other code in the thread.

http://talloc.samba.org/talloc/doc/html/index.html talks about the
thread-safety of talloc, and I don't think it's any of those issues
here.

Any suggestions for how to debug this further would be most welcome.

Thanks,

James


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

import threading


class NotmuchThread(threading.Thread):

    def __init__(self):
        super(NotmuchThread, self).__init__()
        self.job_waiting = threading.Condition()
        self.job_queue = []

    def run(self):
        from notmuch.database import Database
        self.db = Database()
        job = None
        print "Aquiring lock"
        with self.job_waiting:
            if len(self.job_queue) < 1:
                print "Job queue empty so waiting"
                while True:
                    ready = self.job_waiting.wait(1)
                    if ready:
                        break
                    if len(self.job_queue) > 0:
                        break
            print "Got job, releasing lock"
        self.search_threads("tag:inbox")

    def search_threads(self, query_string):
        query = self.db.create_query(query_string)
        print("%X" % query._query)
        threads = query.search_threads()
        return threads

test_thread = NotmuchThread()
test_thread.start()
with test_thread.job_waiting:
    test_thread.job_queue.append()
    test_thread.job_waiting.notify()
import time; time.sleep(1)
test_thread.join()

[PATCH] python: Store pointers as c_void_p to keep references

[James Westby]

From: James Westby <james.westby@linaro.org>

ctypes doesn't return c_void_p return values as that, it returns them as
32-bit integers instead. This has two problems:

  1 - On 64-bit machines anything higher than the max 32-bit integer
      will overflow when passed back in to another function expecting,
      a pointer giving the wrong value.

  2 - If the value isn't stored as a pointer then the memory can be
      re-used and so the object will be corrupted.

The fix for both of these is to store the values as pointers.
---
 bindings/python/notmuch/database.py |   18 +++++++++---------
 bindings/python/notmuch/message.py  |   10 +++++-----
 bindings/python/notmuch/thread.py   |    6 +++---
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index f4bc53e..e5188fb 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -152,7 +152,7 @@ class Database(object):
         if res is None:
             raise NotmuchError(
                 message="Could not create the specified database")
-        self._db = res
+        self._db = c_void_p(res)
 
     def open(self, path, mode=0):
         """Opens an existing database
@@ -171,7 +171,7 @@ class Database(object):
 
         if res is None:
             raise NotmuchError(message="Could not open the specified database")
-        self._db = res
+        self._db = c_void_p(res)
 
     def get_path(self):
         """Returns the file path of an open database"""
@@ -297,7 +297,7 @@ class Database(object):
         dir_p = Database._get_directory(self._db, _str(path))
 
         # return the Directory, init it with the absolute path
-        return Directory(_str(abs_dirpath), dir_p, self)
+        return Directory(_str(abs_dirpath), c_void_p(dir_p), self)
 
     def add_message(self, filename, sync_maildir_flags=False):
         """Adds a new message to the database
@@ -466,7 +466,7 @@ class Database(object):
         tags_p = Database._get_all_tags(self._db)
         if tags_p == None:
             raise NotmuchError(STATUS.NULL_POINTER)
-        return Tags(tags_p, self)
+        return Tags(c_void_p(tags_p), self)
 
     def create_query(self, querystring):
         """Returns a :class:`Query` derived from this database
@@ -600,7 +600,7 @@ class Query(object):
         query_p = Query._create(db.db_p, _str(querystr))
         if query_p is None:
             raise NullPointerError
-        self._query = query_p
+        self._query = c_void_p(query_p)
 
     def set_sort(self, sort):
         """Set the sort order future results will be delivered in
@@ -630,7 +630,7 @@ class Query(object):
 
         if threads_p is None:
             raise NullPointerError
-        return Threads(threads_p, self)
+        return Threads(c_void_p(threads_p), self)
 
     def search_messages(self):
         """Filter messages according to the query and return
@@ -644,7 +644,7 @@ class Query(object):
 
         if msgs_p is None:
             raise NullPointerError
-        return Messages(msgs_p, self)
+        return Messages(c_void_p(msgs_p), self)
 
     def count_messages(self):
         """Estimate the number of messages matching the query
@@ -793,7 +793,7 @@ class Directory(object):
         """
         self._assert_dir_is_initialized()
         files_p = Directory._get_child_files(self._dir_p)
-        return Filenames(files_p, self)
+        return Filenames(c_void_p(files_p), self)
 
     def get_child_directories(self):
         """Gets a :class:`Filenames` iterator listing all the filenames of
@@ -804,7 +804,7 @@ class Directory(object):
         """
         self._assert_dir_is_initialized()
         files_p = Directory._get_child_directories(self._dir_p)
-        return Filenames(files_p, self)
+        return Filenames(c_void_p(files_p), self)
 
     @property
     def path(self):
diff --git a/bindings/python/notmuch/message.py b/bindings/python/notmuch/message.py
index 4bf90c2..672a169 100644
--- a/bindings/python/notmuch/message.py
+++ b/bindings/python/notmuch/message.py
@@ -140,7 +140,7 @@ class Messages(object):
 
         if tags_p == None:
             raise NotmuchError(STATUS.NULL_POINTER)
-        return Tags(tags_p, self)
+        return Tags(c_void_p(tags_p), self)
 
     def __iter__(self):
         """ Make Messages an iterator """
@@ -154,7 +154,7 @@ class Messages(object):
             self._msgs = None
             raise StopIteration
 
-        msg = Message(Messages._get(self._msgs), self)
+        msg = Message(c_void_p(Messages._get(self._msgs)), self)
         nmlib.notmuch_messages_move_to_next(self._msgs)
         return msg
 
@@ -350,7 +350,7 @@ class Message(object):
         if msgs_p is None:
             return None
 
-        return Messages(msgs_p, self)
+        return Messages(c_void_p(msgs_p), self)
 
     def get_date(self):
         """Returns time_t of the message date
@@ -418,7 +418,7 @@ class Message(object):
 
         files_p = Message._get_filenames(self._msg)
 
-        return Filenames(files_p, self).as_generator()
+        return Filenames(c_void_p(files_p), self).as_generator()
 
     def get_flag(self, flag):
         """Checks whether a specific flag is set for this message
@@ -468,7 +468,7 @@ class Message(object):
         tags_p = Message._get_tags(self._msg)
         if tags_p == None:
             raise NotmuchError(STATUS.NULL_POINTER)
-        return Tags(tags_p, self)
+        return Tags(c_void_p(tags_p), self)
 
     def add_tag(self, tag, sync_maildir_flags=False):
         """Adds a tag to the given message
diff --git a/bindings/python/notmuch/thread.py b/bindings/python/notmuch/thread.py
index 5e08eb3..ee4b71d 100644
--- a/bindings/python/notmuch/thread.py
+++ b/bindings/python/notmuch/thread.py
@@ -113,7 +113,7 @@ class Threads(object):
             self._threads = None
             raise StopIteration
 
-        thread = Thread(Threads._get(self._threads), self)
+        thread = Thread(c_void_p(Threads._get(self._threads)), self)
         nmlib.notmuch_threads_move_to_next(self._threads)
         return thread
 
@@ -265,7 +265,7 @@ class Thread(object):
         if msgs_p is None:
             raise NotmuchError(STATUS.NULL_POINTER)
 
-        return Messages(msgs_p, self)
+        return Messages(c_void_p(msgs_p), self)
 
     def get_matched_messages(self):
         """Returns the number of messages in 'thread' that matched the query
@@ -359,7 +359,7 @@ class Thread(object):
         tags_p = Thread._get_tags(self._thread)
         if tags_p == None:
             raise NotmuchError(STATUS.NULL_POINTER)
-        return Tags(tags_p, self)
+        return Tags(c_void_p(tags_p), self)
 
     def __str__(self):
         """A str(Thread()) is represented by a 1-line summary"""
-- 
1.7.5.4

Re: [PATCH] python: Store pointers as c_void_p to keep references

[James Westby]

On Thu,  1 Dec 2011 18:13:05 -0500, James Westby <jw+debian@jameswestby.net> wrote:
> From: James Westby <james.westby@linaro.org>
> 
> ctypes doesn't return c_void_p return values as that, it returns them as
> 32-bit integers instead. This has two problems:
> 
>   1 - On 64-bit machines anything higher than the max 32-bit integer
>       will overflow when passed back in to another function expecting,
>       a pointer giving the wrong value.
> 
>   2 - If the value isn't stored as a pointer then the memory can be
>       re-used and so the object will be corrupted.
> 
> The fix for both of these is to store the values as pointers.

http://osdir.com/ml/python.ctypes/2006-12/msg00048.html states that this
is expected behaviour of ctypes.

Thanks,

James