Last Comment Bug 495057 - Crash [@ push_tag] or [@ real_write] if a message with text/enriched part is viewed
: Crash [@ push_tag] or [@ real_write] if a message with text/enriched part is ...
Status: RESOLVED FIXED
[sg:critical?] (assuming worst case)
: crash, testcase, verified1.8.1.22
Product: MailNews Core
Classification: Components
Component: MIME (show other bugs)
: Trunk
: All All
: -- critical (vote)
: Thunderbird 3.0b3
Assigned To: David :Bienvenu
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-27 07:03 PDT by Bernd Jendrissek
Modified: 2011-06-13 10:01 PDT (History)
13 users (show)
dveditz: wanted1.8.1.x+
bugmail: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Reduced sample of crash-inducing message (mbox format) (2.71 KB, application/octet-stream)
2009-05-27 07:05 PDT, Bernd Jendrissek
no flags Details
crash stack (60.48 KB, text/plain)
2009-05-28 02:36 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
more gdb debug info (11.40 KB, text/plain)
2009-05-28 02:49 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
Even more reduced .eml testcase (998 bytes, text/plain)
2009-05-28 03:36 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
fully reduced 9-line .eml testcase (186 bytes, text/plain)
2009-05-28 04:04 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
proposed fix (724 bytes, patch)
2009-05-31 07:56 PDT, David :Bienvenu
bugmail: review-
Details | Diff | Splinter Review
unit test for text/enriched for the js mime emitter (7.22 KB, patch)
2009-06-01 00:12 PDT, Andrew Sutherland [:asuth]
no flags Details | Diff | Splinter Review
proposed fix (2.84 KB, patch)
2009-06-01 08:38 PDT, David :Bienvenu
bugmail: review+
standard8: superreview+
dveditz: approval1.8.1.next+
Details | Diff | Splinter Review
unit test with \r normalization, why not (7.26 KB, patch)
2009-06-01 09:42 PDT, Andrew Sutherland [:asuth]
mozilla: review+
Details | Diff | Splinter Review

Description Bernd Jendrissek 2009-05-27 07:03:54 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.5) Gecko/2008121621 Ubuntu/8.04 (hardy) Firefox/3.0.10
Build Identifier: Mozilla Thunderbird version 2.0.0.19 (20090105)

A message with this structure:

multipart/mixed
  multipart/related
    multipart/alternative
      text/plain
      text/html
      text/enriched

kills Thunderbird every time.  Let me see if I can attach the actual example...

Reproducible: Always

Steps to Reproduce:
1. Append the attached example to the POP server's mbox folder where it keeps my mail.
2. Hit "Get Mail" in Thunderbird
Actual Results:  
Thunderbird dies, and keeps dying if I log into the POP server.

Expected Results:  
Not crashed!  Display the message, whichever of the alternates it favours.

My colleague's Windows installation (version 2.0.0.12 (20080708)) also dies when she views the evil message.

The only way I can recover is if I hit "Cancel" on the server login prompt, then right-click + delete message (WITHOUT selecting the evil message which would preview it).  Also I have to delete the message from the mbox file.  (vim + :/^From /,$d == win)
Comment 1 Bernd Jendrissek 2009-05-27 07:05:51 PDT
Created attachment 379880 [details]
Reduced sample of crash-inducing message (mbox format)
Comment 2 Gary Kwong [:gkw] [:nth10sd] 2009-05-28 02:36:22 PDT
Created attachment 380081 [details]
crash stack

Not yet checked for a dupe...

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090527 Lightning/1.0pre Shredder/3.0b3pre
Comment 3 Gary Kwong [:gkw] [:nth10sd] 2009-05-28 02:49:51 PDT
Created attachment 380085 [details]
more gdb debug info
Comment 4 Gary Kwong [:gkw] [:nth10sd] 2009-05-28 03:36:02 PDT
Created attachment 380090 [details]
Even more reduced .eml testcase
Comment 5 Gary Kwong [:gkw] [:nth10sd] 2009-05-28 04:04:36 PDT
Created attachment 380095 [details]
fully reduced 9-line .eml testcase

I managed to fully reduce this testcase using Lithium on Ubuntu Linux.

Basically, needed to open a .eml testcase using CLI via

./thunderbird-bin a.eml

and also needed to get some ./thunderbird script env variables and disabled the crash reporter as well.
Comment 6 Gary Kwong [:gkw] [:nth10sd] 2009-05-28 04:18:09 PDT
(In reply to comment #5)
> Created an attachment (id=380095) [details]
> fully reduced 9-line .eml testcase

Mac Breakpad report:

bp-01afd746-6ceb-4fed-b1e4-98e292090528
Comment 7 Gary Kwong [:gkw] [:nth10sd] 2009-05-28 04:55:24 PDT
(In reply to comment #5)
> Created an attachment (id=380095) [details]
> fully reduced 9-line .eml testcase

From the breakpad crash report, I set breakpoints at/near http://hg.mozilla.org/comm-central/index.cgi/annotate/58cf9794dd27/mailnews/mime/src/mimemrel.cpp#l770, opened the testcase ...

... and found that lobj->options is null.

=====

Breakpoint 2, real_write (relobj=0x11f38b00, buf=0x16ecc200 "-\n", size=2) at /Users/skywalker/comm-central/mailnews/mime/src/mimemrel.cpp:768
768         if (!closure) {
(gdb) p closure
$1 = (void *) 0x0
(gdb) n

Breakpoint 1, real_write (relobj=0x11f38b00, buf=0x16ecc200 "-\n", size=2) at /Users/skywalker/comm-central/mailnews/mime/src/mimemrel.cpp:770
770           closure = lobj->options->stream_closure;
(gdb) l
765       else
766     #endif /* MIME_DRAFTS */
767       {
768         if (!closure) {
769           MimeObject* lobj = (MimeObject*) relobj;
770           closure = lobj->options->stream_closure;
771         }
772         return relobj->real_output_fn(buf, size, closure);
773       }
774     }
(gdb) p lobj->options
$6 = (class MimeDisplayOptions *) 0x0
(gdb) i b
Num Type           Disp Enb Address    What
1   breakpoint     keep y   0x146f16e8 in real_write at /Users/skywalker/comm-central/mailnews/mime/src/mimemrel.cpp:770
        breakpoint already hit 1 time
2   breakpoint     keep y   0x146f16dc in real_write at /Users/skywalker/comm-central/mailnews/mime/src/mimemrel.cpp:768
        breakpoint already hit 1 time
3   breakpoint     keep y   0x146f16f4 in real_write at /Users/skywalker/comm-central/mailnews/mime/src/mimemrel.cpp:772
4   breakpoint     keep y   0x146f172d in push_tag at /Users/skywalker/comm-central/mailnews/mime/src/mimemrel.cpp:780
(gdb)
Comment 8 Andrew Sutherland [:asuth] 2009-05-28 13:42:49 PDT
This sounds like it could be the reproducible test case we wanted for bug 482879.
Comment 9 David :Bienvenu 2009-05-28 17:04:18 PDT
maybe the cause is the same, but the crash is different - in this case, relobj has been deleted already
Comment 10 David :Bienvenu 2009-05-29 15:00:28 PDT
I'm less and less convinced this is related to bug 482979 - reverting mimemrel.cpp to the version prior to the mimemrel.cpp changes referred to in that bug doesn't help with this crash.
Comment 11 David :Bienvenu 2009-05-29 15:03:26 PDT
this crashes 2.0, and it's possible there's a free memory read going on...so I marked it security sensitive pending investigation.
Comment 12 David :Bienvenu 2009-05-29 16:45:12 PDT
I suspect there's something something specifically bad in the mime text enriched class - changing that part to some other class fixes the crash.

N.B. - opening the message as a folder instead of as a .eml file produces a different crash...
Comment 13 David :Bienvenu 2009-05-31 07:56:48 PDT
Created attachment 380711 [details] [diff] [review]
proposed fix

This fixes the crash for me - the effect of the fix is to pass in the MimeMultipartRelated*  object, which is what mime_multipart_related_output_fn expects, not the stream_closure object, which is a different object (I've forgotten what it is...). This is what other text classes do, use the output_closure.

We need a test case for this, and we do see a bogus part 1.1.1.1 in the attachment area.

In the bigger picture, it's worth investigating if we really care about supporting text/enriched - I wonder if anyone ever generates it anymore.
Comment 14 Andrew Sutherland [:asuth] 2009-06-01 00:12:17 PDT
Created attachment 380783 [details] [diff] [review]
unit test for text/enriched for the js mime emitter

Here is your unit test...
Comment 15 Andrew Sutherland [:asuth] 2009-06-01 00:14:03 PDT
Comment on attachment 380711 [details] [diff] [review]
proposed fix

This patch now crashes on a message that is just "text/enriched", see the unit test.  I presume something inside the conversion logic was assuming things would be messed up.
Comment 16 Bernd Jendrissek 2009-06-01 04:18:53 PDT
> In the bigger picture, it's worth investigating if we really care about
> supporting text/enriched - I wonder if anyone ever generates it anymore.

We got this text/enriched part from MacMail / Apple Mail; apparently it's still the default mail client on Macs.  We keep it around here at work for testing our "branding server".

(In reply to comment #13)
Comment 17 David :Bienvenu 2009-06-01 07:50:10 PDT
Thx, Andrew. 

The reason this broke the non-multipart related case is that, in that case, the output_fn that MimeRichtextConvert calls (plain ol' mime_output_fn) expect the closure to be mime_stream_data. But in the mime multipart related case, it expects it to be the MimeMultipartRelated*  object. Who knows how the other text/type handlers handle this...The fix has to be in mimetric.cpp so that I don't break anything else. One possible avenue is this comment above MimeRichtextConvert:
/* This function has this clunky interface because it needs to be called
   from outside this module (no MimeObject, etc.)
 */

As near as I can tell, this is not true anymore, so perhaps we can switch it to something more like the other text classes.
Comment 18 David :Bienvenu 2009-06-01 08:16:48 PDT
Comment on attachment 380711 [details] [diff] [review]
proposed fix

better patch coming up.
Comment 19 David :Bienvenu 2009-06-01 08:38:32 PDT
Created attachment 380836 [details] [diff] [review]
proposed fix

This passes asuth's tests.

I tested reply for both cases (single part and multi-part related) and it worked. Edit as draft didn't work for either (not surprising since the mime rich text code is so old it doesn't seem to know about edit as draft), but it didn't crash, and it does work in the same way that the code w/o this patch doesn't work.
Comment 20 David :Bienvenu 2009-06-01 08:40:17 PDT
argh, it doesn't pass asuth's test, but it doesn't crash his test either...

TEST-UNEXPECTED-FAIL | C:\mozilla-build\msys\tbirdhq\objdir-tb\mozilla\_tests\xpcshell\test_mailnewsglobaldb\unit\test_mime_emitter.js
| test failed (with xpcshell return code: 0), see following log:
*** TEST-UNEXPECTED-FAIL | C:\mozilla-build\msys\tbirdhq\objdir-tb\mozilla\_tests\xpcshell\test_mailnewsglobaldb\unit\test_mime_emitter
.js | <B><I>I am not a popular format! sad woo :(</I></B>
*** TEST-UNEXPECTED-FAIL | C:\mozilla-build\msys\tbirdhq\objdir-tb\mozilla\_tests\xpcshell\test_mailnewsglobaldb\unit\test_mime_emitter
.js | 2147500036
Comment 21 David :Bienvenu 2009-06-01 09:15:06 PDT
====== Test function: test_stream_message Parameter: text/enriched
mime part body = I am not a popular format! sad woo
<BR>
sync part = I am not a popular format! sad woo
<BR>
*** exiting

I added some dump statements and simplified the msg body a little - I can't tell how they differ, so it looks to be some sort of whitespace difference in the test.
Comment 22 Andrew Sutherland [:asuth] 2009-06-01 09:42:57 PDT
Created attachment 380845 [details] [diff] [review]
unit test with \r normalization, why not

So, the unit tests passes for me with your patch.  My assumption is that the whitespace is platform-dependent, using \r's on windows because that is a windows thing.  This change attempts to normalize them out so the test passes for you.
Comment 23 David :Bienvenu 2009-06-01 10:04:47 PDT
yes, that would fix it for me, since adding the \r fixed it while I was trying to debug it ;-) I also had to remove a trailing space in the body part to match the trim(), but I may have introduced that by accident...
Comment 24 David :Bienvenu 2009-06-01 10:07:06 PDT
Comment on attachment 380845 [details] [diff] [review]
unit test with \r normalization, why not

yup, this passes for me too, thx!
Comment 25 David :Bienvenu 2009-06-01 13:42:32 PDT
fix checked in.
Comment 26 Daniel Veditz [:dveditz] 2009-06-01 14:24:19 PDT
Comment on attachment 380836 [details] [diff] [review]
proposed fix

Approved for 1.8.1.22, a=dveditz for release-drivers

Please land today (now-ish -- we're way late on this release.)
Comment 27 David :Bienvenu 2009-06-01 15:49:56 PDT
fix on 1.8.1 branch
Comment 28 Gary Kwong [:gkw] [:nth10sd] 2009-06-02 02:53:41 PDT
bienvenu / asuth, did the unit test get checked in as well?

(the in-testsuite? flag should then be changed to a + )
Comment 29 Samuel Sidler (old account; do not CC) 2009-06-02 08:15:20 PDT
(In reply to comment #28)
> bienvenu / asuth, did the unit test get checked in as well?

Probably shouldn't be until the fix is shipped... That's the process we normally use for security bugs anyway.
Comment 30 Al Billings [:abillings] 2009-06-02 14:49:53 PDT
Marking verified for 1.8.1 based on Bienvenu's comments.
Comment 31 Bernd Jendrissek 2010-04-16 09:46:04 PDT
I'm running 2.0.0.23 now and can confirm that my original attachment no longer crashes Thunderbird.  Thanks!
Comment 32 Andrew Sutherland [:asuth] 2011-02-11 03:10:21 PST
While cleaning out my patch queue I noticed that test indeed never landed.

test de-bitrotted and pushed to trunk:
http://hg.mozilla.org/comm-central/rev/be97fd7c7eee

Note You need to log in before you can comment on or make changes to this bug.