crash [@ CountTotalMimeAttachments(MimeContainer*) ]

RESOLVED FIXED in Thunderbird 3.0rc1

Status

MailNews Core
MIME
--
critical
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: Usul, Assigned: Bienvenu)

Tracking

(5 keywords)

1.9.1 Branch
Thunderbird 3.0rc1
x86
Windows XP
crash, fixed1.8.1.24, regression, testcase, topcrash
Bug Flags:
blocking-thunderbird3 +
wanted-thunderbird3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [no l10n impact][sg:moderate] invalid read, crash signature, URL)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
0  	thunderbird.exe  	CountTotalMimeAttachments  	mailnews/mime/src/mimemoz2.cpp:216
1 	thunderbird.exe 	CountTotalMimeAttachments 	mailnews/mime/src/mimemoz2.cpp:223
2 	thunderbird.exe 	CountTotalMimeAttachments 	mailnews/mime/src/mimemoz2.cpp:223
3 	thunderbird.exe 	MimeGetAttachmentList 	mailnews/mime/src/mimemoz2.cpp:568
4 	thunderbird.exe 	mime_display_stream_complete 	mailnews/mime/src/mimemoz2.cpp:975
5 	thunderbird.exe 	nsStreamConverter::OnStopRequest 	mailnews/mime/src/nsStreamConverter.cpp:1068
6 	thunderbird.exe 	nsMsgProtocol::OnStopRequest 	mailnews/base/util/nsMsgProtocol.cpp:393
7 	thunderbird.exe 	nsMailboxProtocol::OnStopRequest 	mailnews/local/src/nsMailboxProtocol.cpp:380
8 	thunderbird.exe 	nsInputStreamPump::OnStateStop 	netwerk/base/src/nsInputStreamPump.cpp:576
9 	thunderbird.exe 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:401
10 	xpcom_core.dll 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:111
11 	xpcom_core.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
12 	xpcom_core.dll 	NS_ProcessNextEvent_P 	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:227
13 	xpcom_core.dll 	nsThread::Shutdown 	xpcom/threads/nsThread.cpp:465
14 	thunderbird.exe 	mozStorageConnection::Close 	storage/src/mozStorageConnection.cpp:244
15 	xpcom_core.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
16 	thunderbird.exe 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2454
17 	thunderbird.exe 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1590
Flags: blocking-thunderbird3?
(Assignee)

Comment 1

8 years ago
is this a top-crash or not? if so, we'd mark it blocking...
Not a top crasher, so not blocking but marking wanted.
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3-
now #1 topcrash, no doubt induced by gloda indexing
Flags: blocking-thunderbird3- → blocking-thunderbird3?
Keywords: topcrash
(Assignee)

Comment 4

8 years ago
ok, marking blocking, and taking for now. I'll poke around the CountTotalMimeAttachments code. A reproducible case would be helpful, though hard to get, I'm sure.
Assignee: nobody → bienvenu
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Keywords: qawanted
(Assignee)

Comment 5

8 years ago
Unless I'm missing something, this code already has null checks, so I don't see any obvious way of fixing this. So a test case would be really helpful.
perhaps one our CC: can whip up a testcase
Keywords: testcase-wanted
bug 468394 may be a dupe but no testcase there either - mime_subclass_p(MimeObjectClass*, MimeObjectClass*)

Updated

8 years ago
Whiteboard: [no l10n impact]
(Reporter)

Updated

8 years ago
Duplicate of this bug: 519425
(Reporter)

Comment 9

8 years ago
(In reply to comment #8)
> *** Bug 519425 has been marked as a duplicate of this bug. ***

We have a identified user having the issue, Andrew what do you need from him ?
(Reporter)

Comment 10

8 years ago
Bienvenu you can tackle it too :-)
(Assignee)

Comment 11

8 years ago
Are we in contact with the user? We need to somehow figure out which message is causing the crash and get a copy of it. How we proceed depends somewhat on how technical the user is. If the user can run a debug build under the debugger, we'd just want to poke at some variables in the debugger to identify the message. If not, then we'd need to turn on some sort of logging to figure out which message is crashing. It would also be helpful to know if it's an imap or local message.
(Reporter)

Comment 12

8 years ago
Massimiliano, how do you feel about running a debug build ?

Comment 13

8 years ago
Ok no problem, I can do it. However, the crash is related to archive mail in the local folders. If I leave this folders Thunderbird runnig fine.
(Assignee)

Comment 14

8 years ago
Massimiliano, if you want to narrow down which message is causing the problem by a binary search, halving the folder each time, and then give me the message, that would be OK too.

How do you mean, "leave this folder"? You can't turn off indexing of a local folder - do you mean you move the folder out of your profile?

Thx for helping with this!

Comment 15

8 years ago
Sorry, I mean move the folder out my profile.
(Assignee)

Comment 16

8 years ago
Unless you're fluent in using a debugger, and able to poke around in the local variables when you crash (apologies for not knowing if you are), it would probably be easier to just find the crashing message. I see from the other bug that the folder is > 10000 messages, which would make it painful. One possibility is make a new folder with just the messages that have attachments, by sorting by the attachment column, and copying them to a new folder, under the supposition that the crashing message has an attachment (just a guess from the stack trace).
(Assignee)

Comment 17

8 years ago
Or,  In the Config Editor (Advanced
prefs) turn on: mailnews.database.global.logging.console and
mailnews.database.global.logging.dump

(.dump will go to stderr, so that works if you know how to run thunderbird from
a shell/console)

and then wait for the crash - I'm not sure if gloda gives enough info with logging to give you an idea of which message it crashed on, but it might.
(Assignee)

Comment 18

8 years ago
The gloda logging might be the easiest way - I think it will tell you the message key of the messages its streaming; after you crash, you should be able to turn on the order received column for your local folder to find what the message with that key is...
(Reporter)

Comment 19

8 years ago
Massimiliano ?
If needed, there is a FF extension JSConsoleOutput that will redirect js console to disk. It's no longer available from the author's website. I think I have a version hacked up for TB. FF version available at http://mac.softpedia.com/progDownload/JS-Console-output-redirector-Download-36363.html

Comment 21

8 years ago
Sorry for my delay... was sick. My Thunderbird version runing on windows XP. Can I make a debug?
(Assignee)

Comment 22

8 years ago
No problem, sorry you were ill - you don't need a debug build to do gloda logging. You should be able to find the message by following these 10 simple steps :-)

1. Use the config editor to set  mailnews.database.global.logging.console to true.
2. Install the extension that Wayne mentioned (he'll have to get that to you somehow).
3 Move the mail folder that causes the crash back into your profile
4. Startup and run until you crash.
5 Save off the file that contains js console output the extension generated, which should have information about the crash, specifically the message key of the last message it tried to index, at the end of the log.
6 Move the bad mail folder away, restart
7 Turn off gloda indexing, shutdown.
8 Move the crashing folder back into your profile, restart, 
9 Click on the bad folder, add the "order received" column to the message list.
10. Sort by order received, and find the message with the key that the log tells you gloda last tried to index.

Comment 23

8 years ago
Problem on point 2. The extension doesn't install to TB3b4
(Reporter)

Comment 24

8 years ago
(In reply to comment #20)
> If needed, there is a FF extension JSConsoleOutput that will redirect js
> console to disk. It's no longer available from the author's website. I think I
> have a version hacked up for TB. FF version available at
> http://mac.softpedia.com/progDownload/JS-Console-output-redirector-Download-36363.html

Wayne can you attached the version you have ?

Comment 25

8 years ago
(In reply to comment #24)
> (In reply to comment #20)
> > If needed, there is a FF extension JSConsoleOutput that will redirect js
> > console to disk. It's no longer available from the author's website. I think I
> > have a version hacked up for TB. FF version available at
> > http://mac.softpedia.com/progDownload/JS-Console-output-redirector-Download-36363.html
> 
> Wayne can you attached the version you have ?

Wayne already gave to me your version but doesn't install ...

Comment 26

8 years ago
(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #20)
> > > If needed, there is a FF extension JSConsoleOutput that will redirect js
> > > console to disk. It's no longer available from the author's website. I think I
> > > have a version hacked up for TB. FF version available at
> > > http://mac.softpedia.com/progDownload/JS-Console-output-redirector-Download-36363.html
> > 
> > Wayne can you attached the version you have ?
> 
> Wayne already gave to me your version but doesn't install ...

Sorry, Wayne already gave to me his version but doesn't install ...

Comment 27

8 years ago
Guys, the last version installed fine but after crash I can not see the output file log in my XP PC
I wating other solution that can help me to find the message broken in my mail box
(Assignee)

Comment 28

8 years ago
Is the log empty? Or it doesn't even exist? I have not tried this extension myself (in fact, I don't have a copy of it myself).

Comment 29

8 years ago
The log doesn't exist (or I do not find)
Ratty made us an update which works!

go to http://downloads.mozdev.org/xsidebar/mods/ and get JS_Console_output_redirector_0.1.1-mod.xpi
install
flip it on in Tools>Redirect JSConsole output to file
log file in <profile>\JSConsoleOutput\ directory

Comment 31

8 years ago
Now the log file exist but is empty after crash.

I have set true follow parameter:


mailnews.database.global.logging.console;true
mailnews.database.global.logging.dump;true
mailnews.database.global.logging.net;true
extensions.logging.enabled;true
extensions.jsconsoleoutput.redirect;true
extensions.jsconsoleoutput.hideChromeErrors;true
casini, so you toggled 
  Tools>Redirect JSConsole output to file
and you see good console output in the file?  But after crash the file is empty?
(Assignee)

Comment 33

8 years ago
Do you crash right away? I had to restart after installing the extension and setting mailnews.database.global.logging.console to true, but then the log was populated immediately.

Comment 34

8 years ago
Created attachment 405232 [details]
jsconsole.log

This my jsconsole.log with some errors.
(Assignee)

Comment 35

8 years ago
Does this crash happen if you disable Lightning and restart?

Comment 36

8 years ago
I can not understand why sometimes there is no jsconsole.log (the file exist but is empty after crash).

With Lightning disable behavior is the same, TB crashed but the jsconsole.log is empty
(Assignee)

Comment 37

8 years ago
I'm pretty sure there's a way to get more logging out of gloda, besides just warnings and errors
(Assignee)

Comment 38

8 years ago
OK, I've generated a try server build that uses nspr logging to log the mailbox urls we run - it should show up here in an hour or two - http://tinderbox.mozilla.org/showbuilds.cgi?tree=ThunderbirdTry

To generate a log, you need to follow these instructions:

https://wiki.mozilla.org/MailNews:Logging

The log module is "mailbox" (not imap or pop3 or any of those). You should add the "sync" option just in case we crash before the data is written to the log, so

NSPR_LOG_MODULES=mailbox:5,sync
(Assignee)

Comment 39

8 years ago
Here's a link to the windows build to try - http://s3.mozillamessaging.com/build/try-server/2009-10-09_16:10-bienvenu@nventure.com-1255129754/bienvenu@nventure.com-1255129754-mail-try-win32.installer.exe

Comment 40

8 years ago
Find the messages that cause crash. When I to select them (also with index gloda disable) TB to crash. Them have the attachment. I will try to move all the other in new folder and send you the broken message.
(Assignee)

Comment 41

8 years ago
great, thanks, Massimiliano.
(Assignee)

Comment 42

8 years ago
If you hide the message pane (f8), it might make it easier to select a message and copy it to a new folder (might want to turn gloda off temporarily as well)

Comment 43

8 years ago
Created attachment 405993 [details]
local folder with one email broken

unzip file to local folder
(Reporter)

Updated

8 years ago
Keywords: testcase-wanted → testcase
(Assignee)

Updated

8 years ago
Group: core-security
(Assignee)

Comment 44

8 years ago
marking as security sensitive - 2.0 has this problem as well, though it's unlikely this is exploitable. It's a bad cast that causes reads of invalid memory addresses. I do have a very simple fix. I need to see if the test case can be simplified. It looks to be simply a multipart mixed message with text/html part and an message/rfc822 part.
(Assignee)

Comment 45

8 years ago
Created attachment 406026 [details] [diff] [review]
fix crash

This fixes the crash. I have a reduced test case (basically taking the text and e-mail addresses out of the original test case). I'd like to add this to the xpcshell-tests, but there are still a few assertions triggered by the test case, and that breaks xpcshell tests in debug builds. The assertions look relatively harmless, in the sense that they're handled, and I've seen them before with other messages.
Attachment #406026 - Flags: superreview?
Attachment #406026 - Flags: review?(bugmail)
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact] → [no l10n impact][has patch for review]
Comment on attachment 406026 [details] [diff] [review]
fix crash

Yes, the code seems much more correct now.

There isn't actually an SR target requested...

If one of the assertions/warnings is about the lack of an identity, I have that resolved in my gloda patch, with the relevant code being as follows if you want to backport it and stick it somewhere:

    acctMgr.createLocalMailAccount();

    let localAccount = acctMgr.FindAccountForServer(acctMgr.localFoldersServer);

    // We need an identity or we get angry warnings.
    let identity = acctMgr.createIdentity();
    localAccount.addIdentity(identity);
    localAccount.defaultIdentity = identity;
Attachment #406026 - Flags: review?(bugmail) → review+
(Assignee)

Updated

8 years ago
Attachment #406026 - Flags: superreview? → superreview?(bugzilla)
(Assignee)

Comment 47

8 years ago
no, the assertions are in libmime, about sending empty lines to the parser, and attempting to close already closed objects...I strongly suspect this code is involved:

http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemsg.cpp#238

where we're hacking on a newline onto the end of the message part, if we didn't find one.
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact][has patch for review] → [no l10n impact][has patch for sr standard8]
(Assignee)

Comment 48

8 years ago
Created attachment 406054 [details]
simplified folder that crashes

this is an sanitized test case.

It turns out if I add a blank line before the ending mime part separator, this problem goes away. It turns out the code I mxr'ed above is the cause of this problem. If I fix that issue, the assertions go away as well, so I can add a test case - yay!
(Assignee)

Comment 49

8 years ago
This regression was caused by the fix for Bug 413874, which was landed in 2.0x as well. I'll attach a cumulative patch, along with a test case.
Keywords: qawanted → regression
Whiteboard: [no l10n impact][has patch for sr standard8] → [no l10n impact]
(Reporter)

Comment 50

8 years ago
(In reply to comment #49)
> This regression was caused by the fix for Bug 413874, which was landed in 2.0x
> as well. I'll attach a cumulative patch, along with a test case.

Could we use the testcase in a xpshell test so we don't regress in the future ?
Flags: in-testsuite?
(Assignee)

Comment 51

8 years ago
yes, that's what I meant I was going to do.
(Assignee)

Comment 52

8 years ago
Created attachment 406070 [details] [diff] [review]
cumulative fix with xpcshell test

The underlying issue was that the patch in bug 413874 didn't pass in the right output buffer size into PL_strncpyz, so we were only appending <CR>, not <CRLF>, which, on windows, was getting libmime rather confused about how the part ended...not sure what would happen on linux/mac - we wouldn't have any line endings, which would probably generate an assertion or twenty.

I still think the change to CountTotalMimeAttachments is worthwhile.

I added a mime streaming test to libmime. libmime could certainly use some more test cases, though the gloda tests exercise most of the basics.
Attachment #406026 - Attachment is obsolete: true
Attachment #406070 - Flags: superreview?(bugzilla)
Attachment #406070 - Flags: review?(bugmail)
Attachment #406026 - Flags: superreview?(bugzilla)
(Assignee)

Comment 53

8 years ago
I audited the rest of the patch in bug 413874 and that was the only instance of this mistake.
Attachment #406070 - Flags: review?(bugmail) → review+
Attachment #406070 - Flags: superreview?(bugzilla) → superreview+
Target Milestone: --- → Thunderbird 3.0rc1
(Assignee)

Comment 54

8 years ago
fix checked in.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 55

8 years ago
Created attachment 406227 [details] [diff] [review]
fix for 1.8 branch

this is just the core port of the patch that just landed on the trunk
Attachment #406227 - Flags: approval1.8.1.next?
(Assignee)

Comment 56

8 years ago
Massimiliano, many thanks for the test case.
does this likely fix nsCounterManager::AddCounterResetsAndIncrements(nsIFrame*) http://crash-stats.mozilla.com/report/index/f2215189-c433-416b-871f-182392090925

strange this bug# does show up in http://talkback-public.mozilla.org/reports/thunderbird/TB20023/index.html  - perhaps talkback not like (MimeContainer*) in the summary.
Blocks: 413874
(Assignee)

Comment 58

8 years ago
(In reply to comment #57)
> does this likely fix nsCounterManager::AddCounterResetsAndIncrements(nsIFrame*)
> http://crash-stats.mozilla.com/report/index/f2215189-c433-416b-871f-182392090925

I don't see how they could be related.
Whiteboard: [no l10n impact] → [no l10n impact][sg:moderate] invalid read
Comment on attachment 406227 [details] [diff] [review]
fix for 1.8 branch

Approved for 1.8.1.24, a=dveditz for release-drivers
Attachment #406227 - Flags: approval1.8.1.next? → approval1.8.1.next+
(Assignee)

Comment 60

8 years ago
fixed for 1.8.1.24
Keywords: fixed1.8.1.24
I want to verify this for 1.8.1.24. How exactly do I add the folder in comment 48 to TB 2 for testing?
Group: core-security

Updated

7 years ago
Crash Signature: [@ CountTotalMimeAttachments(MimeContainer*) ]
(Reporter)

Updated

2 years ago
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.