Last Comment Bug 505717 - crash [@ EscapeFromSpaceLine(nsIOutputStream*, char*, char const*)]
: crash [@ EscapeFromSpaceLine(nsIOutputStream*, char*, char const*)]
Status: RESOLVED FIXED
[needs verification post 3.0.1]
: crash, fixed-seamonkey2.0.3, topcrash
Product: MailNews Core
Classification: Components
Component: Import (show other bugs)
: 1.9.1 Branch
: All All
: -- critical (vote)
: Thunderbird 3.1a1
Assigned To: Jeff Beckley
:
:
Mentors:
http://crash-stats.mozilla.com/report...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-22 05:53 PDT by Ludovic Hirlimann [:Usul]
Modified: 2011-06-13 10:01 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
.1+
.1-fixed


Attachments
Proposed fix (832 bytes, patch)
2009-12-04 13:05 PST, Jeff Beckley
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
Better fix (814 bytes, patch)
2009-12-14 11:25 PST, Jeff Beckley
mozilla: review+
mozilla: superreview+
standard8: approval‑thunderbird3.0.1+
Details | Diff | Splinter Review

Description Ludovic Hirlimann [:Usul] 2009-07-22 05:53:26 PDT
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 2 Wayne Mery (:wsmwk, NI for questions) 2009-07-25 05:58:16 PDT
3.0b3 topcrash

comments are all about
importing data from Eudora
Crashed during import of Eudora settings
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2009-11-28 15:35:46 PST
#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] 2009-12-01 09:45:39 PST
beckley anything you can do on this one ?
Comment 5 Jeff Beckley 2009-12-04 11:39:55 PST
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 2009-12-04 13:05:29 PST
Created attachment 416150 [details] [diff] [review]
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] 2009-12-09 09:21:46 PST
bienvenu ping.
Comment 8 David :Bienvenu 2009-12-10 08:04:44 PST
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 2009-12-10 08:07:14 PST
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 2009-12-14 11:25:53 PST
Created attachment 417514 [details] [diff] [review]
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 2009-12-17 12:48:47 PST
fix pushed to trunk
Comment 12 David :Bienvenu 2009-12-17 12:49:54 PST
changeset:   4554:fdddfa53e4f0

we'll let this bake on the trunk a little bit before landing for 3.01
Comment 13 Mark Banner (:standard8) 2009-12-21 06:10:36 PST
Checked in to branch: http://hg.mozilla.org/releases/comm-1.9.1/rev/a5a1ae991335
Comment 14 Serge Gautherie (:sgautherie) 2009-12-21 11:19:40 PST
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, NI for questions) 2010-01-10 16:16:37 PST
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#

Note You need to log in before you can comment on or make changes to this bug.