Closed Bug 495057 Opened 15 years ago Closed 15 years ago

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

Categories

(MailNews Core :: MIME, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: bernd.jendrissek+mozilla, Assigned: Bienvenu)

Details

(Keywords: crash, testcase, verified1.8.1.22, Whiteboard: [sg:critical?] (assuming worst case))

Crash Data

Attachments

(5 files, 4 obsolete files)

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)
Attached file 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
Severity: normal → critical
Flags: blocking-thunderbird3?
Keywords: crash, testcase
OS: Linux → All
Hardware: x86 → All
Summary: Crash if I download or view a message with text/enriched part → Crash [@ push_tag] if I download or view a message with text/enriched part
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Message Reader UI → MIME
Product: Thunderbird → MailNews Core
QA Contact: message-reader → mime
Attached file Even more reduced .eml testcase (obsolete) —
Attachment #379880 - Attachment is obsolete: true
Attachment #380090 - Attachment mime type: message/rfc822 → text/plain
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.
Attachment #380090 - Attachment is obsolete: true
(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
Summary: Crash [@ push_tag] if I download or view a message with text/enriched part → Crash [@ push_tag] or [@ real_write] if a message with text/enriched part is viewed
(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)
This sounds like it could be the reproducible test case we wanted for bug 482879.
maybe the cause is the same, but the crash is different - in this case, relobj has been deleted already
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.
Group: core-security
this crashes 2.0, and it's possible there's a free memory read going on...so I marked it security sensitive pending investigation.
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...
Attached patch proposed fix (obsolete) — Splinter Review
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.
Assignee: nobody → bienvenu
Attachment #380711 - Flags: superreview?(bugzilla)
Attachment #380711 - Flags: review?(bugmail)
Here is your unit test...
Attachment #380783 - Flags: review?(bienvenu)
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.
Attachment #380711 - Flags: review?(bugmail) → review-
> 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)
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 on attachment 380711 [details] [diff] [review]
proposed fix

better patch coming up.
Attachment #380711 - Attachment is obsolete: true
Attachment #380711 - Flags: superreview?(bugzilla)
Attached patch proposed fixSplinter Review
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.
Attachment #380836 - Flags: superreview?(bugzilla)
Attachment #380836 - Flags: review?(bugmail)
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
====== 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.
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.
Attachment #380783 - Attachment is obsolete: true
Attachment #380845 - Flags: review?(bienvenu)
Attachment #380783 - Flags: review?(bienvenu)
Attachment #380836 - Flags: review?(bugmail) → review+
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 on attachment 380845 [details] [diff] [review]
unit test with \r normalization, why not

yup, this passes for me too, thx!
Attachment #380845 - Flags: review?(bienvenu) → review+
Attachment #380836 - Flags: superreview?(bugzilla) → superreview+
Attachment #380836 - Flags: approval1.8.1.next?
fix checked in.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: blocking-thunderbird3?
Target Milestone: --- → Thunderbird 3.0b3
Flags: wanted1.8.1.x+
Whiteboard: [sg:critical?] (assuming worst case)
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.)
Attachment #380836 - Flags: approval1.8.1.next? → approval1.8.1.next+
fix on 1.8.1 branch
Keywords: fixed1.8.1.22
bienvenu / asuth, did the unit test get checked in as well?

(the in-testsuite? flag should then be changed to a + )
(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.
Marking verified for 1.8.1 based on Bienvenu's comments.
Group: core-security
I'm running 2.0.0.23 now and can confirm that my original attachment no longer crashes Thunderbird.  Thanks!
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
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ push_tag] [@ real_write]
You need to log in before you can comment on or make changes to this bug.