[notmuch] Segfault with weird Message-ID

[Carl Worth]

On Sat, 21 Nov 2009 23:26:15 +0100, Mike Hommey  
wrote:
> I just was able to reproduce after starting over.

Thanks Mike. I was able to reproduce this as well by eliminating the
spurious blank line I had on the 2nd or 3rd line. (So maybe that managed
to sneak in when you sent me the message.)

> header isn't "", and message_id is correctly filled. I can also confirm
> the exception is thrown from notmuch->xapian_db->add_document.

Yes. We were trying to add a term that is too long for Xapian. I've
fixed this by simply falling back to our existing sha-1 code when a
message ID is long.

Thanks so much for the bug report!

-Carl

commit 5d56e931b99d575dbb0b936d24aae5e9903861ad
Author: Carl Worth 
Date:   Sun Nov 22 04:03:49 2009 +0100

add_message: Use sha-1 in place of overly long message ID.

Since Xapian has a limit on the maximum length of a term, we have
to check for that before trying to add the message ID as a term.

This fixes the bug reported by Mike Hommey here:

<20091120132625.GA19246 at glandium.org>

I've also constructed 20 files with a range of message ID lengths
centered around the Xapian term-length limit which I'll use to seed a
new test suite soon.

diff --git a/lib/database.cc b/lib/database.cc
index 169dc5e..f4a445a 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -892,7 +892,7 @@ notmuch_database_add_message (notmuch_database_t *notmuch,

 const char *date, *header;
 const char *from, *to, *subject;
-char *message_id;
+char *message_id = NULL;

 if (message_ret)
*message_ret = NULL;
@@ -937,11 +937,20 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
header = notmuch_message_file_get_header (message_file, "message-id");
if (header && *header != '\0') {
message_id = _parse_message_id (message_file, header, NULL);
+
/* So the header value isn't RFC-compliant, but it's
 * better than no message-id at all. */
if (message_id == NULL)
message_id = talloc_strdup (message_file, header);
-   } else {
+
+   /* Reject a Message ID that's too long. */
+   if (message_id && strlen (message_id) + 1 > NOTMUCH_TERM_MAX) {
+   talloc_free (message_id);
+   message_id = NULL;
+   }
+   }
+
+   if (message_id == NULL ) {
/* No message-id at all, let's generate one by taking a
 * hash over the file's contents. */
char *sha1 = notmuch_sha1_of_file (filename);

[notmuch] Segfault with weird Message-ID

[Mike Hommey]

On Fri, Nov 20, 2009 at 10:05:56PM +0100, Mike Hommey wrote:
> On Fri, Nov 20, 2009 at 09:53:37PM +0100, Carl Worth wrote:
> > On Fri, 20 Nov 2009 14:26:25 +0100, Mike Hommey  > glandium.org> wrote:
> > > - for some reason, xapian doesn't want to add the document corresponding
> > >   to this old spam message: notmuch->xapian_db->add_document throws an
> > >   exception.
> > 
> > I think things had just gone wrong long before then.
> 
> I *did* see it throwing an exception from there. The sad thing is that I
> can't reproduce the problem anymore :-/
> 
> > > I can provide the spam if necessary, or can continue debugging the issue
> > > with some guidance.
> > 
> > Thanks for providing it. It turns out that the giant Message-Id value
> > wasn't causing the problem. Instead the message was corrupt by having a
> > stray new line at the third line. (So GMime is seeing only the first two
> > lines of headers). We *used* to have working code to detect this kind of
> > file as "not an email" but again, this broke when we changed
> > notmuch_message_get_header to return "" instead of NULL for missing
> > headers.
> 
> Interestingly, when I first traced on what message the crash was
> happening, I did see notmuch having the message-id in the message_id
> variable.

I just was able to reproduce after starting over.

header isn't "", and message_id is correctly filled. I can also confirm
the exception is thrown from notmuch->xapian_db->add_document.

> FWIW, that was using c05c3f1.

With 3ae12b1, I get the following output:
Error: A Xapian exception occurred. Halting processing.

But I confirm there is no crash, now.

Cheers,

Mike

[notmuch] Segfault with weird Message-ID

[Mike Hommey]

On Fri, Nov 20, 2009 at 09:53:37PM +0100, Carl Worth wrote:
> On Fri, 20 Nov 2009 14:26:25 +0100, Mike Hommey  
> wrote:
> > - for some reason, xapian doesn't want to add the document corresponding
> >   to this old spam message: notmuch->xapian_db->add_document throws an
> >   exception.
> 
> I think things had just gone wrong long before then.

I *did* see it throwing an exception from there. The sad thing is that I
can't reproduce the problem anymore :-/

> > I can provide the spam if necessary, or can continue debugging the issue
> > with some guidance.
> 
> Thanks for providing it. It turns out that the giant Message-Id value
> wasn't causing the problem. Instead the message was corrupt by having a
> stray new line at the third line. (So GMime is seeing only the first two
> lines of headers). We *used* to have working code to detect this kind of
> file as "not an email" but again, this broke when we changed
> notmuch_message_get_header to return "" instead of NULL for missing
> headers.

Interestingly, when I first traced on what message the crash was
happening, I did see notmuch having the message-id in the message_id
variable.

FWIW, that was using c05c3f1.

I'll see if I can reproduce my segfault again when starting from scratch
again, and will also give a try to your patches.

Cheers,

Mike

[notmuch] Segfault with weird Message-ID

[Carl Worth]

On Fri, 20 Nov 2009 14:26:25 +0100, Mike Hommey  
wrote:
> I got a segfault when importing my maildir. It happened because of an
> old weird email, where the message-id is the following:
> Message-ID: <22b17a1f$4fbe$0550 at myrop (ew6.southwind.net 
> [216.53.98.70]) by onyx.southwind.net from homepage.com (114.230.197.216) by 
> newmail.spectraweb.ch from default (m202.2-25.warwick.net [
> 218.242.202.80]) by host.warwick.net (8.10.0.Beta10/8.10.0.Beta10) with SMTP 
> id e9GKEKk19201>

Thanks for sharing this Mike, (and for sending me the original file).

> Anyways, the stack dump is the following:
> #0  0x76d1e598 in Xapian::Document::add_term(std::string const&, 
> unsigned int) () from /usr/lib/libxapian.so.15
> #1  0x0040f5ff in _notmuch_message_add_term (message=0x0, 
> prefix_name=0x41ad7f "tag", value=0x4191b0 "inbox") at lib/message.cc:587
> #2  0x0040f827 in notmuch_message_add_tag (message=0x0, tag=0x4191b0 
> "inbox") at lib/message.cc:668
> #3  0x00407bc8 in tag_inbox_and_unread (message=0x0) at 
> notmuch-new.c:44
> #4  0x00407f63 in add_files_recursive (notmuch=0x62cc20, 
> path=0x832e90 "/home/mh/Maildir/saved-messages/cur", st=0x7fffe000, 
> state=0x7fffe240) at notmuch-new.c:185
> #5  0x00408036 in add_files_recursive (notmuch=0x62cc20, 
> path=0x832de0 "/home/mh/Maildir/saved-messages", st=0x7fffe000, 
> state=0x7fffe240) at notmuch-new.c:223
> #6  0x00408036 in add_files_recursive (notmuch=0x62cc20, 
> path=0x62c920 "/home/mh/Maildir", st=0x7fffe000, state=0x7fffe240) at 
> notmuch-new.c:223
> #7  0x00408245 in add_files (notmuch=0x62cc20, path=0x62c920 
> "/home/mh/Maildir", state=0x7fffe240) at notmuch-new.c:287
> #8  0x00408704 in notmuch_new_command (ctx=0x61f140, argc=0, 
> argv=0x7fffe3e8) at notmuch-new.c:431
> #9  0x00406ea8 in main (argc=2, argv=0x7fffe3d8) at notmuch.c:400

I didn't get the same crash when importing the file. But I did get a
short document out of it (just a handful of terms indexed) and most
significantly, an empty message-ID term.

Xapian has a limit on the maximum length of a term, so one thing we'll
need to do here is to notice if the message ID exceeds that length and
then treat it as a we treat a missing Message-ID header, (that is,
generate our own message ID by computing a sha-1 hash over the message).

So, there was an obvious bug in the message-ID handling, (the code was
still looking for NULL for a missing header, but we now return "" for a
missing header instead). I've fixed this.

> Now, looking at the code, there seems to me there actually 3 problems:
> - _notmuch_message_create_for_message_id can return NULL, and while
>   there is a test for it in notmuch_database_add_message, the function
>   still returns a success code

Thanks. This is fixed now.

> - things are still going on even when message is NULL in
>   add_files_recursive

I didn't replicate this case, but it *should* be fixed now that
notmuch_database_add_message is returning a non-success value.

> - for some reason, xapian doesn't want to add the document corresponding
>   to this old spam message: notmuch->xapian_db->add_document throws an
>   exception.

I think things had just gone wrong long before then.

> I can provide the spam if necessary, or can continue debugging the issue
> with some guidance.

Thanks for providing it. It turns out that the giant Message-Id value
wasn't causing the problem. Instead the message was corrupt by having a
stray new line at the third line. (So GMime is seeing only the first two
lines of headers). We *used* to have working code to detect this kind of
file as "not an email" but again, this broke when we changed
notmuch_message_get_header to return "" instead of NULL for missing
headers.

See patches below (just pushed now as well) for the fixes.

-Carl

-- next part --
An embedded and charset-unspecified text was scrubbed...
Name: handle-corrupt-mail-and-non-mail.patchset
URL: 

[notmuch] Segfault with weird Message-ID

[Carl Worth]

On Fri, 20 Nov 2009 14:26:25 +0100, Mike Hommey  
wrote:
> I can provide the spam if necessary, or can continue debugging the issue
> with some guidance.

It sounds strange to say it, but yes, please send me that spam!

-Carl

[notmuch] Segfault with weird Message-ID

[Mike Hommey]

Hi,

I got a segfault when importing my maildir. It happened because of an
old weird email, where the message-id is the following:
Message-ID: <22b17a1f$4fbe$0550 at myrop (ew6.southwind.net 
[216.53.98.70]) by onyx.southwind.net from homepage.com (114.230.197.216) by 
newmail.spectraweb.ch from default (m202.2-25.warwick.net [
218.242.202.80]) by host.warwick.net (8.10.0.Beta10/8.10.0.Beta10) with SMTP id 
e9GKEKk19201>

I have absolutely no idea how it got this value, but the mail being
an archived 8 years old spam, I'm not exactly sure if anyone would 
still expect such message id to occur.

Anyways, the stack dump is the following:
#0  0x76d1e598 in Xapian::Document::add_term(std::string const&, 
unsigned int) () from /usr/lib/libxapian.so.15
#1  0x0040f5ff in _notmuch_message_add_term (message=0x0, 
prefix_name=0x41ad7f "tag", value=0x4191b0 "inbox") at lib/message.cc:587
#2  0x0040f827 in notmuch_message_add_tag (message=0x0, tag=0x4191b0 
"inbox") at lib/message.cc:668
#3  0x00407bc8 in tag_inbox_and_unread (message=0x0) at notmuch-new.c:44
#4  0x00407f63 in add_files_recursive (notmuch=0x62cc20, path=0x832e90 
"/home/mh/Maildir/saved-messages/cur", st=0x7fffe000, state=0x7fffe240) 
at notmuch-new.c:185
#5  0x00408036 in add_files_recursive (notmuch=0x62cc20, path=0x832de0 
"/home/mh/Maildir/saved-messages", st=0x7fffe000, state=0x7fffe240) at 
notmuch-new.c:223
#6  0x00408036 in add_files_recursive (notmuch=0x62cc20, path=0x62c920 
"/home/mh/Maildir", st=0x7fffe000, state=0x7fffe240) at 
notmuch-new.c:223
#7  0x00408245 in add_files (notmuch=0x62cc20, path=0x62c920 
"/home/mh/Maildir", state=0x7fffe240) at notmuch-new.c:287
#8  0x00408704 in notmuch_new_command (ctx=0x61f140, argc=0, 
argv=0x7fffe3e8) at notmuch-new.c:431
#9  0x00406ea8 in main (argc=2, argv=0x7fffe3d8) at notmuch.c:400

And the most likely problem is that message is NULL.

Now, looking at the code, there seems to me there actually 3 problems:
- _notmuch_message_create_for_message_id can return NULL, and while
  there is a test for it in notmuch_database_add_message, the function
  still returns a success code
- things are still going on even when message is NULL in
  add_files_recursive
- for some reason, xapian doesn't want to add the document corresponding
  to this old spam message: notmuch->xapian_db->add_document throws an
  exception.

I can provide the spam if necessary, or can continue debugging the issue
with some guidance.

Cheers,

Mike