Segmentation fault at gmime-iconv with python binding

Segmentation fault at gmime-iconv with python binding

[Kazuo Teramoto]

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

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

[-- Attachment #2: test.py --]
[-- Type: text/x-python, Size: 202 bytes --]

#!/usr/bin/env python2
import notmuch
db = notmuch.Database(mode=notmuch.Database.MODE.READ_WRITE)
q_new = notmuch.Query(db, 'id:"4EFC743A.3060609@april.org"')
for t in q_new.search_threads():
    pass

[-- Attachment #3: 4EFC743A.3060609_april.org --]
[-- Type: application/vnd.lotus-organizer, Size: 1309 bytes --]

[-- Attachment #4: notmuch_py_gmime.gdb_bt --]
[-- Type: application/octet-stream, Size: 4306 bytes --]

GNU gdb (GDB) 7.3.1
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /usr/bin/python2...(no debugging symbols found)...done.
(gdb) run
Starting program: /usr/bin/python2 test.py
[Thread debugging using libthread_db enabled]

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff5f4fd82 in cache_node_lookup (cache=0x0, key=0x7fffffffaab0 "iso-8859-1:UTF-8", use=1) at cache.c:110
110		node = g_hash_table_lookup (cache->node_hash, key);
(gdb) bt
#0  0x00007ffff5f4fd82 in cache_node_lookup (cache=0x0, key=0x7fffffffaab0 "iso-8859-1:UTF-8", use=1) at cache.c:110
#1  0x00007ffff5f2759c in g_mime_iconv_open (to=0x761ef0 "UTF-8", from=0x83d590 "iso-8859-1") at gmime-iconv.c:261
#2  0x00007ffff5f4633f in rfc2047_decode_word (in=0x82f350 "=?ISO-8859-1?Q?Fran=E7ois_Boulogne?=", inlen=36) at gmime-utils.c:1839
#3  0x00007ffff5f46ad1 in g_mime_utils_header_decode_phrase (phrase=0x82f350 "=?ISO-8859-1?Q?Fran=E7ois_Boulogne?=") at gmime-utils.c:2084
#4  0x00007ffff5f4a759 in _internet_address_decode_name (ia=0x83d630, name=0x88fae0) at internet-address.c:1367
#5  0x00007ffff5f4afab in decode_address (in=0x7fffffffad60) at internet-address.c:1659
#6  0x00007ffff5f4b003 in internet_address_list_parse_string (str=0x766530 "\"=?ISO-8859-1?Q?Fran=E7ois_Boulogne?=\" <boulogne.f@gmail.com>") at internet-address.c:1692
#7  0x00007ffff61915d7 in _thread_add_message (thread=0x6d99d0, message=0x6d9330) at lib/thread.cc:236
#8  0x00007ffff6191dd7 in _notmuch_thread_create (ctx=0x7d54e0, notmuch=0x6e01d0, seed_doc_id=213092, match_set=0x83c888, sort=NOTMUCH_SORT_NEWEST_FIRST) at lib/thread.cc:470
#9  0x00007ffff61906b7 in notmuch_threads_get (threads=0x83c870) at lib/query.cc:392
#10 0x00007ffff65a7e34 in ffi_call_unix64 () from /usr/lib/libffi.so.5
#11 0x00007ffff65a7855 in ffi_call () from /usr/lib/libffi.so.5
#12 0x00007ffff67bb1f7 in _ctypes_callproc () from /usr/lib/python2.7/lib-dynload/_ctypes.so
#13 0x00007ffff67b4a86 in ?? () from /usr/lib/python2.7/lib-dynload/_ctypes.so
#14 0x00007ffff7a66683 in PyObject_Call () from /usr/lib/libpython2.7.so.1.0
#15 0x00007ffff7afbbda in PyEval_EvalFrameEx () from /usr/lib/libpython2.7.so.1.0
#16 0x00007ffff7afe8ef in PyEval_EvalCodeEx () from /usr/lib/libpython2.7.so.1.0
#17 0x00007ffff7a8b15c in function_call () from /usr/lib/libpython2.7.so.1.0
#18 0x00007ffff7a66683 in PyObject_Call () from /usr/lib/libpython2.7.so.1.0
#19 0x00007ffff7a752bf in instancemethod_call () from /usr/lib/libpython2.7.so.1.0
#20 0x00007ffff7a66683 in PyObject_Call () from /usr/lib/libpython2.7.so.1.0
#21 0x00007ffff7abc002 in call_method () from /usr/lib/libpython2.7.so.1.0
#22 0x00007ffff7af8b1f in PyEval_EvalFrameEx () from /usr/lib/libpython2.7.so.1.0
#23 0x00007ffff7afe8ef in PyEval_EvalCodeEx () from /usr/lib/libpython2.7.so.1.0
#24 0x00007ffff7afea22 in PyEval_EvalCode () from /usr/lib/libpython2.7.so.1.0
#25 0x00007ffff7b18d8c in run_mod () from /usr/lib/libpython2.7.so.1.0
#26 0x00007ffff7b19b90 in PyRun_FileExFlags () from /usr/lib/libpython2.7.so.1.0
#27 0x00007ffff7b1a60f in PyRun_SimpleFileExFlags () from /usr/lib/libpython2.7.so.1.0
#28 0x00007ffff7b2bd25 in Py_Main () from /usr/lib/libpython2.7.so.1.0
#29 0x00007ffff747e38d in __libc_start_main () from /lib/libc.so.6
#30 0x00000000004006a1 in _start ()
(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) 

Re: Segmentation fault at gmime-iconv with python binding

[David Bremner]

On Thu, 29 Dec 2011 22:57:27 -0200, Kazuo Teramoto <kaz.rag@gmail.com> 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

[PATCH 0/2] Multiples calls of g_mime_init

[Kazuo Teramoto]

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

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

[Kazuo Teramoto]

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

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

[David Bremner]

On Fri, 30 Dec 2011 19:58:09 -0200, Kazuo Teramoto <kaz.rag@gmail.com> 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

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

[Kazuo Teramoto]

On 2011-12-31T00:55:59, David Bremner wrote:

>On Fri, 30 Dec 2011 19:58:09 -0200, Kazuo Teramoto <kaz.rag@gmail.com> 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?

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

[Austin Clements]

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;
>  

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

[David Bremner]

On Fri, 30 Dec 2011 19:58:09 -0200, Kazuo Teramoto <kaz.rag@gmail.com> wrote:
> g_mime_init already check for multiple initializations.
> ---
>  lib/index.cc       |    4 ----
>  lib/message-file.c |    4 ----

Hi Kazuo, 

I'm going to mark this obsolete for now, because of the plan to replace it with
some central init function.

d

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

[Kazuo Teramoto]

On 2012-01-21T11:25:39, David Bremner wrote:

>On Fri, 30 Dec 2011 19:58:09 -0200, Kazuo Teramoto <kaz.rag@gmail.com> wrote:
>> g_mime_init already check for multiple initializations.
>> ---
>>  lib/index.cc       |    4 ----
>>  lib/message-file.c |    4 ----
>
>I'm going to mark this obsolete for now, because of the plan to replace it with
>some central init function.

Yes please do it. Thanks.

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

[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

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

[David Bremner]

On Fri, 30 Dec 2011 19:58:10 -0200, Kazuo Teramoto <kaz.rag@gmail.com> 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

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

[Kazuo Teramoto]

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

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

[Kazuo Teramoto]

As reported in
id:"CAEbOPGyuHnz4BPtDutnTPUHcP3eYcRCRkXhYoJR43RUMw671+g@mail.gmail.com"
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

revised patch for gmime init, with test.

[David Bremner]

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.

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

[David Bremner]

From: David Bremner <bremner@debian.org>

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

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

[David Bremner]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 10052 bytes --]

From: David Bremner <bremner@debian.org>

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: <4EFC743A.3060609@april.org>
+Date: Thu, 29 Dec 2010 15:07:54 +0100
+From: "=?ISO-8859-1?Q?Fran=E7ois_Boulogne?=" <boulogne.f@gmail.com>
+User-Agent: Mozilla/5.0 (X11; Linux i686;
+ rv:9.0) Gecko/20111224 Thunderbird/9.0.1
+MIME-Version: 1.0
+To: Allan McRae <allan@archlinux.org>, 
+ "Discussion about the Arch User Repository (AUR)" <aur-general@archlinux.org>
+References: <4EFC3931.6030007@april.org> <4EFC3D62.4030202@archlinux.org>
+In-Reply-To: <4EFC3D62.4030202@archlinux.org>
+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 <olivier.berger@it-sudparis.eu>
+To: olivier.berger@it-sudparis.eu
+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: <877h1wv7mg.fsf@inf-8657.int-evry.fr>
+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:4EFC743A.3060609@april.org
+id:877h1wv7mg.fsf@inf-8657.int-evry.fr
 id:1258544095-16616-1-git-send-email-chris@chris-wilson.co.uk
 id:877htoqdbo.fsf@yoom.home.cworth.org
 id:878we4qdqf.fsf@yoom.home.cworth.org
@@ -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
-["1258544095-16616-1-git-send-email-chris@chris-wilson.co.uk",
+["4EFC743A.3060609@april.org",
+"877h1wv7mg.fsf@inf-8657.int-evry.fr",
+"1258544095-16616-1-git-send-email-chris@chris-wilson.co.uk",
 "877htoqdbo.fsf@yoom.home.cworth.org",
 "878we4qdqf.fsf@yoom.home.cworth.org",
 "87aaykqe24.fsf@yoom.home.cworth.org",
@@ -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

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

[David Bremner]

From: Kazuo Teramoto <kaz.rag@gmail.com>

As reported in
id:"CAEbOPGyuHnz4BPtDutnTPUHcP3eYcRCRkXhYoJR43RUMw671+g@mail.gmail.com"
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

Re: revised patch for gmime init, with test.

[Pieter Praet]

On Sat, 31 Dec 2011 23:22:46 -0400, David Bremner <david@tethera.net> 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
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch


Peace

-- 
Pieter

[1] id:"87mxa95u3d.fsf@zancas.localnet"

Re: revised patch for gmime init, with test.

[David Bremner]

On Thu, 12 Jan 2012 18:25:38 +0100, Pieter Praet <pieter@praet.org> wrote:
> On Sat, 31 Dec 2011 23:22:46 -0400, David Bremner <david@tethera.net> wrote:

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

There is some merit it to this. On the other hand, it makes the history
messier.  [1] would have also been prevented by making the patch against
the right branch.

Re: revised patch for gmime init, with test.

[David Bremner]

On Thu, 12 Jan 2012 23:46:46 -0400, David Bremner <david@tethera.net> wrote:
> On Thu, 12 Jan 2012 18:25:38 +0100, Pieter Praet <pieter@praet.org> wrote:
> > On Sat, 31 Dec 2011 23:22:46 -0400, David Bremner <david@tethera.net> wrote:
> 
> > 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].
> 
> There is some merit it to this. On the other hand, it makes the history
> messier.  [1] would have also been prevented by making the patch against
> the right branch.

I thought about this a bit more, and I agree that at least the release
candidates (basically anything tagged on branch release) ought to be
merged back to master. Since any series of bugfix patches seems to be
cause for a new release candidate, this should avoid the need to have
doubly applied patches.

I'm less convinced about the need to merge every little doc change and
debian packaging change back to master right away. This might be a
purely aesthetic objection; I'm not sure if the extra merge commits
cause any problems for e.g. bisection.

d

Re: revised patch for gmime init, with test.

[Jameson Graef Rollins]

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

On Fri, 13 Jan 2012 05:05:35 -0400, David Bremner <david@tethera.net> wrote:
> I thought about this a bit more, and I agree that at least the release
> candidates (basically anything tagged on branch release) ought to be
> merged back to master. Since any series of bugfix patches seems to be
> cause for a new release candidate, this should avoid the need to have
> doubly applied patches.
> 
> I'm less convinced about the need to merge every little doc change and
> debian packaging change back to master right away. This might be a
> purely aesthetic objection; I'm not sure if the extra merge commits
> cause any problems for e.g. bisection.

Doesn't everything need to be merged into master eventually anyway?  It
seems to me that unless it's a change that very narrowly targeting an
issue in a release branch that is not an issue in master, every patch
will ultimately need to be applied to both.  It doesn't really make
sense to me to apply a change to one branch and not the other, if they
will eventually need to be applied to both anyway.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: revised patch for gmime init, with test.

[David Bremner]

On Fri, 13 Jan 2012 12:52:48 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> 
> Doesn't everything need to be merged into master eventually anyway?  It
> seems to me that unless it's a change that very narrowly targeting an
> issue in a release branch that is not an issue in master, every patch
> will ultimately need to be applied to both.  It doesn't really make
> sense to me to apply a change to one branch and not the other, if they
> will eventually need to be applied to both anyway.

The following two sequences of commands apply the same changes, but
result in a different history graph.

1) notmuch checkout release && git am patch  && \
   notmuch checkout master && git cherry-pick release

2) notmuch checkout release && git am patch  && \
   notmuch checkout master && git merge release

well, they apply the same changes if release was an ancestor of master
when the they both began.

Re: revised patch for gmime init, with test.

[Pieter Praet]

On Fri, 13 Jan 2012 05:05:35 -0400, David Bremner <david@tethera.net> wrote:
> On Thu, 12 Jan 2012 23:46:46 -0400, David Bremner <david@tethera.net> wrote:
> > On Thu, 12 Jan 2012 18:25:38 +0100, Pieter Praet <pieter@praet.org> wrote:
> > > On Sat, 31 Dec 2011 23:22:46 -0400, David Bremner <david@tethera.net> wrote:
> > 
> > > 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].
> > 
> > There is some merit it to this. On the other hand, it makes the history
> > messier.  [1] would have also been prevented by making the patch against
> > the right branch.
> 
> I thought about this a bit more, and I agree that at least the release
> candidates (basically anything tagged on branch release) ought to be
> merged back to master. Since any series of bugfix patches seems to be
> cause for a new release candidate, this should avoid the need to have
> doubly applied patches.
> 

Thanks!

> I'm less convinced about the need to merge every little doc change and
> debian packaging change back to master right away. This might be a
> purely aesthetic objection; [...]

See my previous reply [1].

> [...] I'm not sure if the extra merge commits
> cause any problems for e.g. bisection.
> 

Infrequent merging increases the possibility of bugs due to unforeseen
interactions between commits on different branches, which is likely to
require one to do a multitude of fake merges (`git merge --no-commit')
in order to properly track down the offending commit, so... frequent
merging would actually *prevent* issues when bisecting.


> d


Peace

-- 
Pieter

[1] id:"878vlar7ka.fsf@praet.org"

Re: revised patch for gmime init, with test.

[Pieter Praet]

On Thu, 12 Jan 2012 23:46:46 -0400, David Bremner <david@tethera.net> wrote:
> On Thu, 12 Jan 2012 18:25:38 +0100, Pieter Praet <pieter@praet.org> wrote:
> > On Sat, 31 Dec 2011 23:22:46 -0400, David Bremner <david@tethera.net> wrote:
> 
> > 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].
> 
> There is some merit it to this. On the other hand, it makes the history
> messier.  [...]

This *can* get rather messy when interlacing the 'master'/'maint' merges
with non-rebased changesets pulled from other repos (most often aren't
rebased in advance since they've already been published), which is a
common occurence in the magit [1] repo.

But take Org-mode [2] for example (which is an *extremely* active project),
where non-rebased changesets are rare(ish), and where the 'maint' branch
is constantly being merged back into 'master', resulting in a very clean
ladder-like commit log, eg. :

  --*----*----*----*----*----*---*----*---*---*--- master
     \                 /             /         \
      *-------*-------*-------------*-----------*--- maint
     0.11   bugfix   NEWS        bugfix       0.12~rc1


> [...] [1] would have also been prevented by making the patch against
> the right branch.
> 

True, but if we can obviate the need to check if we're on the right
branch altogether, without causing any unwanted side-effects, wouldn't
it be counterproductive not to?


Peace

-- 
Pieter

[1] git://github.com/magit/magit.git
[2] git://orgmode.org/org-mode.git

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

[Tomi Ollila]

On Fri, 30 Dec 2011 23:02:39 -0400, David Bremner <david@tethera.net> wrote:
> On Fri, 30 Dec 2011 19:58:10 -0200, Kazuo Teramoto <kaz.rag@gmail.com> 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 ;)

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

[Patrick Totzke]

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:4eddf2b1.4288980a.0b74.5557@mx.google.com"

hth,
/p

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

[Kazuo Teramoto]

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:4eddf2b1.4288980a.0b74.5557@mx.google.com"
>

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.

Re: Segmentation fault at gmime-iconv with python binding

[Sebastian Spaeth]

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

On Fri, 30 Dec 2011 10:58:06 -0400, David Bremner <david@tethera.net> wrote:
> On Thu, 29 Dec 2011 22:57:27 -0200, Kazuo Teramoto <kaz.rag@gmail.com> 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

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]