495057 - Crash [@ push_tag] or [@ real_write] if a message with text/enriched part is viewed

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Bernd Jendrissek]

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]

Attachment: Reduced sample of crash-inducing message (mbox format)

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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] (NOT official MoCo now)]

Attachment: more gdb debug info

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Even more reduced .eml testcase

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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] (NOT official MoCo now)]

(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] (NOT official MoCo now)]

(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] (he/him)]

This sounds like it could be the reproducible test case we wanted for bug 482879.

Comment 9

[David :Bienvenu]

maybe the cause is the same, but the crash is different - in this case, relobj has been deleted already

Comment 10

[David :Bienvenu]

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]

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]

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]

Attachment: 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] (he/him)]

Attachment: unit test for text/enriched for the js mime emitter

Here is your unit test...

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

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]

> 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]

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]

Comment on attachment 380711 [details] [diff] [review]
proposed fix

better patch coming up.

Comment 19

[David :Bienvenu]

Attachment: 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]

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]

====== 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] (he/him)]

Attachment: 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]

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]

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]

fix checked in.

Comment 26

[Daniel Veditz [:dveditz]]

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]

fix on 1.8.1 branch

Comment 28

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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)]

(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 - ex-MoCo]]

Marking verified for 1.8.1 based on Bienvenu's comments.

Comment 31

[Bernd Jendrissek]

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] (he/him)]

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