Closed Bug 1370011 Opened 4 years ago Closed 4 years ago

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


(MailNews Core :: Composition, defect)

Not set


(thunderbird_esr5254+ fixed, thunderbird54 fixed, thunderbird55 fixed)

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


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


Crash Data


(1 file)

See for example

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
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=""></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]

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:

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: - 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:
- [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.
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Comment on attachment 8874129 [details] [diff] [review]

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]

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.