--- /dev/null
+Return-Path: <cworth@cworth.org>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+ by olra.theworths.org (Postfix) with ESMTP id 1D1DB431FBF;\r
+ Fri, 20 Nov 2009 12:53:50 -0800 (PST)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+ by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+ with ESMTP id efKiFtXgGhKT; Fri, 20 Nov 2009 12:53:49 -0800 (PST)\r
+Received: from cworth.org (localhost [127.0.0.1])\r
+ by olra.theworths.org (Postfix) with ESMTP id 8EC7D431FAE;\r
+ Fri, 20 Nov 2009 12:53:48 -0800 (PST)\r
+From: Carl Worth <cworth@cworth.org>\r
+To: Mike Hommey <mh+notmuch@glandium.org>, notmuch@notmuchmail.org\r
+In-Reply-To: <20091120132625.GA19246@glandium.org>\r
+References: <20091120132625.GA19246@glandium.org>\r
+Date: Fri, 20 Nov 2009 21:53:37 +0100\r
+Message-ID: <87y6m0lxym.fsf@yoom.home.cworth.org>\r
+MIME-Version: 1.0\r
+Content-Type: multipart/mixed; boundary="=-=-="\r
+Subject: Re: [notmuch] Segfault with weird Message-ID\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.12\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+ <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Fri, 20 Nov 2009 20:53:50 -0000\r
+\r
+--=-=-=\r
+\r
+On Fri, 20 Nov 2009 14:26:25 +0100, Mike Hommey <mh+notmuch@glandium.org> wrote:\r
+> I got a segfault when importing my maildir. It happened because of an\r
+> old weird email, where the message-id is the following:\r
+> Message-ID: <000022b17a1f$00004fbe$00000550@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 [\r
+> 218.242.202.80]) by host.warwick.net (8.10.0.Beta10/8.10.0.Beta10) with SMTP id e9GKEKk19201>\r
+\r
+Thanks for sharing this Mike, (and for sending me the original file).\r
+\r
+> Anyways, the stack dump is the following:\r
+> #0 0x00007ffff6d1e598 in Xapian::Document::add_term(std::string const&, unsigned int) () from /usr/lib/libxapian.so.15\r
+> #1 0x000000000040f5ff in _notmuch_message_add_term (message=0x0, prefix_name=0x41ad7f "tag", value=0x4191b0 "inbox") at lib/message.cc:587\r
+> #2 0x000000000040f827 in notmuch_message_add_tag (message=0x0, tag=0x4191b0 "inbox") at lib/message.cc:668\r
+> #3 0x0000000000407bc8 in tag_inbox_and_unread (message=0x0) at notmuch-new.c:44\r
+> #4 0x0000000000407f63 in add_files_recursive (notmuch=0x62cc20, path=0x832e90 "/home/mh/Maildir/saved-messages/cur", st=0x7fffffffe000, state=0x7fffffffe240) at notmuch-new.c:185\r
+> #5 0x0000000000408036 in add_files_recursive (notmuch=0x62cc20, path=0x832de0 "/home/mh/Maildir/saved-messages", st=0x7fffffffe000, state=0x7fffffffe240) at notmuch-new.c:223\r
+> #6 0x0000000000408036 in add_files_recursive (notmuch=0x62cc20, path=0x62c920 "/home/mh/Maildir", st=0x7fffffffe000, state=0x7fffffffe240) at notmuch-new.c:223\r
+> #7 0x0000000000408245 in add_files (notmuch=0x62cc20, path=0x62c920 "/home/mh/Maildir", state=0x7fffffffe240) at notmuch-new.c:287\r
+> #8 0x0000000000408704 in notmuch_new_command (ctx=0x61f140, argc=0, argv=0x7fffffffe3e8) at notmuch-new.c:431\r
+> #9 0x0000000000406ea8 in main (argc=2, argv=0x7fffffffe3d8) at notmuch.c:400\r
+\r
+I didn't get the same crash when importing the file. But I did get a\r
+short document out of it (just a handful of terms indexed) and most\r
+significantly, an empty message-ID term.\r
+\r
+Xapian has a limit on the maximum length of a term, so one thing we'll\r
+need to do here is to notice if the message ID exceeds that length and\r
+then treat it as a we treat a missing Message-ID header, (that is,\r
+generate our own message ID by computing a sha-1 hash over the message).\r
+\r
+So, there was an obvious bug in the message-ID handling, (the code was\r
+still looking for NULL for a missing header, but we now return "" for a\r
+missing header instead). I've fixed this.\r
+\r
+> Now, looking at the code, there seems to me there actually 3 problems:\r
+> - _notmuch_message_create_for_message_id can return NULL, and while\r
+> there is a test for it in notmuch_database_add_message, the function\r
+> still returns a success code\r
+\r
+Thanks. This is fixed now.\r
+\r
+> - things are still going on even when message is NULL in\r
+> add_files_recursive\r
+\r
+I didn't replicate this case, but it *should* be fixed now that\r
+notmuch_database_add_message is returning a non-success value.\r
+\r
+> - for some reason, xapian doesn't want to add the document corresponding\r
+> to this old spam message: notmuch->xapian_db->add_document throws an\r
+> exception.\r
+\r
+I think things had just gone wrong long before then.\r
+\r
+> I can provide the spam if necessary, or can continue debugging the issue\r
+> with some guidance.\r
+\r
+Thanks for providing it. It turns out that the giant Message-Id value\r
+wasn't causing the problem. Instead the message was corrupt by having a\r
+stray new line at the third line. (So GMime is seeing only the first two\r
+lines of headers). We *used* to have working code to detect this kind of\r
+file as "not an email" but again, this broke when we changed\r
+notmuch_message_get_header to return "" instead of NULL for missing\r
+headers.\r
+\r
+See patches below (just pushed now as well) for the fixes.\r
+\r
+-Carl\r
+\r
+\r
+--=-=-=\r
+Content-Disposition: inline;\r
+ filename=handle-corrupt-mail-and-non-mail.patchset\r
+\r
+>From 52292c548512214fd3dd205edb4ca9cf7955f2b3 Mon Sep 17 00:00:00 2001\r
+From: Carl Worth <cworth@cworth.org>\r
+Date: Fri, 20 Nov 2009 19:31:00 +0100\r
+Subject: [PATCH 1/3] add_message: Properly handle missing Message-ID once again.\r
+\r
+There's been a fair amount of fallout from when we changed\r
+message_file_get_header from returning NULL to returning "" for\r
+missing headers. This is yet more fallout from that, (where we were\r
+accepting an empty message-ID rather than generating one like we want\r
+to).\r
+---\r
+ lib/database.cc | 2 +-\r
+ 1 files changed, 1 insertions(+), 1 deletions(-)\r
+\r
+diff --git a/lib/database.cc b/lib/database.cc\r
+index 726c5a9..294247e 100644\r
+--- a/lib/database.cc\r
++++ b/lib/database.cc\r
+@@ -908,7 +908,7 @@ notmuch_database_add_message (notmuch_database_t *notmuch,\r
+ * is to find a message ID (or else create one ourselves). */\r
+ \r
+ header = notmuch_message_file_get_header (message_file, "message-id");\r
+- if (header) {\r
++ if (header && *header != '\0') {\r
+ message_id = _parse_message_id (message_file, header, NULL);\r
+ /* So the header value isn't RFC-compliant, but it's\r
+ * better than no message-id at all. */\r
+-- \r
+1.6.5.2\r
+\r
+\r
+>From 656e4c413d84984dcc5fbd8016907ed03c343cb8 Mon Sep 17 00:00:00 2001\r
+From: Carl Worth <cworth@cworth.org>\r
+Date: Fri, 20 Nov 2009 21:02:11 +0100\r
+Subject: [PATCH 2/3] notmuch_database_add_message: Add missing error-value propagation.\r
+\r
+Thanks to Mike Hommey for doing the analysis that led to noticing that\r
+this was missing.\r
+---\r
+ lib/database.cc | 5 ++++-\r
+ 1 files changed, 4 insertions(+), 1 deletions(-)\r
+\r
+diff --git a/lib/database.cc b/lib/database.cc\r
+index 294247e..58a350d 100644\r
+--- a/lib/database.cc\r
++++ b/lib/database.cc\r
+@@ -940,8 +940,11 @@ notmuch_database_add_message (notmuch_database_t *notmuch,\r
+ \r
+ talloc_free (message_id);\r
+ \r
+- if (message == NULL)\r
++ if (message == NULL) {\r
++ ret = COERCE_STATUS (private_status,\r
++ "Unexpected status value from _notmuch_message_create_for_message_id");\r
+ goto DONE;\r
++ }\r
+ \r
+ /* Is this a newly created message object? */\r
+ if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {\r
+-- \r
+1.6.5.2\r
+\r
+\r
+>From 3ae12b1e286d1c0041a2e3957cb01daa2981dad9 Mon Sep 17 00:00:00 2001\r
+From: Carl Worth <cworth@cworth.org>\r
+Date: Fri, 20 Nov 2009 21:46:37 +0100\r
+Subject: [PATCH 3/3] add_message: Re-fix handling of non-mail files.\r
+\r
+More fallout from _get_header now returning "" for missing headers.\r
+\r
+The bug here is that we would no longer detect that a file is not an\r
+email message and give up on it like we should.\r
+\r
+And this time, I actually audited all callers to\r
+notmuch_message_get_header, so hopefully we're done fixing this\r
+bug over and over.\r
+---\r
+ lib/database.cc | 10 +++++-----\r
+ lib/message.cc | 2 +-\r
+ 2 files changed, 6 insertions(+), 6 deletions(-)\r
+\r
+diff --git a/lib/database.cc b/lib/database.cc\r
+index 58a350d..207246c 100644\r
+--- a/lib/database.cc\r
++++ b/lib/database.cc\r
+@@ -323,7 +323,7 @@ _parse_message_id (void *ctx, const char *message_id, const char **next)\r
+ const char *s, *end;\r
+ char *result;\r
+ \r
+- if (message_id == NULL)\r
++ if (message_id == NULL || *message_id == '\0')\r
+ return NULL;\r
+ \r
+ s = message_id;\r
+@@ -391,7 +391,7 @@ parse_references (void *ctx,\r
+ {\r
+ char *ref;\r
+ \r
+- if (refs == NULL)\r
++ if (refs == NULL || *refs == '\0')\r
+ return;\r
+ \r
+ while (*refs) {\r
+@@ -896,9 +896,9 @@ notmuch_database_add_message (notmuch_database_t *notmuch,\r
+ subject = notmuch_message_file_get_header (message_file, "subject");\r
+ to = notmuch_message_file_get_header (message_file, "to");\r
+ \r
+- if (from == NULL &&\r
+- subject == NULL &&\r
+- to == NULL)\r
++ if ((from == NULL || *from == '\0') &&\r
++ (subject == NULL || *subject == '\0') &&\r
++ (to == NULL || *to == '\0'))\r
+ {\r
+ ret = NOTMUCH_STATUS_FILE_NOT_EMAIL;\r
+ goto DONE;\r
+diff --git a/lib/message.cc b/lib/message.cc\r
+index 41dddd0..e0b8a8e 100644\r
+--- a/lib/message.cc\r
++++ b/lib/message.cc\r
+@@ -491,7 +491,7 @@ _notmuch_message_set_date (notmuch_message_t *message,\r
+ \r
+ /* GMime really doesn't want to see a NULL date, so protect its\r
+ * sensibilities. */\r
+- if (date == NULL)\r
++ if (date == NULL || *date == '\0')\r
+ time_value = 0;\r
+ else\r
+ time_value = g_mime_utils_header_decode_date (date, NULL);\r
+-- \r
+1.6.5.2\r
+\r
+\r
+--=-=-=--\r
projects / notmuch-archives.git / commitdiff