684481 - crash in nsImapService::OpenAttachment fails to check to check for a null string before trying to use nsDependentCString

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

per IRC w/ standard8 ... crash nsImapService::OpenAttachment fails to check to check for a null string before trying to use nsDependentCString
not a regression afaict - https://crash-stats.mozilla.com/report/list?product=Thunderbird&query_search=signature&query_type=exact&query=strlen%20%7C%20nsDependentCString%3A%3AnsDependentCString%28char%20const%2A%29&reason_type=contains&date=09%2F02%2F2011%2021%3A48%3A13&range_value=4&range_unit=weeks&hang_type=any&process_type=all&do_query=1&signature=strlen%20%7C%20nsDependentCString%3A%3AnsDependentCString%28char%20const%2A%29

standard8 thinks this may be a regression in v7. based on spot checking of crash-stats I'm inclined to agree (but I could be incorrect). looks to be at least 5 individuals crashing with v7

bp-63c0a42f-b29e-43aa-a83d-8839b2110901
EXCEPTION_ACCESS_VIOLATION_READ
0x0
0	mozcrt19.dll	strlen	strlen.asm:81
1	xul.dll	nsDependentCString::nsDependentCString	objdir-tb/mozilla/dist/include/nsTDependentString.h:90
2	xul.dll	nsImapService::OpenAttachment	mailnews/imap/src/nsImapService.cpp:399
3	xul.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102

Comment 1

[Wayne Mery (:wsmwk)]

And not a duplicate of bug 676882.

Filed two more bugs, although it is unclear if these still exist on trunk.
 nsMessenger::SaveAllAttachments
 nsComponentManagerImpl::RegisterContractID
 nsAbCardProperty::SetPropertyAsAString
We will be able to tell better 
 a) after patch this bug is lands for v7 :)
 b) after v7 ships

Comment 2

[Mark Banner (:standard8)]

Attachment: Branch patch

Not exactly sure why this comes out as null, but this should stop the crash at least.

Comment 3

[David :Bienvenu]

don't you mean:

+          if (aFileName)
+            mailUrl->SetFileName(nsDependentCString(aFileName));

Comment 4

[Mark Banner (:standard8)]

Attachment: Branch patch v2

I blame it on lack of sleep and/or lack of coding recently.

Comment 5

[David :Bienvenu]

Comment on attachment 560464 [details] [diff] [review]
Branch patch v2

:-)

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 560464 [details] [diff] [review]
Branch patch v2

Checked into aurora and beta:

http://hg.mozilla.org/releases/comm-aurora/rev/983882cfe7c4
http://hg.mozilla.org/releases/comm-beta/rev/e06e69c69a0d

Comment 7

[Mark Banner (:standard8)]

Proper beta landing: http://hg.mozilla.org/releases/comm-beta/rev/cafdd37224b1

Comment 8

[Mark Banner (:standard8)]

Attachment: Trunk fix

A bit later than planned. If I can get reviews on this today, then I can land before the branch and avoid some interesting flag tweaks.

Comment 9

[[e-mail]]

Comment on attachment 562425 [details] [diff] [review]
Trunk fix

>+  void openAttachment(in ACString aContentType, 
>+                      in AUTF8String aFileName, 
>+                      in AUTF8String aUrl,
>+                      in AUTF8String aMessageUri, 
The message service function to which this delegates describes these parameters all as ACString rather than AUTF8String. They can't both be correct ;-)

>+                      in nsISupports aDisplayConsumer, 
Nit: some trailing spaces crept in, here is one example.

>+    PRInt32 partStart = nsCString(aUrl).Find("part=");
PromiseFlatCString

>+    uri += Substring(aUrl, partStart, aUrl.FindChar('&', partStart + 5));
I think FindChar returns the offset from the start of the string, rather than the start of the search, so you'll need to subtract partStart.

>-NS_IMETHODIMP nsMailboxService::OpenAttachment(const char *aContentType,
>-                                               const char *aFileName,
>-                                               const char *aUrl,
>-                                               const char *aMessageUri,
>+NS_IMETHODIMP nsMailboxService::OpenAttachment(const nsACString &aContentType,
>+                                               const nsACString &aFileName,
>+                                               const nsACString &aUrl,
>+                                               const nsACString &aMessageUri,
That's convenient :-)

Comment 10

[David :Bienvenu]

I'm going to wait for Neil's comments to be addressed before I review this, especially because of this part:

>+    uri += Substring(aUrl, partStart, aUrl.FindChar('&', partStart + 5));
I think FindChar returns the offset from the start of the string, rather than the start of the search, so you'll need to subtract partStart.

Comment 11

[Mark Banner (:standard8)]

Ok, my plan at the moment is to land the branch patch on trunk, and then I'll branch out the API improvements to another bug. That way we'll get everything covered right before the merge.

Comment 12

[Mark Banner (:standard8)]

Landed the branch patch onto trunk: http://hg.mozilla.org/comm-central/rev/ea4ec56a9b02

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 562425 [details] [diff] [review]
Trunk fix

Filed bug 689544 to get this patch landed.