Segmentation fault at gmime-iconv with python binding

classic Classic list List threaded Threaded
28 messages Options
12
Kazuo Teramoto Kazuo Teramoto
Reply | Threaded
Open this post in threaded view
|

Segmentation fault at gmime-iconv with python binding

Hi!

When I try to run the attached test.py after adding the attached email
(4EFC743A.3060609_april.org) to notmuch db I got a segmentation fault
(gdb bt attached).

This is what I think a relevant part of it:
~~~~~~~~
(gdb) frame 1
#1  0x00007ffff5f2759c in g_mime_iconv_open (to=0x761ef0 "UTF-8",
from=0x83d590 "iso-8859-1") at gmime-iconv.c:261
261 if ((node = (IconvCacheNode *) cache_node_lookup (iconv_cache,
key, TRUE))) {
(gdb) list
256 key = g_alloca (strlen (from) + strlen (to) + 2);
257 sprintf (key, "%s:%s", from, to);
258
259 ICONV_CACHE_LOCK ();
260
261 if ((node = (IconvCacheNode *) cache_node_lookup (iconv_cache,
key, TRUE))) {
262 if (node->used) {
263 if ((cd = iconv_open (to, from)) == (iconv_t) -1)
264 goto exception;
265 } else {
(gdb) print iconv_cache
$1 = (Cache *) 0x0
(gdb)
~~~~~~~~

iconv_cache is initialized in g_mime_iconv_init() that is called by
g_mime_init().

notmuch CLI show the message correct. I know nothing about gmime or
notmuch code, but can this be the case of the python bindings not
calling g_mime_init() correctly?

Regards,
Kazuo Teramoto

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

test.py (278 bytes) Download Attachment
4EFC743A.3060609_april.org (1K) Download Attachment
notmuch_py_gmime.gdb_bt (5K) Download Attachment
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: Segmentation fault at gmime-iconv with python binding

On Thu, 29 Dec 2011 22:57:27 -0200, Kazuo Teramoto <[hidden email]> wrote:

> notmuch CLI show the message correct. I know nothing about gmime or
> notmuch code, but can this be the case of the python bindings not
> calling g_mime_init() correctly?

That seems likely. We worked around a similar problem by calling
g_type_init in notmuch_database_open. I'm not sure if it is permissible
to call g_mime_init more than once. The g_mime docs are, uhh, concise.

d


_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Kazuo Teramoto Kazuo Teramoto
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/2] Multiples calls of g_mime_init

The gmime docs don't says if is ok to call g_mime_init multiple times,
but the code have a check for it in a form like this:
~~~~~~~~
static unsigned int initialized = 0;
g_mime_init (guint32 flags)
{
    if (initialized++)
        return;
~~~~~~~~
so the init code is run only once and notmuch don't need to explicit
check for an already initialized gmime.

This make possible to call g_mime_init again in lib/database.cc and this
call really solve the OP segmentation fault in python bindings.

Kazuo Teramoto (2):
  lib: Remove unnecessary checks when calling g_mime_init
  lib: call g_mime_init from notmuch_database_open

 lib/database.cc    |    5 +++++
 lib/index.cc       |    4 ----
 lib/message-file.c |    4 ----
 3 files changed, 5 insertions(+), 8 deletions(-)

--
1.7.8.1

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Kazuo Teramoto Kazuo Teramoto
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] lib: Remove unnecessary checks when calling g_mime_init

g_mime_init already check for multiple initializations.
---
 lib/index.cc       |    4 ----
 lib/message-file.c |    4 ----
 2 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/lib/index.cc b/lib/index.cc
index d8f8b2b..6764929 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -419,12 +419,8 @@ _notmuch_message_index_file (notmuch_message_t *message,
     FILE *file = NULL;
     const char *from, *subject;
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
-    static int initialized = 0;
 
-    if (! initialized) {
  g_mime_init (0);
- initialized = 1;
-    }
 
     file = fopen (filename, "r");
     if (! file) {
diff --git a/lib/message-file.c b/lib/message-file.c
index 915aba8..78c7820 100644
--- a/lib/message-file.c
+++ b/lib/message-file.c
@@ -223,14 +223,10 @@ notmuch_message_file_get_header (notmuch_message_file_t *message,
     char *header, *decoded_value, *header_sofar, *combined_header;
     const char *s, *colon;
     int match, newhdr, hdrsofar, is_received;
-    static int initialized = 0;
 
     is_received = (strcmp(header_desired,"received") == 0);
 
-    if (! initialized) {
  g_mime_init (0);
- initialized = 1;
-    }
 
     message->parsing_started = 1;
 
--
1.7.8.1

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Kazuo Teramoto Kazuo Teramoto
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] lib: call g_mime_init from notmuch_database_open

In reply to this post by Kazuo Teramoto
We need to call g_mime_init to correct initialize the structures needed
by gmime before using it.
---
 lib/database.cc |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index d11dfaf..07ca3fd 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -28,6 +28,8 @@
 #include <glib.h> /* g_free, GPtrArray, GHashTable */
 #include <glib-object.h> /* g_type_init */
 
+#include <gmime/gmime.h> /* g_mime_init */
+
 using namespace std;
 
 #define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))
@@ -608,6 +610,9 @@ notmuch_database_open (const char *path,
     /* Initialize the GLib type system and threads */
     g_type_init ();
 
+    /* Initialize gmime */
+    g_mime_init (0);
+
     notmuch = talloc (NULL, notmuch_database_t);
     notmuch->exception_reported = FALSE;
     notmuch->path = talloc_strdup (notmuch, path);
--
1.7.8.1

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Patrick Totzke Patrick Totzke
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] Multiples calls of g_mime_init

In reply to this post by Kazuo Teramoto
hi!

while I cannot confirm that this patch solves the stated issue (cannot reproduce),
It seeminly doesn't break things either and I'm all in favour of doing things right™.

On a related note: It also doesn't fix the recent segfaults of the bindings
that occur when you try to write to a locked database.
cf "id:[hidden email]"

hth,
/p
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Kazuo Teramoto Kazuo Teramoto
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] Multiples calls of g_mime_init

On 2011-12-30T23:16:47, Patrick Totzke wrote:

>It seeminly doesn't break things either and I'm all in favour of doing
>things right™.

And talking about doing things right, we probably need to call
g_mime_shutdown() too, one for every time we call g_mime_init(). I will
send a patch after I do some more tests.

>On a related note: It also doesn't fix the recent segfaults of the bindings
>that occur when you try to write to a locked database.
>cf "id:[hidden email]"
>

This still hit me. If I remember right when I did a test what is
happening is that the binding is trying to close the db, so we need a
check if the db is opened before trying to close it.
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] lib: Remove unnecessary checks when calling g_mime_init

In reply to this post by Kazuo Teramoto
On Fri, 30 Dec 2011 19:58:09 -0200, Kazuo Teramoto <[hidden email]> wrote:
> g_mime_init already check for multiple initializations.
> ---
>  lib/index.cc       |    4 ----
>  lib/message-file.c |    4 ----
>  2 files changed, 0 insertions(+), 8 deletions(-)

Is this needed to fix the bug? It seems less futureproof to rely on
gmime to check for double initialization if we don't have to.

d
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] lib: call g_mime_init from notmuch_database_open

In reply to this post by Kazuo Teramoto
On Fri, 30 Dec 2011 19:58:10 -0200, Kazuo Teramoto <[hidden email]> wrote:
> We need to call g_mime_init to correct initialize the structures needed
> by gmime before using it.
> ---
>  lib/database.cc |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)

I can confirm this patch (alone) fixes the segfault for me. and doesn't
cause in test failures when applied against the release branch.  I'm
considering pushing it into the 0.11 release. Any comments on that?

d
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Kazuo Teramoto Kazuo Teramoto
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] lib: Remove unnecessary checks when calling g_mime_init

In reply to this post by David Bremner-2
On 2011-12-31T00:55:59, David Bremner wrote:

>On Fri, 30 Dec 2011 19:58:09 -0200, Kazuo Teramoto <[hidden email]> wrote:
>> g_mime_init already check for multiple initializations.
>> ---
>>  lib/index.cc       |    4 ----
>>  lib/message-file.c |    4 ----
>>  2 files changed, 0 insertions(+), 8 deletions(-)
>
>Is this needed to fix the bug? It seems less futureproof to rely on
>gmime to check for double initialization if we don't have to.
>

No its not. But if you decide to push only the other part we probably
need to add the initialization check to it, for consistency. I can send
another patch, would you like it?
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Kazuo Teramoto Kazuo Teramoto
Reply | Threaded
Open this post in threaded view
|

[PATCH] lib: call g_mime_init() from notmuch_database_open()

In reply to this post by David Bremner-2
I'm resending the PATCH 2/2 as a standalone, leaving the matter of
removing the checks for the future. I added a more detailed commit too.

This is the only patch needed to fix the segfault I reported.

Kazuo Teramoto (1):
  lib: call g_mime_init() from notmuch_database_open()

 lib/database.cc |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

--
1.7.8.1

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Kazuo Teramoto Kazuo Teramoto
Reply | Threaded
Open this post in threaded view
|

[PATCH] lib: call g_mime_init() from notmuch_database_open()

As reported in
id:"[hidden email]"
sometimes gmime try to access a NULL pointer, e.g. g_mime_iconv_open()
try to access iconv_cache that is NULL if g_mime_init() is not called.
This cause notmuch to segfault when calling gmime functions.

Calling g_mime_init() initialize iconv_cache and others variables needed
by gmime, making sure they are initialized when notmuch calls gmime
functions.
---
 lib/database.cc |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index d11dfaf..8103bd9 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -28,6 +28,8 @@
 #include <glib.h> /* g_free, GPtrArray, GHashTable */
 #include <glib-object.h> /* g_type_init */
 
+#include <gmime/gmime.h> /* g_mime_init */
+
 using namespace std;
 
 #define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))
@@ -585,6 +587,7 @@ notmuch_database_open (const char *path,
     struct stat st;
     int err;
     unsigned int i, version;
+    static int initialized = 0;
 
     if (asprintf (&notmuch_path, "%s/%s", path, ".notmuch") == -1) {
  notmuch_path = NULL;
@@ -608,6 +611,12 @@ notmuch_database_open (const char *path,
     /* Initialize the GLib type system and threads */
     g_type_init ();
 
+    /* Initialize gmime */
+    if (! initialized) {
+ g_mime_init (0);
+ initialized = 1;
+    }
+
     notmuch = talloc (NULL, notmuch_database_t);
     notmuch->exception_reported = FALSE;
     notmuch->path = talloc_strdup (notmuch, path);
--
1.7.8.1

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Austin Clements Austin Clements
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] lib: Remove unnecessary checks when calling g_mime_init

In reply to this post by Kazuo Teramoto
Shouldn't we remove the g_mime_init's from this code entirely if we're
going to do it in notmuch_database_open?

Quoth Kazuo Teramoto on Dec 30 at  7:58 pm:

> g_mime_init already check for multiple initializations.
> ---
>  lib/index.cc       |    4 ----
>  lib/message-file.c |    4 ----
>  2 files changed, 0 insertions(+), 8 deletions(-)
>
> diff --git a/lib/index.cc b/lib/index.cc
> index d8f8b2b..6764929 100644
> --- a/lib/index.cc
> +++ b/lib/index.cc
> @@ -419,12 +419,8 @@ _notmuch_message_index_file (notmuch_message_t *message,
>      FILE *file = NULL;
>      const char *from, *subject;
>      notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
> -    static int initialized = 0;
>  
> -    if (! initialized) {
>   g_mime_init (0);
> - initialized = 1;
> -    }
>  
>      file = fopen (filename, "r");
>      if (! file) {
> diff --git a/lib/message-file.c b/lib/message-file.c
> index 915aba8..78c7820 100644
> --- a/lib/message-file.c
> +++ b/lib/message-file.c
> @@ -223,14 +223,10 @@ notmuch_message_file_get_header (notmuch_message_file_t *message,
>      char *header, *decoded_value, *header_sofar, *combined_header;
>      const char *s, *colon;
>      int match, newhdr, hdrsofar, is_received;
> -    static int initialized = 0;
>  
>      is_received = (strcmp(header_desired,"received") == 0);
>  
> -    if (! initialized) {
>   g_mime_init (0);
> - initialized = 1;
> -    }
>  
>      message->parsing_started = 1;
>  
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

revised patch for gmime init, with test.

In reply to this post by Kazuo Teramoto
It turns out that our existing (trivial) python test is enough to
catch this bug, but the corpus needs to be augmented.  This
augmentation is a bit intrusive so I'm thinking of cherry-picking only
the actual fix to the release branch.

Unfortunately the test message is 8 bit, so it may be encoded in some
inconvenient way for patch application. The message is attached to an
earlier message in the thread if you want to double check.

I also wondered about putting g_type_init inside the (!initialized)
test, but decided against it on the grounds of minimality.

I think we want to in the medium term factor out all of the
initialization code into one (probably private) function; we can clean
things up a bit more then.

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 1/3] test: use file based comparison for search '*' test

From: David Bremner <[hidden email]>

This seems a bit easier to maintain, and is more accurate since lines
are not joined together.
---
 test/search |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/test/search b/test/search
index e7c8c54..4710d57 100755
--- a/test/search
+++ b/test/search
@@ -79,8 +79,9 @@ output=$(notmuch search 'subject:"subject search test (phrase)"' | notmuch_searc
 test_expect_equal "$output" "thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; subject search test (phrase) (inbox unread)"
 
 test_begin_subtest 'Search for all messages ("*")'
-output=$(notmuch search '*' | notmuch_search_sanitize)
-test_expect_equal "$output" "thread:XXX   2009-11-18 [1/1] Chris Wilson; [notmuch] [PATCH 1/2] Makefile: evaluate pkg-config once (inbox unread)
+notmuch search '*' | notmuch_search_sanitize > OUTPUT
+cat <<EOF >EXPECTED
+thread:XXX   2009-11-18 [1/1] Chris Wilson; [notmuch] [PATCH 1/2] Makefile: evaluate pkg-config once (inbox unread)
 thread:XXX   2009-11-18 [2/2] Alex Botero-Lowry, Carl Worth; [notmuch] [PATCH] Error out if no query is supplied to search instead of going into an infinite loop (attachment inbox unread)
 thread:XXX   2009-11-18 [2/2] Ingmar Vanhassel, Carl Worth; [notmuch] [PATCH] Typsos (inbox unread)
 thread:XXX   2009-11-18 [3/3] Adrian Perez de Castro, Keith Packard, Carl Worth; [notmuch] Introducing myself (inbox signed unread)
@@ -99,7 +100,7 @@ thread:XXX   2009-11-18 [1/1] Jan Janak; [notmuch] [PATCH] notmuch new: Support
 thread:XXX   2009-11-18 [1/1] Stewart Smith; [notmuch] [PATCH] count_files: sort directory in inode order before statting (inbox unread)
 thread:XXX   2009-11-18 [1/1] Stewart Smith; [notmuch] [PATCH 2/2] Read mail directory in inode number order (inbox unread)
 thread:XXX   2009-11-18 [1/1] Stewart Smith; [notmuch] [PATCH] Fix linking with gcc to use g++ to link in C++ libs. (inbox unread)
-thread:XXX   2009-11-18 [2/2] Lars Kellogg-Stedman; [notmuch] \"notmuch help\" outputs to stderr? (attachment inbox signed unread)
+thread:XXX   2009-11-18 [2/2] Lars Kellogg-Stedman; [notmuch] "notmuch help" outputs to stderr? (attachment inbox signed unread)
 thread:XXX   2009-11-17 [1/1] Mikhail Gusarov; [notmuch] [PATCH] Handle rename of message file (inbox unread)
 thread:XXX   2009-11-17 [2/2] Alex Botero-Lowry, Carl Worth; [notmuch] preliminary FreeBSD support (attachment inbox unread)
 thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; body search (inbox unread)
@@ -117,7 +118,9 @@ thread:XXX   2000-01-01 [1/1] Search By From Name; search by from (name) (inbox
 thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; search by to (address) (inbox unread)
 thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; search by to (name) (inbox unread)
 thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; subject search test (phrase) (inbox unread)
-thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; this phrase should not match the subject search test (inbox unread)"
+thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; this phrase should not match the subject search test (inbox unread)
+EOF
+test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "Search body (utf-8):"
 add_message '[subject]="utf8-message-body-subject"' '[date]="Sat, 01 Jan 2000 12:00:00 -0000"' '[body]="message body utf8: bödý"'
--
1.7.7.3

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 2/3] test: add two new messages to corpus with iso-8859-1 encoding

In reply to this post by David Bremner-2
From: David Bremner <[hidden email]>

One is quoted printable, the other users 8 bit encoding. The latter
triggers a bug in the python bindings due to missing call to
g_mime_init. The corresponding test is marked broken in this commit.
---
 test/corpus/cur/52:2,                              |   39 ++++++++++++++++++++
 test/corpus/cur/53:2,                              |   20 ++++++++++
 test/emacs.expected-output/notmuch-hello           |    4 +-
 .../notmuch-hello-no-saved-searches                |    2 +-
 .../emacs.expected-output/notmuch-hello-view-inbox |    2 +
 .../emacs.expected-output/notmuch-hello-with-empty |    4 +-
 .../emacs.expected-output/notmuch-search-tag-inbox |    2 +
 test/python                                        |    1 +
 test/search                                        |    2 +
 test/search-output                                 |   16 +++++++-
 10 files changed, 85 insertions(+), 7 deletions(-)
 create mode 100644 test/corpus/cur/52:2,
 create mode 100644 test/corpus/cur/53:2,

diff --git a/test/corpus/cur/52:2, b/test/corpus/cur/52:2,
new file mode 100644
index 0000000..6028340
--- /dev/null
+++ b/test/corpus/cur/52:2,
@@ -0,0 +1,39 @@
+Message-ID: <[hidden email]>
+Date: Thu, 29 Dec 2010 15:07:54 +0100
+From: "=?ISO-8859-1?Q?Fran=E7ois_Boulogne?=" <[hidden email]>
+User-Agent: Mozilla/5.0 (X11; Linux i686;
+ rv:9.0) Gecko/20111224 Thunderbird/9.0.1
+MIME-Version: 1.0
+To: Allan McRae <[hidden email]>,
+ "Discussion about the Arch User Repository (AUR)" <[hidden email]>
+References: <[hidden email]> <[hidden email]>
+In-Reply-To: <[hidden email]>
+Content-Type: text/plain; charset=ISO-8859-1
+Content-Transfer-Encoding: 8bit
+Subject: Re: [aur-general] Guidelines: cp, mkdir vs install
+
+Le 29/12/2011 11:13, Allan McRae a �crit :
+> On 29/12/11 19:56, Fran�ois Boulogne wrote:
+>> Hi,
+>>
+>> Looking to improve the quality of my packages, I read again the guidelines.
+>> https://wiki.archlinux.org/index.php/Arch_Packaging_Standards
+>>
+>> However, it don't see anything about the install command like
+>> install -d $pkgdir/usr/{bin,share/man/man1,share/locale}
+>>
+>> Some contributors on AUR use cp or mkdir to install files/dir (when no
+>> makefile is provided) and others use install command.
+>>
+>> What's the opinion of TU on this point?
+>>
+>
+> Use install with -m specifying the correct permissions
+>
+
+Thank you Allan
+
+
+--
+Fran�ois Boulogne.
+https://www.sciunto.org
diff --git a/test/corpus/cur/53:2, b/test/corpus/cur/53:2,
new file mode 100644
index 0000000..7a1e2e5
--- /dev/null
+++ b/test/corpus/cur/53:2,
@@ -0,0 +1,20 @@
+From: Olivier Berger <[hidden email]>
+To: [hidden email]
+Subject: Essai =?iso-8859-1?Q?accentu=E9?=
+User-Agent: Notmuch/0.10.1 (http://notmuchmail.org) Emacs/23.3.1 (i486-pc-linux-gnu)
+X-Draft-From: ("nnimap+localdovecot:INBOX" 44228)
+Date: Fri, 16 Dec 2010 16:49:59 +0100
+Message-ID: <[hidden email]>
+MIME-Version: 1.0
+Content-Type: text/plain; charset=iso-8859-1
+Content-Transfer-Encoding: quoted-printable
+
+Du texte accentu=E9 pour =E7a ...
+
+=E0 la bonne heure !
+--=20
+Olivier BERGER=20
+http://www-public.it-sudparis.eu/~berger_o/ - OpenPGP-Id: 2048R/5819D7E8
+Ingenieur Recherche - Dept INF
+Institut TELECOM, SudParis (http://www.it-sudparis.eu/), Evry (France)
+
diff --git a/test/emacs.expected-output/notmuch-hello b/test/emacs.expected-output/notmuch-hello
index 48143bd..de57de2 100644
--- a/test/emacs.expected-output/notmuch-hello
+++ b/test/emacs.expected-output/notmuch-hello
@@ -1,8 +1,8 @@
-   Welcome to notmuch. You have 50 messages.
+   Welcome to notmuch. You have 52 messages.
 
 Saved searches: [edit]
 
-  50 inbox           50 unread    
+  52 inbox           52 unread    
 
 Search:                                                                      
 
diff --git a/test/emacs.expected-output/notmuch-hello-no-saved-searches b/test/emacs.expected-output/notmuch-hello-no-saved-searches
index 7c09e40..f1fc4d6 100644
--- a/test/emacs.expected-output/notmuch-hello-no-saved-searches
+++ b/test/emacs.expected-output/notmuch-hello-no-saved-searches
@@ -1,4 +1,4 @@
-   Welcome to notmuch. You have 50 messages.
+   Welcome to notmuch. You have 52 messages.
 
 Search:                                                                      
 
diff --git a/test/emacs.expected-output/notmuch-hello-view-inbox b/test/emacs.expected-output/notmuch-hello-view-inbox
index 894ae5f..1688d67 100644
--- a/test/emacs.expected-output/notmuch-hello-view-inbox
+++ b/test/emacs.expected-output/notmuch-hello-view-inbox
@@ -20,4 +20,6 @@
   2009-11-18 [1/1]   Alexander Botero-Lowry  [notmuch] request for pull (inbox unread)
   2009-11-18 [2/2]   Keith Packard, Alexander Botero-Lowry    [notmuch] [PATCH] Create a default notmuch-show-hook that highlights URLs and uses word-wrap (inbox unread)
   2009-11-18 [1/1]   Chris Wilson         [notmuch] [PATCH 1/2] Makefile: evaluate pkg-config once (inbox unread)
+  2010-12-16 [1/1]   Olivier Berger       Essai accentué (inbox unread)
+  2010-12-29 [1/1]   François Boulogne    [aur-general] Guidelines: cp, mkdir vs install (inbox unread)
 End of search results.
diff --git a/test/emacs.expected-output/notmuch-hello-with-empty b/test/emacs.expected-output/notmuch-hello-with-empty
index 2a267c9..dd8728b 100644
--- a/test/emacs.expected-output/notmuch-hello-with-empty
+++ b/test/emacs.expected-output/notmuch-hello-with-empty
@@ -1,8 +1,8 @@
-   Welcome to notmuch. You have 50 messages.
+   Welcome to notmuch. You have 52 messages.
 
 Saved searches: [edit]
 
-  50 inbox           50 unread           0 empty    
+  52 inbox           52 unread           0 empty    
 
 Search:                                                                      
 
diff --git a/test/emacs.expected-output/notmuch-search-tag-inbox b/test/emacs.expected-output/notmuch-search-tag-inbox
index 9456ccf..8a53555 100644
--- a/test/emacs.expected-output/notmuch-search-tag-inbox
+++ b/test/emacs.expected-output/notmuch-search-tag-inbox
@@ -1,3 +1,5 @@
+  2010-12-29 [1/1]   François Boulogne    [aur-general] Guidelines: cp, mkdir vs install (inbox unread)
+  2010-12-16 [1/1]   Olivier Berger       Essai accentué (inbox unread)
   2009-11-18 [1/1]   Chris Wilson         [notmuch] [PATCH 1/2] Makefile: evaluate pkg-config once (inbox unread)
   2009-11-18 [2/2]   Alex Botero-Lowry, Carl Worth  [notmuch] [PATCH] Error out if no query is supplied to search instead of going into an infinite loop (attachment inbox unread)
   2009-11-18 [2/2]   Ingmar Vanhassel, Carl Worth  [notmuch] [PATCH] Typsos (inbox unread)
diff --git a/test/python b/test/python
index c3aa726..0b56db3 100755
--- a/test/python
+++ b/test/python
@@ -5,6 +5,7 @@ test_description="python bindings"
 add_email_corpus
 
 test_begin_subtest "compare thread ids"
+test_subtest_known_broken
 test_python <<EOF
 import notmuch
 db = notmuch.Database(mode=notmuch.Database.MODE.READ_WRITE)
diff --git a/test/search b/test/search
index 4710d57..a7a0b18 100755
--- a/test/search
+++ b/test/search
@@ -81,6 +81,8 @@ test_expect_equal "$output" "thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; s
 test_begin_subtest 'Search for all messages ("*")'
 notmuch search '*' | notmuch_search_sanitize > OUTPUT
 cat <<EOF >EXPECTED
+thread:XXX   2010-12-29 [1/1] François Boulogne; [aur-general] Guidelines: cp, mkdir vs install (inbox unread)
+thread:XXX   2010-12-16 [1/1] Olivier Berger; Essai accentué (inbox unread)
 thread:XXX   2009-11-18 [1/1] Chris Wilson; [notmuch] [PATCH 1/2] Makefile: evaluate pkg-config once (inbox unread)
 thread:XXX   2009-11-18 [2/2] Alex Botero-Lowry, Carl Worth; [notmuch] [PATCH] Error out if no query is supplied to search instead of going into an infinite loop (attachment inbox unread)
 thread:XXX   2009-11-18 [2/2] Ingmar Vanhassel, Carl Worth; [notmuch] [PATCH] Typsos (inbox unread)
diff --git a/test/search-output b/test/search-output
index 10291c3..8b57a43 100755
--- a/test/search-output
+++ b/test/search-output
@@ -29,6 +29,8 @@ thread:THREADID
 thread:THREADID
 thread:THREADID
 thread:THREADID
+thread:THREADID
+thread:THREADID
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
@@ -56,6 +58,8 @@ cat <<EOF >EXPECTED
 "THREADID",
 "THREADID",
 "THREADID",
+"THREADID",
+"THREADID",
 "THREADID"]
 EOF
 test_expect_equal_file OUTPUT EXPECTED
@@ -63,6 +67,8 @@ test_expect_equal_file OUTPUT EXPECTED
 test_begin_subtest "--output=messages"
 notmuch search --output=messages '*' >OUTPUT
 cat <<EOF >EXPECTED
+id:[hidden email]
+id:[hidden email]
 id:[hidden email]
 id:[hidden email]
 id:[hidden email]
@@ -119,7 +125,9 @@ test_expect_equal_file OUTPUT EXPECTED
 test_begin_subtest "--output=messages --format=json"
 notmuch search --format=json --output=messages '*' >OUTPUT
 cat <<EOF >EXPECTED
-["[hidden email]",
+["[hidden email]",
+"[hidden email]",
+"[hidden email]",
 "[hidden email]",
 "[hidden email]",
 "[hidden email]",
@@ -175,6 +183,8 @@ test_expect_equal_file OUTPUT EXPECTED
 test_begin_subtest "--output=files"
 notmuch search --output=files '*' | sed -e "s,$MAIL_DIR,MAIL_DIR," >OUTPUT
 cat <<EOF >EXPECTED
+MAIL_DIR/cur/52:2,
+MAIL_DIR/cur/53:2,
 MAIL_DIR/cur/50:2,
 MAIL_DIR/cur/49:2,
 MAIL_DIR/cur/48:2,
@@ -232,7 +242,9 @@ test_expect_equal_file OUTPUT EXPECTED
 test_begin_subtest "--output=files --format=json"
 notmuch search --format=json --output=files '*' | sed -e "s,$MAIL_DIR,MAIL_DIR," >OUTPUT
 cat <<EOF >EXPECTED
-["MAIL_DIR/cur/50:2,",
+["MAIL_DIR/cur/52:2,",
+"MAIL_DIR/cur/53:2,",
+"MAIL_DIR/cur/50:2,",
 "MAIL_DIR/cur/49:2,",
 "MAIL_DIR/cur/48:2,",
 "MAIL_DIR/cur/47:2,",
--
1.7.7.3


_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 3/3] lib: call g_mime_init() from notmuch_database_open()

In reply to this post by David Bremner-2
From: Kazuo Teramoto <[hidden email]>

As reported in
id:"[hidden email]"
sometimes gmime tries to access a NULL pointer, e.g. g_mime_iconv_open()
tries to access iconv_cache that is NULL if g_mime_init() is not called.
This causes notmuch to segfault when calling gmime functions.

Calling g_mime_init() initializes iconv_cache and others variables needed
by gmime, making sure they are initialized when notmuch calls gmime
functions.

Test marked fix by db.
---
 lib/database.cc |    9 +++++++++
 test/python     |    1 -
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index d11dfaf..8103bd9 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -28,6 +28,8 @@
 #include <glib.h> /* g_free, GPtrArray, GHashTable */
 #include <glib-object.h> /* g_type_init */
 
+#include <gmime/gmime.h> /* g_mime_init */
+
 using namespace std;
 
 #define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))
@@ -585,6 +587,7 @@ notmuch_database_open (const char *path,
     struct stat st;
     int err;
     unsigned int i, version;
+    static int initialized = 0;
 
     if (asprintf (&notmuch_path, "%s/%s", path, ".notmuch") == -1) {
  notmuch_path = NULL;
@@ -608,6 +611,12 @@ notmuch_database_open (const char *path,
     /* Initialize the GLib type system and threads */
     g_type_init ();
 
+    /* Initialize gmime */
+    if (! initialized) {
+ g_mime_init (0);
+ initialized = 1;
+    }
+
     notmuch = talloc (NULL, notmuch_database_t);
     notmuch->exception_reported = FALSE;
     notmuch->path = talloc_strdup (notmuch, path);
diff --git a/test/python b/test/python
index 0b56db3..c3aa726 100755
--- a/test/python
+++ b/test/python
@@ -5,7 +5,6 @@ test_description="python bindings"
 add_email_corpus
 
 test_begin_subtest "compare thread ids"
-test_subtest_known_broken
 test_python <<EOF
 import notmuch
 db = notmuch.Database(mode=notmuch.Database.MODE.READ_WRITE)
--
1.7.7.3

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] lib: call g_mime_init from notmuch_database_open

In reply to this post by David Bremner-2
On Fri, 30 Dec 2011 23:02:39 -0400, David Bremner <[hidden email]> wrote:
> On Fri, 30 Dec 2011 19:58:10 -0200, Kazuo Teramoto <[hidden email]> wrote:
> > We need to call g_mime_init to correct initialize the structures needed
> > by gmime before using it.
> > ---
> >  lib/database.cc |    5 +++++
 > >  1 files changed, 5 insertions(+), 0 deletions(-)
>
> I can confirm this patch (alone) fixes the segfault for me. and doesn't
> cause in test failures when applied against the release branch.  I'm
> considering pushing it into the 0.11 release. Any comments on that?

Do it. The current implementation does not break anything (in our case
as we don't do g_mime_shutdown() (0, 1 or 2 times)... The usage needs to
be "fixed" soon after...

> d

.. my suggestions how to fix this:

1) just call g_mime_init(0) without checking whether it is initialized
   already. As it't return type is 'void' it can be expected to keep some
   global storage (and reference count...). If it returned something else
   it either returns common (singleton) instance (and keeps reference count)
   or returns new instance each time called.... Never call g_mime_shutdown();
   just let operating system free resources when program exits. The first
   lines in g_mime_shutdown():

        if (--initialized)
                return;

   looks a bit suspicious -- what if application happens to call
   g_mime_shutdown() too often and then trying to call g_mime_init() again...

2) create function

  void
  notmuch_g_type_init (void)
  {
        static int initialized;
        if (initialized++)
        return;
        g_mime_init(0)
        atexit (g_mime_shutdown);
  }

  and use that in place of g_mime_init() always (perhaps use macro trickery
  to disallow g_mime_init (and g_mime_shutdown).

  Note that although atexit() makes pretty sure that in normal exit
  g_mime_shutdown() is called due to various reasons that normal exit
  case is not reached (dies by signal, usage of _exit, execve(2)
  hardware failure, lost power etc.)(*). Therefore, IMO, I'd leave this
  (particular) cleanup to be done by the operating system -- less,
  simpler application code.


Tomi

(*) I recall reading an article about 'design for failure' or 'expect
    failure' like 5 years ago but cannot find it anymore. The point on that
    article was that software can fail in any point (specially, compare
    man 2 rename  and then around lines 580-610 in
    http://git.busybox.net/busybox/tree/sysklogd/syslogd.c ). Since then
    I've tried to prioritize in finalize things that matter early and leave
    operating system to do other cleanpus for me (why do something yourself
    that machine can do for you). Did you know that doing full windows
    reboot is faster if the machine is just unplugged from power source
    for a second and then restarted instead of doing "clean" reboot cycle.
    Linux boots so fast I don't know if I'd notice the difference ;)
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
spaetz spaetz
Reply | Threaded
Open this post in threaded view
|

Re: Segmentation fault at gmime-iconv with python binding

In reply to this post by David Bremner-2
On Fri, 30 Dec 2011 10:58:06 -0400, David Bremner <[hidden email]> wrote:
> On Thu, 29 Dec 2011 22:57:27 -0200, Kazuo Teramoto <[hidden email]> wrote:
>
> > notmuch CLI show the message correct. I know nothing about gmime or
> > notmuch code, but can this be the case of the python bindings not
> > calling g_mime_init() correctly?

The python binding are never calling g_mime_init, as that is not exposed
to the python bindings. So either libnotmuch needs to take care of that
or there needs to be a clear guideline as to when a binding needs to
call what gmime stuff itself.

Sebastian

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

attachment0 (203 bytes) Download Attachment
Pieter Praet Pieter Praet
Reply | Threaded
Open this post in threaded view
|

Re: revised patch for gmime init, with test.

In reply to this post by David Bremner-2
On Sat, 31 Dec 2011 23:22:46 -0400, David Bremner <[hidden email]> wrote:
> It turns out that our existing (trivial) python test is enough to
> catch this bug, but the corpus needs to be augmented.  This
> augmentation is a bit intrusive so I'm thinking of cherry-picking only
> the actual fix to the release branch.
>

Due to the ~same change being applied in multiple places (and thus
with differing hashes), this has the potential of causing confusion
and/or quite some extra work when debugging using git-bisect(1), so
I'd like to propose that bugfixes for (to-be-)released code are only
applied on the 'maint' branch ('release' in the case of Notmuch),
and then immediately merged back into 'master'.  In fact, this would
preferrably happen after *every* (series of) commit(s) on the 'maint'
branch, to prevent issues like [1].


In this specific case (g_mime_init fix), it would have sufficed to
apply the test suite augmentation patches on 'master' and the bugfix
only on 'maint', merging 'maint' into 'master', and then removing
the "test_subtest_known_broken" line in a followup commit on 'master'.

Thus, maintainers would have fewer merge conflicts to resolve,
developers would at all times benefit from release-specific fixes,
and (pre-)release users would have clear indication of what was fixed
due to the test suite reporting "FIXED" instead of "PASS".


Thanks!

> Unfortunately the test message is 8 bit, so it may be encoded in some
> inconvenient way for patch application. The message is attached to an
> earlier message in the thread if you want to double check.
>
> I also wondered about putting g_type_init inside the (!initialized)
> test, but decided against it on the grounds of minimality.
>
> I think we want to in the medium term factor out all of the
> initialization code into one (probably private) function; we can clean
> things up a bit more then.
>
> _______________________________________________
> notmuch mailing list
> [hidden email]
> http://notmuchmail.org/mailman/listinfo/notmuch


Peace

--
Pieter

[1] id:"[hidden email]"
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch
12