Closed Bug 1370011 Opened 3 years ago Closed 3 years ago

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

Categories

(MailNews Core :: Composition, defect)

defect
Not set
critical

Tracking

(thunderbird_esr5254+ fixed, thunderbird54 fixed, thunderbird55 fixed)

RESOLVED FIXED
Thunderbird 55.0
Tracking Status
thunderbird_esr52 54+ fixed
thunderbird54 --- fixed
thunderbird55 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Crash Data

Attachments

(1 file)

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

nsSmtpUrl.cpp:98 reads
  switch (NS_ToUpper(decodedName.First()))
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.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8874129 - Flags: review?(acelists)
I'll fix the trailing spaces when landing.
Severity: normal → critical
Test case: Prepare a message containing a like like this: <a href="mailto:henk@henk.com.au?=">henk@henk.com.au</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 :-(
Yep, sadly so. Bugmail for the previous comment crashes Daily.
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?
Attachment #8874129 - Flags: review?(acelists) → review+
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?
The possibility has not been mentioned, so I take it this is not a regression?
Crash Signature: [@ nsACString_internal::First ]
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.
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:foo@example.com?=
- [OK] -> TB crashes.

Attachment 8874129 [details] [diff] fixes both problems.
Please don't sent any more crashing examples since you've just crashed me again :-(
Thanks for testing!
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.
https://hg.mozilla.org/comm-central/rev/b1b6c970edf3410c7589cb01fdcc93c50435cbf2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Comment on attachment 8874129 [details] [diff] [review]
1370011-crash-calling-first.patch

Should uplift to TB 52 as well for good housekeeping.
Attachment #8874129 - Flags: approval-comm-esr52?
Attachment #8874129 - Flags: approval-comm-beta+
Attachment #8874129 - Flags: approval-comm-esr52? → approval-comm-esr52+
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.
You need to log in before you can comment on or make changes to this bug.