1 Return-Path: <cworth@cworth.org>
\r
2 X-Original-To: notmuch@notmuchmail.org
\r
3 Delivered-To: notmuch@notmuchmail.org
\r
4 Received: from localhost (localhost [127.0.0.1])
\r
5 by olra.theworths.org (Postfix) with ESMTP id 1D1DB431FBF;
\r
6 Fri, 20 Nov 2009 12:53:50 -0800 (PST)
\r
7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org
\r
8 Received: from olra.theworths.org ([127.0.0.1])
\r
9 by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)
\r
10 with ESMTP id efKiFtXgGhKT; Fri, 20 Nov 2009 12:53:49 -0800 (PST)
\r
11 Received: from cworth.org (localhost [127.0.0.1])
\r
12 by olra.theworths.org (Postfix) with ESMTP id 8EC7D431FAE;
\r
13 Fri, 20 Nov 2009 12:53:48 -0800 (PST)
\r
14 From: Carl Worth <cworth@cworth.org>
\r
15 To: Mike Hommey <mh+notmuch@glandium.org>, notmuch@notmuchmail.org
\r
16 In-Reply-To: <20091120132625.GA19246@glandium.org>
\r
17 References: <20091120132625.GA19246@glandium.org>
\r
18 Date: Fri, 20 Nov 2009 21:53:37 +0100
\r
19 Message-ID: <87y6m0lxym.fsf@yoom.home.cworth.org>
\r
21 Content-Type: multipart/mixed; boundary="=-=-="
\r
22 Subject: Re: [notmuch] Segfault with weird Message-ID
\r
23 X-BeenThere: notmuch@notmuchmail.org
\r
24 X-Mailman-Version: 2.1.12
\r
26 List-Id: "Use and development of the notmuch mail system."
\r
27 <notmuch.notmuchmail.org>
\r
28 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,
\r
29 <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>
\r
30 List-Archive: <http://notmuchmail.org/pipermail/notmuch>
\r
31 List-Post: <mailto:notmuch@notmuchmail.org>
\r
32 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>
\r
33 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,
\r
34 <mailto:notmuch-request@notmuchmail.org?subject=subscribe>
\r
35 X-List-Received-Date: Fri, 20 Nov 2009 20:53:50 -0000
\r
39 On Fri, 20 Nov 2009 14:26:25 +0100, Mike Hommey <mh+notmuch@glandium.org> wrote:
\r
40 > I got a segfault when importing my maildir. It happened because of an
\r
41 > old weird email, where the message-id is the following:
\r
42 > 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
43 > 218.242.202.80]) by host.warwick.net (8.10.0.Beta10/8.10.0.Beta10) with SMTP id e9GKEKk19201>
\r
45 Thanks for sharing this Mike, (and for sending me the original file).
\r
47 > Anyways, the stack dump is the following:
\r
48 > #0 0x00007ffff6d1e598 in Xapian::Document::add_term(std::string const&, unsigned int) () from /usr/lib/libxapian.so.15
\r
49 > #1 0x000000000040f5ff in _notmuch_message_add_term (message=0x0, prefix_name=0x41ad7f "tag", value=0x4191b0 "inbox") at lib/message.cc:587
\r
50 > #2 0x000000000040f827 in notmuch_message_add_tag (message=0x0, tag=0x4191b0 "inbox") at lib/message.cc:668
\r
51 > #3 0x0000000000407bc8 in tag_inbox_and_unread (message=0x0) at notmuch-new.c:44
\r
52 > #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
53 > #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
54 > #6 0x0000000000408036 in add_files_recursive (notmuch=0x62cc20, path=0x62c920 "/home/mh/Maildir", st=0x7fffffffe000, state=0x7fffffffe240) at notmuch-new.c:223
\r
55 > #7 0x0000000000408245 in add_files (notmuch=0x62cc20, path=0x62c920 "/home/mh/Maildir", state=0x7fffffffe240) at notmuch-new.c:287
\r
56 > #8 0x0000000000408704 in notmuch_new_command (ctx=0x61f140, argc=0, argv=0x7fffffffe3e8) at notmuch-new.c:431
\r
57 > #9 0x0000000000406ea8 in main (argc=2, argv=0x7fffffffe3d8) at notmuch.c:400
\r
59 I didn't get the same crash when importing the file. But I did get a
\r
60 short document out of it (just a handful of terms indexed) and most
\r
61 significantly, an empty message-ID term.
\r
63 Xapian has a limit on the maximum length of a term, so one thing we'll
\r
64 need to do here is to notice if the message ID exceeds that length and
\r
65 then treat it as a we treat a missing Message-ID header, (that is,
\r
66 generate our own message ID by computing a sha-1 hash over the message).
\r
68 So, there was an obvious bug in the message-ID handling, (the code was
\r
69 still looking for NULL for a missing header, but we now return "" for a
\r
70 missing header instead). I've fixed this.
\r
72 > Now, looking at the code, there seems to me there actually 3 problems:
\r
73 > - _notmuch_message_create_for_message_id can return NULL, and while
\r
74 > there is a test for it in notmuch_database_add_message, the function
\r
75 > still returns a success code
\r
77 Thanks. This is fixed now.
\r
79 > - things are still going on even when message is NULL in
\r
80 > add_files_recursive
\r
82 I didn't replicate this case, but it *should* be fixed now that
\r
83 notmuch_database_add_message is returning a non-success value.
\r
85 > - for some reason, xapian doesn't want to add the document corresponding
\r
86 > to this old spam message: notmuch->xapian_db->add_document throws an
\r
89 I think things had just gone wrong long before then.
\r
91 > I can provide the spam if necessary, or can continue debugging the issue
\r
92 > with some guidance.
\r
94 Thanks for providing it. It turns out that the giant Message-Id value
\r
95 wasn't causing the problem. Instead the message was corrupt by having a
\r
96 stray new line at the third line. (So GMime is seeing only the first two
\r
97 lines of headers). We *used* to have working code to detect this kind of
\r
98 file as "not an email" but again, this broke when we changed
\r
99 notmuch_message_get_header to return "" instead of NULL for missing
\r
102 See patches below (just pushed now as well) for the fixes.
\r
108 Content-Disposition: inline;
\r
109 filename=handle-corrupt-mail-and-non-mail.patchset
\r
111 >From 52292c548512214fd3dd205edb4ca9cf7955f2b3 Mon Sep 17 00:00:00 2001
\r
112 From: Carl Worth <cworth@cworth.org>
\r
113 Date: Fri, 20 Nov 2009 19:31:00 +0100
\r
114 Subject: [PATCH 1/3] add_message: Properly handle missing Message-ID once again.
\r
116 There's been a fair amount of fallout from when we changed
\r
117 message_file_get_header from returning NULL to returning "" for
\r
118 missing headers. This is yet more fallout from that, (where we were
\r
119 accepting an empty message-ID rather than generating one like we want
\r
122 lib/database.cc | 2 +-
\r
123 1 files changed, 1 insertions(+), 1 deletions(-)
\r
125 diff --git a/lib/database.cc b/lib/database.cc
\r
126 index 726c5a9..294247e 100644
\r
127 --- a/lib/database.cc
\r
128 +++ b/lib/database.cc
\r
129 @@ -908,7 +908,7 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
\r
130 * is to find a message ID (or else create one ourselves). */
\r
132 header = notmuch_message_file_get_header (message_file, "message-id");
\r
134 + if (header && *header != '\0') {
\r
135 message_id = _parse_message_id (message_file, header, NULL);
\r
136 /* So the header value isn't RFC-compliant, but it's
\r
137 * better than no message-id at all. */
\r
142 >From 656e4c413d84984dcc5fbd8016907ed03c343cb8 Mon Sep 17 00:00:00 2001
\r
143 From: Carl Worth <cworth@cworth.org>
\r
144 Date: Fri, 20 Nov 2009 21:02:11 +0100
\r
145 Subject: [PATCH 2/3] notmuch_database_add_message: Add missing error-value propagation.
\r
147 Thanks to Mike Hommey for doing the analysis that led to noticing that
\r
150 lib/database.cc | 5 ++++-
\r
151 1 files changed, 4 insertions(+), 1 deletions(-)
\r
153 diff --git a/lib/database.cc b/lib/database.cc
\r
154 index 294247e..58a350d 100644
\r
155 --- a/lib/database.cc
\r
156 +++ b/lib/database.cc
\r
157 @@ -940,8 +940,11 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
\r
159 talloc_free (message_id);
\r
161 - if (message == NULL)
\r
162 + if (message == NULL) {
\r
163 + ret = COERCE_STATUS (private_status,
\r
164 + "Unexpected status value from _notmuch_message_create_for_message_id");
\r
168 /* Is this a newly created message object? */
\r
169 if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {
\r
174 >From 3ae12b1e286d1c0041a2e3957cb01daa2981dad9 Mon Sep 17 00:00:00 2001
\r
175 From: Carl Worth <cworth@cworth.org>
\r
176 Date: Fri, 20 Nov 2009 21:46:37 +0100
\r
177 Subject: [PATCH 3/3] add_message: Re-fix handling of non-mail files.
\r
179 More fallout from _get_header now returning "" for missing headers.
\r
181 The bug here is that we would no longer detect that a file is not an
\r
182 email message and give up on it like we should.
\r
184 And this time, I actually audited all callers to
\r
185 notmuch_message_get_header, so hopefully we're done fixing this
\r
188 lib/database.cc | 10 +++++-----
\r
189 lib/message.cc | 2 +-
\r
190 2 files changed, 6 insertions(+), 6 deletions(-)
\r
192 diff --git a/lib/database.cc b/lib/database.cc
\r
193 index 58a350d..207246c 100644
\r
194 --- a/lib/database.cc
\r
195 +++ b/lib/database.cc
\r
196 @@ -323,7 +323,7 @@ _parse_message_id (void *ctx, const char *message_id, const char **next)
\r
197 const char *s, *end;
\r
200 - if (message_id == NULL)
\r
201 + if (message_id == NULL || *message_id == '\0')
\r
205 @@ -391,7 +391,7 @@ parse_references (void *ctx,
\r
209 - if (refs == NULL)
\r
210 + if (refs == NULL || *refs == '\0')
\r
214 @@ -896,9 +896,9 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
\r
215 subject = notmuch_message_file_get_header (message_file, "subject");
\r
216 to = notmuch_message_file_get_header (message_file, "to");
\r
218 - if (from == NULL &&
\r
219 - subject == NULL &&
\r
221 + if ((from == NULL || *from == '\0') &&
\r
222 + (subject == NULL || *subject == '\0') &&
\r
223 + (to == NULL || *to == '\0'))
\r
225 ret = NOTMUCH_STATUS_FILE_NOT_EMAIL;
\r
227 diff --git a/lib/message.cc b/lib/message.cc
\r
228 index 41dddd0..e0b8a8e 100644
\r
229 --- a/lib/message.cc
\r
230 +++ b/lib/message.cc
\r
231 @@ -491,7 +491,7 @@ _notmuch_message_set_date (notmuch_message_t *message,
\r
233 /* GMime really doesn't want to see a NULL date, so protect its
\r
234 * sensibilities. */
\r
235 - if (date == NULL)
\r
236 + if (date == NULL || *date == '\0')
\r
239 time_value = g_mime_utils_header_decode_date (date, NULL);
\r