505717 - crash [@ EscapeFromSpaceLine(nsIOutputStream*, char*, char const*)]

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[Ludovic Hirlimann [:Usul]]

0  	thunderbird.exe  	EscapeFromSpaceLine  	mailnews/base/util/nsMsgUtils.cpp:796
1 	thunderbird.exe 	nsOutlookCompose::CopyComposedMessage 	mailnews/import/outlook/src/nsOutlookCompose.cpp:906
2 	thunderbird.exe 	nsOutlookMail::ImportMailbox 	mailnews/import/outlook/src/nsOutlookMail.cpp:510
3 	thunderbird.exe 	ImportOutlookMailImpl::ImportMailbox 	mailnews/import/outlook/src/nsOutlookImport.cpp:478
4 	thunderbird.exe 	ImportMailThread 	mailnews/import/src/nsImportMail.cpp:916
5 	nspr4.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:426
6 	nspr4.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:122
7 	mozcrt19.dll 	_callthreadstartex 	threadex.c:348
8 	mozcrt19.dll 	_threadstartex 	threadex.c:326
9 	kernel32.dll 	kernel32.dll@0x44910 	
10 	ntdll.dll 	ntdll.dll@0x3e4b5 	
11 	ntdll.dll 	ntdll.dll@0x3e488

Comment 1

[Magnus Melin [:mkmelin]]

http://hg.mozilla.org/comm-central/annotate/860fb123cbac/mailnews/base/util/nsMsgUtils.cpp#l796

Comment 2

[Wayne Mery (:wsmwk)]

3.0b3 topcrash

comments are all about
importing data from Eudora
Crashed during import of Eudora settings

Comment 3

[Wayne Mery (:wsmwk)]

#5 crash for 3.0b4, 1.8% of crashes
#3 crash for 3.0, 3.2% of crashes (but it's early)

setting blocking?

a few are emerging with french comments, like this one about outlook ...
"Failed to import a number of emails from Outlook2000, often containing attachments, and organized in many directories"
bp-e90c254f-ff43-430b-b824-c3da42091126
Frame	Module	Signature [Expand]	Source
0	thunderbird.exe	EscapeFromSpaceLine	mailnews/base/util/nsMsgUtils.cpp:801
1	thunderbird.exe	nsOutlookCompose::CopyComposedMessage	mailnews/import/outlook/src/nsOutlookCompose.cpp:890
2	thunderbird.exe	nsOutlookMail::ImportMailbox	mailnews/import/outlook/src/nsOutlookMail.cpp:506
3	thunderbird.exe	ImportOutlookMailImpl::ImportMailbox	mailnews/import/outlook/src/nsOutlookImport.cpp:478
4	thunderbird.exe	ImportMailThread	mailnews/import/src/nsImportMail.cpp:916
5	nspr4.dll	_PR_NativeRunThread	nsprpub/pr/src/threads/combined/pruthr.c:426 

bp-9da9bbba-62b6-44bc-85b5-d8c9f2091127
bp-45abfad1-d480-4195-b826-90f752091123
0	thunderbird.exe	EscapeFromSpaceLine	 mailnews/base/util/nsMsgUtils.cpp:801
1	thunderbird.exe	nsEudoraCompose::CopyComposedMessage	mailnews/import/eudora/src/nsEudoraCompose.cpp:989
2	thunderbird.exe	nsEudoraMailbox::ImportMessage	mailnews/import/eudora/src/nsEudoraMailbox.cpp:578
3	thunderbird.exe	nsEudoraMailbox::ImportMailboxUsingTOC	mailnews/import/eudora/src/nsEudoraMailbox.cpp:423
4	thunderbird.exe	nsEudoraMailbox::ImportMailbox	mailnews/import/eudora/src/nsEudoraMailbox.cpp:271
5	thunderbird.exe	ImportEudoraMailImpl::ImportMailbox	mailnews/import/eudora/src/nsEudoraImport.cpp:598
6	thunderbird.exe	ImportMailThread	mailnews/import/src/nsImportMail.cpp:916

Comment 4

[Ludovic Hirlimann [:Usul]]

beckley anything you can do on this one ?

Comment 5

[Jeff Beckley]

Here's where it's crashing:

while ((pChar < end) && (*pChar != '\r') && (*(pChar+1) != '\n'))
  pChar++;

I see a flaw in the code in a particular scenario.  If the passed in buffer ends in a CR, then the check to see if the next character is a LF could go past the end of the buffer and produce an access violation.

I'll take a stab at making a fix.

Comment 6

[Jeff Beckley]

Attachment: Proposed fix

Here's a potential fix. It treats a line that ends in just a CR like a partial line.

Comment 7

[Ludovic Hirlimann [:Usul]]

bienvenu ping.

Comment 8

[David :Bienvenu]

Comment on attachment 416150 [details] [diff] [review]
Proposed fix

Thx for the fix. Can you put spaces around the '+' in pChar+1 - two places?

Comment 9

[David :Bienvenu]

I think with the patch we won't escape From lines that aren't CRLF terminated - is there any reason not to treat them as full lines?

Comment 10

[Jeff Beckley]

Attachment: Better fix

Here's a new patch.  Note that this change only affects the case where there isn't a CRLF-terminated line at the end of the buffer.

With the current code (and remaining in the patch), if the line is only CR-terminated in the middle of the buffer it will treat that as a line.

If you get to the end of a buffer without a CRLF then you wind up in the the else case of the (pChar < end) check, and that portion does From escaping as well.

Comment 11

[David :Bienvenu]

fix pushed to trunk

Comment 12

[David :Bienvenu]

changeset:   4554:fdddfa53e4f0

we'll let this bake on the trunk a little bit before landing for 3.01

Comment 13

[Mark Banner (:standard8)]

Checked in to branch: http://hg.mozilla.org/releases/comm-1.9.1/rev/a5a1ae991335

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 417514 [details] [diff] [review]
Better fix

>diff --git a/mailnews/base/util/nsMsgUtils.cpp b/mailnews/base/util/nsMsgUtils.cpp
>--- a/mailnews/base/util/nsMsgUtils.cpp
>+++ b/mailnews/base/util/nsMsgUtils.cpp
>@@ -793,17 +793,19 @@ nsresult EscapeFromSpaceLine(nsIOutputSt
> {
>   nsresult rv;
>   char *pChar;
>   PRUint32 written;
> 
>   pChar = start;
>   while (start < end)
>   {
>-    while ((pChar < end) && (*pChar != '\r') && (*(pChar+1) != '\n'))
>+    while ((pChar < end) && (*pChar != '\r') && ((pChar+1) < end) && (*(pChar+1) != '\n'))

Can't (pChar < end) and ((pChar+1) < end) be merged?

Comment 15

[Wayne Mery (:wsmwk)]

amazingly even though we are getting >100 crashes per day with 3.0.0, this is (and was) virtually non-existent on trunk, 3.0pre and 3.0.1pre [1]. so can't verify this is gone without a testcase.  But we should know within ~48 hours after 3.0.1 ships.


[1] http://crash-stats.mozilla.com/query/query?product=Thunderbird&version=Thunderbird%3A3.0.1pre&version=Thunderbird%3A3.0pre&version=Thunderbird%3A3.1a1pre&version=Thunderbird%3A3.2a1pre&date=12%2F15%2F2009&range_value=4&range_unit=weeks&query_search=signature&query_type=exact&query=EscapeFromSpaceLine%28nsIOutputStream*%2C+char*%2C+char+const*%29&build_id=&do_query=1#