Closed Bug 513543 Opened 15 years ago Closed 15 years ago

Crash [@ MimeInlineTextHTML_parse_eof ]

Categories

(MailNews Core :: MIME, defect)

Product:
MailNews Core
Component:
MIME
Version:
1.9.1 Branch
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(blocking-thunderbird3.0 .1+, thunderbird3.0 .1-fixed)

Status:
VERIFIED FIXED
Milestone:
Thunderbird 3.1a1
Tracking Flags:
Tracking Status
blocking-thunderbird3.0 --- .1+
thunderbird3.0 --- .1-fixed

People

(Reporter: Usul, Assigned: asuth)

Details

(4 keywords, Whiteboard: [needs verification post 3.0.1])

Crash Data

Attachments

(8 files, 1 obsolete file)

this fixes the crash for the test case I have
2.27 KB, patch
Details | Diff | Splinter Review
crashing test message
1.62 KB, text/plain
Details
add test case - checked in
7.36 KB, patch
asuth
: review+
neil
: superreview+
standard8
: approval-thunderbird3.0.1+
Details | Diff | Splinter Review
chroniquery execution trace of test_mime_emitter.js for a synthetic approximation of the crasher
390.22 KB, text/html
Details
mbox file for synthetic message created by unit tests; corresponds to crasher
1.25 KB, text/plain
Details
chroniquery trace of a good case for differential analysis
394.18 KB, text/html
Details
v1 don't close a child that is already closed in jerky empty multipart sections
1.13 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Bienvenu
: approval-thunderbird3.0.1+
Details | Diff | Splinter Review
v1 unit test that creates a jerky message
9.49 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review 
0  	thunderbird.exe  	MimeInlineTextHTML_parse_eof  	mailnews/mime/src/mimethtm.cpp:236
1 	thunderbird.exe 	MimeMultipart_close_child 	mailnews/mime/src/mimemult.cpp:623
2 	thunderbird.exe 	MimeMultipart_parse_line 	mailnews/mime/src/mimemult.cpp:185
3 	thunderbird.exe 	convert_and_send_buffer 	mailnews/mime/src/mimebuf.cpp:184
4 	thunderbird.exe 	mime_LineBuffer 	mailnews/mime/src/mimebuf.cpp:272
5 	thunderbird.exe 	MimeObject_parse_buffer 	mailnews/mime/src/mimeobj.cpp:275
6 	thunderbird.exe 	MimeMultipartRelated_parse_eof 	mailnews/mime/src/mimemrel.cpp:1100
7 	thunderbird.exe 	MimeMultipart_parse_eof 	mailnews/mime/src/mimemult.cpp:773
8 	thunderbird.exe 	MimeMultipart_finalize 	mailnews/mime/src/mimemult.cpp:133
9 	thunderbird.exe 	mime_free 	mailnews/mime/src/mimei.cpp:328
10 	thunderbird.exe 	MimeContainer_finalize 	mailnews/mime/src/mimecont.cpp:109
11 	thunderbird.exe 	MimeMessage_finalize 	mailnews/mime/src/mimemsg.cpp:122
12 	thunderbird.exe 	mime_free 	mailnews/mime/src/mimei.cpp:328
13 	thunderbird.exe 	mime_display_stream_complete 	mailnews/mime/src/mimemoz2.cpp:992
14 	thunderbird.exe 	nsStreamConverter::OnStopRequest 	mailnews/mime/src/nsStreamConverter.cpp:1068
15 	thunderbird.exe 	nsMsgProtocol::OnStopRequest 	mailnews/base/util/nsMsgProtocol.cpp:393
16 	thunderbird.exe 	nsMailboxProtocol::OnStopRequest 	mailnews/local/src/nsMailboxProtocol.cpp:380
17 	thunderbird.exe 	nsInputStreamPump::OnStateStop 	netwerk/base/src/nsInputStreamPump.cpp:576
18 	thunderbird.exe 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:401
19 	xpcom_core.dll 	nsOutputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:111
20 	xpcom_core.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
21 	xpcom_core.dll 	NS_ProcessNextEvent_P 	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:227
22 	thunderbird.exe 	nsXULWindow::ShowModal 	xpfe/appshell/src/nsXULWindow.cpp:415
23 	thunderbird.exe 	nsContentTreeOwner::ShowAsModal 	xpfe/appshell/src/nsContentTreeOwner.cpp:528
24 	thunderbird.exe 	nsWindowWatcher::OpenWindowJSInternal 	embedding/components/windowwatcher/src/nsWindowWatcher.cpp:992
25 	thunderbird.exe 	nsWindowWatcher::OpenWindowJS 	embedding/components/windowwatcher/src/nsWindowWatcher.cpp:487
26 	thunderbird.exe 	nsGlobalWindow::OpenInternal 	dom/src/base/nsGlobalWindow.cpp:7339
27 	thunderbird.exe 	nsGlobalWindow::OpenDialog 	dom/src/base/nsGlobalWindow.cpp:5152
28 	xpcom_core.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
29 	thunderbird.exe 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2295
30 	thunderbird.exe 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1590
31 	js3250.dll 	js_Invoke 	js/src/jsinterp.cpp:1386
#6 crash for 3.0b4pre, but i have my doubts this would develop into a 3.0b4 topcrash

brief search indicates there are only 3.0b2 and 3.0b4pre crashes. no 3.0b3
hmm, a ton of crashes with the 20090911031354 build, but that seems to have backed off.  still, it's now #1 crash using 3 day range.
Whiteboard: [check crash-stats after 3.0b4]
#4 crash. certifiable 3.0b4 topcrash IMO
Flags: blocking-thunderbird3?
Keywords: topcrash
Whiteboard: [check crash-stats after 3.0b4]
Whiteboard: [no l10n impact]
Assignee: nobody → bienvenu
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Keywords: qawanted
It's very possible that the fix for bug 505221 will also fix this issue. We'll have to keep an eye on crash-stats.
Whiteboard: [no l10n impact] → [no l10n impact][possibly fixed by fix for bug 505221]
I haven't seen this in crash-stats for the four days after the fix for bug 505221 landed, so I'm going to tentatively mark this fixed, and try to keep an eye out for this stack on crash-stats going forward.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0rc1
probably true, although the crash appearance varies widely - sometimes as often as 5 days apart.  last crash so far is 20091014 build.   when 3.0rc1 ships, QA will want to verify all fixed bugs marked topcrash haven't reappeared
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [no l10n impact][possibly fixed by fix for bug 505221] → [no l10n impact]
Status: REOPENED → NEW
Summary: Crash [@MimeInlineTextHTML_parse_eof ] → Crash [@ MimeInlineTextHTML_parse_eof ]
Resetting blocking+ to blocking? - this was on a bug that was marked fixed and has now been reopened.
Flags: blocking-thunderbird3+ → blocking-thunderbird3?
Target Milestone: Thunderbird 3.0rc1 → ---
Not going to block because we don't have a test case.
Flags: blocking-thunderbird3? → blocking-thunderbird3-
suggest looking at checkins in days prior to 2009082300.  Nothing really stands out for me except  Bug 511602 -  import labs'/myk's StringBundle.js JS module into mailnews/core/utils

I optimistically suggest this is triggered either by a regression or a significant feature change/addition, because 2009082300, 2009082400, 2009082600 3.0b4pre builds (and thereafter) all register crashes.  ref: http://crash-stats.mozilla.com/report/list?product=Thunderbird&query_search=signature&query_type=exact&query=MimeInlineTextHTML_parse_eof&date=9%2F1%2F2009&range_value=4&range_unit=weeks&do_query=1&signature=MimeInlineTextHTML_parse_eof  (but afaict no crashes for *nightlies* of June, july and most of august)

Crashes continue steadily in September nightlies
 http://crash-stats.mozilla.com/report/list?product=Thunderbird&query_search=signature&query_type=exact&query=MimeInlineTextHTML_parse_eof&date=10%2F1%2F2009&range_value=4&range_unit=weeks&do_query=1&signature=MimeInlineTextHTML_parse_eof

Note 2009071500 (3.0b3) does show crashes (as in the above link)

as usual crash comments are rare ...

indexing was in progress
bp-39e4d893-2da4-417e-921d-5a5eb2091125
0	thunderbird.exe	MimeInlineTextHTML_parse_eof	 mailnews/mime/src/mimethtm.cpp:236
1	thunderbird.exe	MimeMultipart_close_child	mailnews/mime/src/mimemult.cpp:623
2	thunderbird.exe	MimeMultipart_parse_line	mailnews/mime/src/mimemult.cpp:185
3	thunderbird.exe	convert_and_send_buffer	mailnews/mime/src/mimebuf.cpp:184
4	thunderbird.exe	mime_LineBuffer	mailnews/mime/src/mimebuf.cpp:272
5	thunderbird.exe	MimeObject_parse_buffer	mailnews/mime/src/mimeobj.cpp:275
6	thunderbird.exe	MimeMultipartRelated_parse_eof	mailnews/mime/src/mimemrel.cpp:1100
7	thunderbird.exe	MimeMultipart_parse_eof	mailnews/mime/src/mimemult.cpp:773
8	thunderbird.exe	MimeMultipart_finalize	mailnews/mime/src/mimemult.cpp:133
9	thunderbird.exe	mime_free	mailnews/mime/src/mimei.cpp:322
10	thunderbird.exe	MimeContainer_finalize	mailnews/mime/src/mimecont.cpp:109
11	thunderbird.exe	MimeMessage_finalize	mailnews/mime/src/mimemsg.cpp:122
12	thunderbird.exe	mime_free	mailnews/mime/src/mimei.cpp:322
13	thunderbird.exe	mime_display_stream_complete	mailnews/mime/src/mimemoz2.cpp:992 


slightly different line#/stack here with 3.0b2 crash ...
bp-cc5b678b-4dfc-4539-9fd3-c18f92090319
Trying to view a large IMAP message. This is a message that used to take an inordinately long time to view on Thunderbird 2 (~10 mins to display a 7Mb message on a LAN)
0	thunderbird.exe	MimeInlineTextHTML_parse_eof	 mimethtm.cpp:236
1	thunderbird.exe	MimeMultipartRelated_parse_eof	mimemrel.cpp:1136
2	thunderbird.exe	MimeMultipart_close_child	mimemult.cpp:616
3	thunderbird.exe	MimeMultipart_parse_line	mimemult.cpp:185
4	thunderbird.exe	convert_and_send_buffer	mimebuf.cpp:184
5	thunderbird.exe	mime_LineBuffer	mimebuf.cpp:272
6	thunderbird.exe	MimeObject_parse_buffer	mimeobj.cpp:275
7	thunderbird.exe	MimeMessage_parse_line	mimemsg.cpp:232
8	thunderbird.exe	convert_and_send_buffer	mimebuf.cpp:184
9	thunderbird.exe	mime_LineBuffer	mimebuf.cpp:272
10	thunderbird.exe	MimeObject_parse_buffer	mimeobj.cpp:275
11	thunderbird.exe	mime_display_stream_write	mimemoz2.cpp:946
12	thunderbird.exe	nsStreamConverter::OnDataAvailable	nsStreamConverter.cpp:938
13	thunderbird.exe	nsDocumentOpenInfo::OnDataAvailable	uriloader/base/nsURILoader.cpp:306
Keywords: regression, regressionwindow-wanted
person at http://forums.mozillazine.org/viewtopic.php?f=31&t=1636505&p=8184515#p8184515 says "doesn't appear if i uncheck gloda"
Keywords: testcase
HI,

On irc, wsmwk tell me to send reply here...
So, TB3 crash every time I check global indexing options.
What could I do, to help you?
We need to figure out which message is making Thunderbird crash.
Are you using imap ? Pop ?
Another thing you can do is zip your profile and send it to either binevenu or andrew sutherland (they are on the cc list here) .
I use 7 pop3, 1 imap.
3Gb of mail.
You could try using the glodaquilla extension to see what's indexed and what's not, and use the activity manager to see which folder gloda is indexing. Then you might be able to narrow down which message is causing the crash.
You can also follow step two at https://wiki.mozilla.org/Thunderbird:Using_Gloda.
Some news.
With glodaquilla, i force TB indexing account by account. So on inbox pop3.free.fr TB crash. bp-489e260c-e93a-4b42-a6e7-fe25e2091210
When i look the inbox folder, not all my messages have a gloda id. eg. There are holes in the gloda_id sequences.
I think gloda indexes from newest to oldest within a given folder, so I think the newest message that's not indexed is likely to be the problem one (but I could be wrong). You could try copying it to its own folder, and let gloda try to index that folder, and see if it crashes. If not, you could divide and conquer with a test folder, to narrow it down.
Try 1/I copy all message to a new folder => TB crash when indexing on this new folder
Try 2/I copy all my mail not_gloda_indexed on a new folder, and let my mail gloda_indexed on inbox folder => TB passed inbox floder and crash on the beginning of the new folder
Try 3/I let the first mail on this new folder and i copy all the other mail on another folder. So TB doesn't index the only file but crash on the first mail on the "new new" folder.

Do you understand?
All my tries : 
bp-7fcd8ba5-2b5d-491a-a8b6-3cd882091210
bp-59dc07fd-8207-46b0-9819-fdb052091210	
bp-52098382-3212-4574-821d-8f2942091210	
bp-bc5c5440-48c5-49da-bd60-e89aa2091210	
bp-5e15ea36-ede3-4e1f-92b4-d8ab22091210
I'm not sure I understand, but what I need is the actual message that causes the crash so that I can try it here. Have you narrowed it down to a particular message, or are you having trouble doing that?
Yes, not very clear...it's difficult to explain me in english (i'm french)
The problem, i think, TB doesn't crash on a particular message.
if i move the message to another folder, TB still crash.
I'm pretty sure it's crashing on a particular message, or maybe multiple messages. Do you have a small-ish folder that it crashes on that you can zip up and e-mail to me at bienvenu@mozillamessaging.com?
Yes, you're right, i've got it!
When i try to look THE message, TB crash!
i will send you the mail, if i can to isolate the message!
Thx very much to Betan, I've reproduced the crash, and have a small patch that makes us not crash. I'll try to generate a smaller anonymous test case and attach it. From the crash, I suspect Andrew is right that we're doing something like calling eof() twice, because obj->options ends up being null.
Which is to say, we want to try to fix the underlying cause as well as do some bullet-proofing.
Marking security sensitive for now. This does fix the test case but I do need to investigate some more...
Flags: in-testsuite?
(In reply to comment #28)
> Created an attachment (id=417010) [details]
> this fixes the crash for the test case I have

Is this a WIP, and that's why you didn't set review requests ?
 
> Marking security sensitive for now. This does fix the test case but I do need
> to investigate some more...

Making that happen now.
Group: core-security
I think that patch is fine, but there's no regression test yet, and I haven't figured out the underlying issue, so that's why I haven't requested review yet.
looks like the fix for bug 478175 did cause this. This is a stab at a fix but Andrew will know if there's a better way of handling this case. I'll attach a relatively reduced test case in a minute, and try to add an xpcshell test for the reduced test case (I'm not sure if the message generator will generate such a confused message, so if it would, Andrew, let me know...otherwise, I'm just going to use the test case I have in the xpcshell test)
Attachment #417146 - Flags: review?(bugmail)
Attached file crashing test message — 
This mailbox crashes TB w/o the patches.
bienvenu, comment 26 implies this is a null-deref crash.  The stacks in comment 21 seem to support that.  Was there a reason you marked this bug security-sensitive?
Brandon - just being cautious while i figured out what the underlying problem was - we deref an offset of null, but fairly close to null so I could clear the security-sensitive flag. Right now it's a DOS for 3.0, however...
adds a little framework for adding test cases.
Attachment #417146 - Attachment is obsolete: true
Attachment #417182 - Flags: superreview?(bugzilla)
Attachment #417182 - Flags: review?(bugmail)
Attachment #417146 - Flags: review?(bugmail)
this should block 3.01
Status: NEW → ASSIGNED
blocking-thunderbird3.0: --- → .1+
Whiteboard: [no l10n impact] → [no l10n impact][has patch for review asuth, sr standard8]
So, honestly the chroniquery traces might have been overkill given that I established that it was the empty section that was crashing us by playing with the synthetic test case before, and such a crasher strongly implies that this was what was up.  On the other hand, it's nice in code as complex as libmime to be confident you're not just papering over a deeper problem.

I'll attach the test_mime_emitter.js test dude separately.
Assignee: bienvenu → bugmail
Attachment #418143 - Flags: superreview?(bienvenu)
Attachment #418143 - Flags: review?(bienvenu)
I also add test logic so that when doing equivalence checking we ignore the degenerate parts which is in keeping with libmime's behavior prior to the patch too.  (I also add a case to check that.)
Attachment #418146 - Flags: review?(bienvenu)
Comment on attachment 417182 [details] [diff] [review]
add test case - checked in

I think we still want this patch:
- libmime unit tests are always a good thing
- Although the crasher only happens when other code does something illegal (like trying to close an object twice), it's arguably better to have no possible crashers.

However, I would suggest that you add a "status = -1" to the new failure block since "goto FAIL" will not do it for us and arguably that's really a failure mode.
Attachment #417182 - Flags: review?(bugmail) → review+
Comment on attachment 418146 [details] [diff] [review]
v1 unit test that creates a jerky message

Probably "tells us" instead of "indicates"

+ * A part that indicates that tells us to produce NO output in a multipart
Attachment #418146 - Flags: review?(bienvenu) → review+
Attachment #418143 - Flags: superreview?(bienvenu)
Attachment #418143 - Flags: superreview+
Attachment #418143 - Flags: review?(bienvenu)
Attachment #418143 - Flags: review+
(In reply to comment #42)

> However, I would suggest that you add a "status = -1" to the new failure block
> since "goto FAIL" will not do it for us and arguably that's really a failure
> mode.

If I do that, it breaks the display of a good part (this is w/o your patch), so I'm a bit leary of doing that.
Whiteboard: [no l10n impact][has patch for review asuth, sr standard8] → [no l10n impact][has patch for sr standard8]
Comment on attachment 418143 [details] [diff] [review]
v1 don't close a child that is already closed in jerky empty multipart sections

+'ing for 3.01, though it should bake on the trunk for a little bit first.
Attachment #418143 - Flags: approval-thunderbird3.0.1+
pushed fix attachment 418143 [details] [diff] [review]:
http://hg.mozilla.org/comm-central/rev/a414c7bac0d9

pushed test attachment 418146 [details] [diff] [review] with requested change:
http://hg.mozilla.org/comm-central/rev/d1eb2e5e6aa6

I am marking this fixed because this makes the problem go away, but we should still land bienvenu's patch once it gets approved.  I am going to leave that up to bienvenu.  Feel free to reopen if it simplifies your tracking.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1a1
Comment on attachment 417182 [details] [diff] [review]
add test case - checked in

switching sr request to Neil, for just the mimemrel.cpp part. This is in essence a null pointer check, because eventually we will deref a null pointer if this happens.
Attachment #417182 - Flags: superreview?(bugzilla) → superreview?(neil)
Whiteboard: [no l10n impact][has patch for sr standard8] → [no l10n impact][has patch for sr neil]
Comment on attachment 417182 [details] [diff] [review]
add test case - checked in

Should this set status?
(In reply to comment #48)
> (From update of attachment 417182 [details] [diff] [review])
> Should this set status?

See #c44 - I tried setting status to -1, and it made the message a little more broken (the image failed to display in my test case, but it displays correctly when I hit that code if I don't set status to -1), so I thought it would be better to display more of the message. It's somewhat hypothetical in terms of this bug, because asuth's patch should prevent us from hitting the buggy codepath anymore.
Comment on attachment 417182 [details] [diff] [review]
add test case - checked in

Sorry, I must have overlooked that.
Attachment #417182 - Flags: superreview?(neil) → superreview+
Whiteboard: [no l10n impact][has patch for sr neil] → [ready to land?]
Attachment #417182 - Attachment description: add test case → add test case - checked in
I've checked in my fix and unit test - I believe asuth's fix is the only one we *know* we need for the branch, but we could take mine as well.
Whiteboard: [ready to land?]
Comment on attachment 417182 [details] [diff] [review]
add test case - checked in

a=Standard8
Attachment #417182 - Flags: approval-thunderbird3.0.1+
I've landed all the patches for 3.01 - I had to tweak the test_mimeStreaming.js patch not to use IOUtils, because that doesn't exist on the 3.01 branch.
status-thunderbird3.0: --- → .1-fixed
non-existent on trunk, 3.0pre and 3.0.1pre [1]. so can't verify this is gone without a testcase.  But at ~30 crashes/day for 3.0.0 we should know if it's gone within ~48 hours after 3.0.1 ships.

#7 crash for 3.0.0
Whiteboard: [needs verification post 3.0.1]
Ok, now no more crash for me with TB 3.0.1.
(In reply to comment #26)
> Thx very much to Betan, I've reproduced the crash, and have a small patch that
> makes us not crash. I'll try to generate a smaller anonymous test case and
> attach it. From the crash, I suspect Andrew is right that we're doing something
> like calling eof() twice, because obj->options ends up being null.

 Thanks!
yeah, thx, marking verified.
Status: RESOLVED → VERIFIED
Keywords: fixed-seamonkey2.0.3
This bug is still marked security-sensitive. Should it be opened as non-security or does it need a rating and an advisory?
I don't think it was exploitable, but in any case, build with fix has been shipped so opening up.
Group: core-security
Crash Signature: [@ MimeInlineTextHTML_parse_eof ]
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: