Crash in nsPlainTextSerializer::AddToLine

VERIFIED FIXED in Firefox 48

Status

()

Core
Serializers
--
critical
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: wsmwk, Assigned: Jorg K (GMT+2))

Tracking

({crash, topcrash-thunderbird})

unspecified
mozilla49
x86
Windows 7
crash, topcrash-thunderbird
Points:
---

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed, thunderbird_esr4547+ fixed)

Details

(Whiteboard: [regression:TB45], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

#10 crash for 45.1.0

Oldest build found is bp-1bbdd46e-daf9-440c-ac89-6190d2160521
 0 	xul.dll	nsPlainTextSerializer::AddToLine(wchar_t const*, int)	dom/base/nsPlainTextSerializer.cpp:1334
1 	xul.dll	nsPlainTextSerializer::Write(nsAString_internal const&)	dom/base/nsPlainTextSerializer.cpp:1718
2 	xul.dll	nsPlainTextSerializer::DoAddText(bool, nsAString_internal const&)	dom/base/nsPlainTextSerializer.cpp:1086
3 	xul.dll	nsPlainTextSerializer::AppendText(nsIContent*, int, int, nsAString_internal&)	dom/base/nsPlainTextSerializer.cpp:346
4 	xul.dll	nsDocumentEncoder::SerializeNodeStart(nsINode*, int, int, nsAString_internal&, nsINode*)	dom/base/nsDocumentEncoder.cpp:457
5 	xul.dll	nsDocumentEncoder::SerializeToStringRecursive(nsINode*, nsAString_internal&, bool, unsigned int)	dom/base/nsDocumentEncoder.cpp:560
6 	xul.dll	nsDocumentEncoder::SerializeToStringRecursive(nsINode*, nsAString_internal&, bool, unsigned int)	dom/base/nsDocumentEncoder.cpp:560
7 	xul.dll	nsDocumentEncoder::SerializeToStringRecursive(nsINode*, nsAString_internal&, bool, unsigned int)	dom/base/nsDocumentEncoder.cpp:560
8 	xul.dll	nsDocumentEncoder::SerializeToStringRecursive(nsINode*, nsAString_internal&, bool, unsigned int)	dom/base/nsDocumentEncoder.cpp:560
9 	xul.dll	nsDocumentEncoder::SerializeToStringRecursive(nsINode*, nsAString_internal&, bool, unsigned int)	dom/base/nsDocumentEncoder.cpp:560
10 	xul.dll	nsDocumentEncoder::EncodeToStringWithMaxLength(unsigned int, nsAString_internal&)	dom/base/nsDocumentEncoder.cpp:1222
(Assignee)

Comment 1

a year ago
Also a top crash in TB 45.0.

Looks like what we did to fix the "Asian crisis" in bug 1225864 revealed a possible crash. Look at the code:
https://hg.mozilla.org/releases/mozilla-esr45/annotate/1f7c05ab920b/dom/base/nsPlainTextSerializer.cpp#l1334

// fallback if the line breaker is unavailable or failed
if (!mLineBreaker) {
  goodSpace = mWrapColumn-prefixwidth;
  while (goodSpace >= 0 &&
         !nsCRT::IsAsciiSpace(mCurrentLine.CharAt(goodSpace))) {
    goodSpace--;
  }
}

Now, the line breaker was switched off to solve the "Asian crisis", so the code that was previously not run is now run.

I'd say that goodSpace should be tested not to exceed mCurrentLine.Length() or else it will crash ;-)

That should be reproducible by setting pref mailnews.wraplength to a large value, say: 999999, before sending a message.

I could imagine that people in their desperation did this to avoid the erroneous spaces being inserted into Asian text.
(Assignee)

Updated

a year ago
Component: General → Serializers
Product: Thunderbird → Core
(Assignee)

Comment 2

a year ago
Created attachment 8755204 [details] [diff] [review]
Simple patch to stop addressing beyond length of string
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8755204 - Flags: review?(masayuki)
(Assignee)

Updated

a year ago
Keywords: regression, regressionwindow-wanted
Comment on attachment 8755204 [details] [diff] [review]
Simple patch to stop addressing beyond length of string

>       // fallback if the line breaker is unavailable or failed
>       if (!mLineBreaker) {
>         goodSpace = mWrapColumn-prefixwidth;
>+        if (goodSpace >= mCurrentLine.Length()) {
>+          goodSpace = mCurrentLine.Length() - 1;
>+        }
>         while (goodSpace >= 0 &&
>                !nsCRT::IsAsciiSpace(mCurrentLine.CharAt(goodSpace))) {
>           goodSpace--;
>         }
>       }

Of course, looks not bad. However, how about the case if mWrapColumn < prefixWidth? And also if mCurrentLine.Length() is 0?
Attachment #8755204 - Flags: review?(masayuki) → review-
(Assignee)

Comment 4

a year ago
The line that follows the loop is:

if (goodSpace == NS_LINEBREAKER_NEED_MORE_TEXT) {
where
#define NS_LINEBREAKER_NEED_MORE_TEXT -1

So goodSpace==-1 is expected which is what you get with mCurrentLine.Length()==0.

If mWrapColumn < prefixWidth then the crash loop doesn't run. Either mWrapColumn-prefixWidth==-1 or even less, but both are covered below.

I think my change is all we need. The crash happens in the loop when addressing to far.

Please reconsider your decision or what am I missing?
(Assignee)

Comment 5

a year ago
s/to far/too far/
(Assignee)

Updated

a year ago
Flags: needinfo?(masayuki)
You said, "Now, the line breaker was switched off to solve the "Asian crisis", so the code that was previously not run is now run." in comment 1. That means that this block may have other bugs such as I pointed. So, I strongly believe that you should set goodSpace to expected values by following code.

I read nsILineBreaker::Next(). Then, NS_LINEBREAKER_NEED_MORE_TEXT means "not found the word separator". So, indeed, as you said, if mCurrentLine.Length() is 0, goodSpace should be NS_LINEBREAKER_NEED_MORE_TEXT. However, your code is tricky for this case. Could you separate the case like:

if (mCurrentLine.IsEmpty()) {
  goodSpace = NS_LINEBREAKER_NEED_MORE_TEXT;
} else {
  goodSpace =
    std::min(mWrapColumn - prefixwidth, mCurrentLine.Length() - 1);
  while (...) {
    ...
  }
}

This is clearer what the block does. (Although, I'm still worry about the case mWrapColumn - prefwidth < 0. Should we add MOZ_ASSERT() before computing goodSpace?)
Flags: needinfo?(masayuki)
https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/dom/base/nsPlainTextSerializer.cpp#1333

>       if (goodSpace == NS_LINEBREAKER_NEED_MORE_TEXT) {
>         // If we don't found a good place to break, accept long line and
>         // try to find another place to break
>         goodSpace=(prefixwidth>mWrapColumn+1)?1:mWrapColumn-prefixwidth+1;

Hmm, if prefixwidth is larger than mWrapColumn, looks like we should fallback the case to the following this block. So, should it be:

if (mCurrentLine.IsEmpty() || mWrapColumn < prefixwidth) {
  goodSpace = NS_LINEBREAKER_NEED_MORE_TEXT;
} else {
  ...

?
(Assignee)

Comment 8

a year ago
Thanks for looking into it. I'll prepare a new patch within the next 24 hours.
(Assignee)

Comment 9

a year ago
Created attachment 8755435 [details] [diff] [review]
Patch to stop addressing beyond length of string with a little more clean-up

I followed your two suggestions so the patch should pass the review.

Thanks for encouraging a little clean-up to make things clearer even in this old code ;-)
Attachment #8755204 - Attachment is obsolete: true
Attachment #8755435 - Flags: review?(masayuki)
Comment on attachment 8755435 [details] [diff] [review]
Patch to stop addressing beyond length of string with a little more clean-up

Thank *you*!
Attachment #8755435 - Flags: review?(masayuki) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 11

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/acd9850f1f21dd14be319c45fcdcffbffcf09fa8
Bug 1274837 - don't crash by accessing string beyond its length. r=masayuki

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 12

a year ago
Kent should consider inclusion at the earliest convenience. Now we don't insert erroneous spaces for our Asian users, instead we crash ;-(
Flags: needinfo?(rkent)
The choices for timing are:

1) Push to 45.1.1 which we should build this week, even though it did not make our release candidate. I am reluctant to do this, and this bug itself is a good example of how a fix can sometimes cause unforeseen problems.

2) Try to get this in beta 47.0, which we should build next week, then target for 45.2 which we should build in about three weeks. This is what I would prefer. It requires that we add either a Thunderbird 45 version branch for the beta in m-beta, or do a one-time release branch with this patch added.

Opinions yay or nay accepted.
Flags: needinfo?(rkent)
(Assignee)

Comment 14

a year ago
(In reply to Kent James (:rkent) from comment #13)
> Opinions yay or nay accepted.
Can get worse than crashing ;-)
(Assignee)

Comment 15

a year ago
Sorry: I meant to say: CAN'T get worse than crashing ;-)
The crash rate for non-release builds [1] suggests a week or two, especially on beta, would be decent exposure to those areas of code.  And the crash rate on release (0.66%) and lack of angry comments suggests not severe enough that we should to rush it through. So choice #2 for me as well


[1] https://crash-stats.mozilla.com/search/?date=%3E2015-12-01&signature=%3DnsPlainTextSerializer%3A%3AAddToLine&release_channel=!release&_facets=signature&_columns=date&_columns=version&_columns=build_id&_columns=platform&_columns=email&_columns=user_comments#crash-reports
Blocks: 1225864
(In reply to Jorg K (PTO during summer, NI me) from comment #15)
> Sorry: I meant to say: CAN'T get worse than crashing ;-)

There is, for example, crashing more often that is worse. We are living with this crash, there are other regressions I could imagine that would be more disastrous that would result in shutting off of updates.

Comment 18

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/acd9850f1f21
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 19

a year ago
Comment on attachment 8755435 [details] [diff] [review]
Patch to stop addressing beyond length of string with a little more clean-up

Approval Request Comment
[Feature/regressing bug #]: Not a regression, was always wrong, but faulty code path exercised due to fixing bug 333064.
[User impact if declined]: Needs to be uplifted if bug 333064 is uplifted.
[Describe test coverage new/current, TreeHerder]: No special test.
[Risks and why]: Low, just stopped addressing beyond the end of a string.
Also see bug 333064 comment #81.
[String/UUID change made/needed]: None.
Attachment #8755435 - Flags: approval-mozilla-aurora?
Pushed to a release branch on mozilla-beta for Thunderbird 47.0b1
Comment on attachment 8755435 [details] [diff] [review]
Patch to stop addressing beyond length of string with a little more clean-up

Fix a crash, taking it
Attachment #8755435 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 22

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2507c070a55
status-firefox48: --- → fixed
Pushed  to THUNDERBIRD_45_VERBRANCH http://hg.mozilla.org/releases/mozilla-esr45/rev/ec50b82997a8
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → 47+
so far so good https://crash-stats.mozilla.com/search/?product=Thunderbird&version=45.2.0&signature=~nsPlainTextSerializer%3A%3AAddToLine&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.