04/4933e7d9e07d7169cb02c74d5ba24cbd74bb57

   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
  20 MIME-Version: 1.0\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
  25 Precedence: list\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
  36 \r
  37 --=-=-=\r
  38 \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
  44 \r
  45 Thanks for sharing this Mike, (and for sending me the original file).\r
  46 \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
  58 \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
  62 \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
  67 \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
  71 \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
  76 \r
  77 Thanks. This is fixed now.\r
  78 \r
  79 > - things are still going on even when message is NULL in\r
  80 >   add_files_recursive\r
  81 \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
  84 \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
  87 >   exception.\r
  88 \r
  89 I think things had just gone wrong long before then.\r
  90 \r
  91 > I can provide the spam if necessary, or can continue debugging the issue\r
  92 > with some guidance.\r
  93 \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
 100 headers.\r
 101 \r
 102 See patches below (just pushed now as well) for the fixes.\r
 103 \r
 104 -Carl\r
 105 \r
 106 \r
 107 --=-=-=\r
 108 Content-Disposition: inline;\r
 109  filename=handle-corrupt-mail-and-non-mail.patchset\r
 110 \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
 115 \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
 120 to).\r
 121 ---\r
 122  lib/database.cc |    2 +-\r
 123  1 files changed, 1 insertions(+), 1 deletions(-)\r
 124 \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
 131  \r
 132         header = notmuch_message_file_get_header (message_file, "message-id");\r
 133 -       if (header) {\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
 138 -- \r
 139 1.6.5.2\r
 140 \r
 141 \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
 146 \r
 147 Thanks to Mike Hommey for doing the analysis that led to noticing that\r
 148 this was missing.\r
 149 ---\r
 150  lib/database.cc |    5 ++++-\r
 151  1 files changed, 4 insertions(+), 1 deletions(-)\r
 152 \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
 158  \r
 159         talloc_free (message_id);\r
 160  \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
 165             goto DONE;\r
 166 +       }\r
 167  \r
 168         /* Is this a newly created message object? */\r
 169         if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {\r
 170 -- \r
 171 1.6.5.2\r
 172 \r
 173 \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
 178 \r
 179 More fallout from _get_header now returning "" for missing headers.\r
 180 \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
 183 \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
 186 bug over and over.\r
 187 ---\r
 188  lib/database.cc |   10 +++++-----\r
 189  lib/message.cc  |    2 +-\r
 190  2 files changed, 6 insertions(+), 6 deletions(-)\r
 191 \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
 198      char *result;\r
 199  \r
 200 -    if (message_id == NULL)\r
 201 +    if (message_id == NULL || *message_id == '\0')\r
 202         return NULL;\r
 203  \r
 204      s = message_id;\r
 205 @@ -391,7 +391,7 @@ parse_references (void *ctx,\r
 206  {\r
 207      char *ref;\r
 208  \r
 209 -    if (refs == NULL)\r
 210 +    if (refs == NULL || *refs == '\0')\r
 211         return;\r
 212  \r
 213      while (*refs) {\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
 217  \r
 218 -       if (from == NULL &&\r
 219 -           subject == NULL &&\r
 220 -           to == NULL)\r
 221 +       if ((from == NULL || *from == '\0') &&\r
 222 +           (subject == NULL || *subject == '\0') &&\r
 223 +           (to == NULL || *to == '\0'))\r
 224         {\r
 225             ret = NOTMUCH_STATUS_FILE_NOT_EMAIL;\r
 226             goto DONE;\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
 232  \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
 237         time_value = 0;\r
 238      else\r
 239         time_value = g_mime_utils_header_decode_date (date, NULL);\r
 240 -- \r
 241 1.6.5.2\r
 242 \r
 243 \r
 244 --=-=-=--\r