1370011 - Crash in nsACString_internal::First() called by mailnews/compose/src/nsSmtpUrl.cpp:98

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Jorg K (CEST = GMT+2)]

See for example
https://crash-stats.mozilla.com/report/index/43bf66dc-0461-4a18-bd40-d4a360170603

nsSmtpUrl.cpp:98 reads
  switch (NS_ToUpper(decodedName.First()))

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1370011-crash-calling-first.patch

Obvious fix. All the cases in the switch do a break at the end, so if the string is empty to start with, we break, too.

Comment 2

[Jorg K (CEST = GMT+2)]

I'll fix the trailing spaces when landing.

Comment 3

[Jorg K (CEST = GMT+2)]

Test case: Prepare a message containing a like like this: <a href="mailto:[e-mail]?=">[e-mail]</a>
For me, it crashed when viewing this e-mail. Clearly the token after the ? and before the = is empty, hence we crash.

With the patch it doesn't crash.

Hmm, when you receive this message with Daily, you might crash :-(

Comment 4

[Jorg K (CEST = GMT+2)]

Yep, sadly so. Bugmail for the previous comment crashes Daily.

Comment 5

[:aceman]

Comment on attachment 8874129 [details] [diff] [review]
1370011-crash-calling-first.patch

Review of attachment 8874129 [details] [diff] [review]:
-----------------------------------------------------------------

OK, in nsCString::First() there is an assertion that length is > 0. Does that crash TB?

Comment 6

[Jorg K (CEST = GMT+2)]

Sadly it's a MOZ_RELEASE_ASSERT() and that sure as hell brings down the application:
https://hg.mozilla.org/releases/mozilla-beta/annotate/8ff7d34dd934/xpcom/string/nsTSubstring.cpp#l46

BTW, did the bugmail crash your Daily?

Comment 7

[Wayne Mery (:wsmwk)]

The possibility has not been mentioned, so I take it this is not a regression?

Comment 8

[Jorg K (CEST = GMT+2)]

Sure it's a regression from whoever decided to add a MOZ_RELEASE_ASSERT() to nsTSubstring.cpp in Gecko54, let me see:
https://hg.mozilla.org/mozilla-central/rev/3e552501cd1f#l7.15 - Bug 1340577.

Comment 9

[Alfred Peters [:infofrommozilla]]

I can confirm that such a link in a HTML message crashes the TB.

Isn't this a security issue?

But such a Mail isn't needed. Just compose a HTML Mail:
- insert a Link - target: mailto:[e-mail]?=
- [OK] -> TB crashes.

Attachment 8874129 [details] [diff] fixes both problems.

Comment 10

[Jorg K (CEST = GMT+2)]

Please don't sent any more crashing examples since you've just crashed me again :-(
Thanks for testing!

Comment 11

[:aceman]

Yes comment 9 crashes me (on Linux).

So if First() has to return first char and has no way to report error (length == 0) they made it a hard crash. It was NS_ASSERTION before. mData[0] may be uninitialized memory if length == 0 so the assertion is guarding against that. We must make sure to not call First() in that case and the patch does that.

Comment 12

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/b1b6c970edf3410c7589cb01fdcc93c50435cbf2

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8874129 [details] [diff] [review]
1370011-crash-calling-first.patch

Should uplift to TB 52 as well for good housekeeping.

Comment 14

[Jorg K (CEST = GMT+2)]

Beta (TB 54):
https://hg.mozilla.org/releases/comm-beta/rev/e8a04501e8bf6e81351a9bd665b2ca7319eec624

Comment 15

[Jorg K (CEST = GMT+2)]

TB 52 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/0b42f505344cd3285bb4eab28f0ff632e6aedc48

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 8874129 [details] [diff] [review]
1370011-crash-calling-first.patch

Review of attachment 8874129 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/src/nsSmtpUrl.cpp
@@ +94,5 @@
>  
>        nsCString decodedName;
>        MsgUnescapeString(nsDependentCString(token), 0, decodedName);
> +      
> +      if (decodedName.Length() == 0)

Nit: I think .IsEmpty() would have been nicer.