321487 - Crash when moving caret backwards over text frame consisting of a single CR [nsTextFrame::PeekOffset]

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Uri Bernstein]

This is a followup on bug 319705 (see bug 319705 comment #9, 2nd paragraph).

If, using script, you create a DOM text node consisting of a single CR (0x0d), moving the caret backwords over this text crashes the browser (at least on OS X. Since this is a case of uninitialized memory used as an array subscript, other OSs might not crash).

I'll attach a testcase immediately. Here's my analysis of the problem (based bug 319705 comment #6 and #7):

What happens is that PrepareUnicodeText() in this case leaves
indexBuffer.mBuffer uninitialized (the single character in the content is not
mapped to anything). Later, when nsTextFrame::PeekOffset() examines ip[i]
(which is really indexBuffer.mBuffer[0]), it finds junk there, and from there
on all kinds of things can happen.
The reason PrepareUnicodeText() does not init indexBuffer.mBuffer[0] is that
the call to aTX.GetNextWord() returns nsnull, which then causes an immediate
break out of the loop.
I can't say whether GetNextWord() really should return nsnull in this case (and
then it should just be handled better by PrepareUnicodeText()), or whether the
problem is in GetNextWord() itself.

Comment 1

[Uri Bernstein]

Attachment: evil testcase

Load the testcase in the browser, then:
- Turn on caret browsing mode (F7)
- Place the caret in the word "World"
- Use left-arrow to move into the word "Hello"

You should crash as you try to move over the (invisible) CR between the two words.

The crash is in the following line:
            if ((ip[i] < ip[aPos->mStartOffset - mContentOffset]) &&
                (clusterBuffer.mBuffer[ip[i] - mContentOffset]) &&
                (! IS_LOW_SURROGATE(paintBuffer.mBuffer[ip[i]-mContentOffset])))

i=0. ip[0] is junk, so what actually crashes is probably |clusterBuffer.mBuffer[ip[i] - mContentOffset]|

Comment 2

[Uri Bernstein]

All/All because the underlying problem is clearly cross-platform (regardless whether other platforms actually crash).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Is this the sort of thing that could cause jumps into random memory?  Or just reads of random memory?

Comment 4

[Uri Bernstein]

(In reply to comment #3)
> Is this the sort of thing that could cause jumps into random memory?  Or just
> reads of random memory?

It seems to me that this can not cause random-memory jumps, only reads.

BTW, I had a chance to test the attached testcase on Windows, and it did not crash (not that this matters much).

Comment 5

[Boris Zbarsky [:bzbarsky]]

So which case do we end up hitting in PrepareUnicodeText?  Do we get isWhitespace equal to true?  Or no?

Comment 6

[Uri Bernstein]

(In reply to comment #5)
> So which case do we end up hitting in PrepareUnicodeText?  Do we get
> isWhitespace equal to true?  Or no?

No, isWhitespace is false - but we never get to the point where we ask about it.
We get into |if (nsnull == bp)| (line 2118), and thus break out of the loop (line 2227). From there on, nothing sets aIndexBuffer.mBuffer[0].

Note that empirically this appears to be a regression: I'm not crashing with builds from 2005-04-30 and before (first crashing build that I found so far is 2005-05-03).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hmm.  Does GetNextWord() return non-null for a single space, say?

Comment 8

[Uri Bernstein]

(In reply to comment #7)
> Hmm.  Does GetNextWord() return non-null for a single space, say?

Yes, it returns a non-null for a single space.

Comment 9

[Uri Bernstein]

Attachment: testcase using ­

What causes \r to behave differently (than e.g. a space) in nsTextTransformer::GetNextWord() is that it's explicitly specified in the IS_DISCARDED macro. This is also true for ­ - this testcase demonstartes the same crash, with ­ which does not require script to be inserted. Instructions for crashing are as in the previous testcase.

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK.  And the same would happen with BiDi control chars.  So what do we need to do in PrepareUnicodeDimensions to deal with a sequence of discarded chars and nothing else?  rbs, any ideas?

Comment 11

[Uri Bernstein]

Attachment: patch?

Apparently, there is already a solution (or at least a "solution") for this in the bidi case. So here I'm just adopting that solution for the general case.
This does fix my two testcases, but I don't really know the code well enough to explain why initializing the index buffer this way is (or isn't) correct (or even acceptable) in this case.

Comment 12

[Karsten Düsterloh]

(In reply to comment #9)
> Created an attachment (id=206805) [edit]
> testcase using ­

Probably related: bug 319914.

Comment 13

[Frank Wein [:mcsmurf]]

Yeah that patch fixes the ­ issue from Bug 319914. Note pasting ­ (and moving cursor over it in a debug build) actually worked once (meaning did not crash), this has regressed in 2004, for some rough regression range see the Bug.

Comment 14

[Uri Bernstein]

Notice bug 244876 which would explain why this did not crash before 2004-05-31 (but not why it started crashing only about a year later).
It's possible that whoever wrote this code actually relied on newly-initialized buffers to be zero-filled (as they were before bug 244876 was fixed).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Do we want this on branches?  Seems like we would...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 206812 [details] [diff] [review]
patch?

Looks reasonable.

Comment 17

[rbs]

-#ifdef IBMBIDI
-      if (indexp && (mState & NS_FRAME_IS_BIDI) ) {
+      if (indexp) {
         while (--n >= 0) {
           *indexp++ = strInx++;
         }
       }
-#endif // IBMBIDI


I wonder why Bidi is doing what it is doing. For normal cases, it seems to me that strInx shouldn't be incremented because it is a bit at odd with the spirit of that code.

For future refences, here is what that code is doing. Assume for simplicity mContentOffset = 0, and suppose the corresponding DOM content of the nsTextFrame has these four characters:

0    1      2     3   4         mContentLength = 5
a|ß|­|­|b

Now, assume that it has the CSS "text-transform: uppercase". Hence the German ß "expands" to SS and ­ "shrinks" to nothing. PrepareUnicodeText is meant to deal with both situations: expansion and contraction. In the example, the prepared output becomes:

0 12 3     <--- textlength = 4
A|SS|B

What PrepareUnicodeText does is to encode this transformation. To do so, indexp will run up to mContentLength and record the _beginning_ of the mapped "segment" correspond to each DOM character. Thus:

indexp[]: runs from 0 to 5, and maps 
a|&szlig;|&shy;|&shy;|b
0|   1   |  2  |  3  |4|5    <--- this is indexp
                        ^ extra
to:

0|   1   |  3  |  3  |3|4    <--- this is strInx
                        ^ textlength

In other words, the i-th DOM character is representable with
the peparedText[ indexp[i] : indexp[i + 1] - 1 ].

You can see how the extra is set from this last bit of code:

  if (aIndexBuffer) {
    PRInt32* ip = aIndexBuffer->mBuffer;
    ip[mContentLength] = ip[mContentLength-1];
    if ((ip[mContentLength] - mContentOffset) < textLength) {
      // Must set up last one for selection beyond edge if in boundary
      ip[mContentLength] = textLength + mContentOffset;
    }
  }

Note how it does not increment ip[mContentLength] if the content has indeed collapsed to nothingness.

Given that the spirit of the code is to map the i-th DOM character to the corresponding peparedText[ indexp[i] ... indexp[i + 1] - 1 ], incrementing is creating fictional (unset) references. Can you just try:

+      if (indexp) {
         while (--n >= 0) {
-          *indexp++ = strInx++;
           *indexp++ = strInx;
         }
       }

Comment 18

[Uri Bernstein]

Attachment: patch v2

Yes, this works as well, and makes more sense.
Thanks for the explanation in comment #17 - I now understand this better.

Comment 19

[rbs]

Comment on attachment 206907 [details] [diff] [review]
patch v2

r=rbs

Comment 20

[Uri Bernstein]

Patch checked in, 2005-12-27 07:15.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 206907 [details] [diff] [review]
patch v2

a=dveditz for drivers

Comment 22

[Uri Bernstein]

Checked in to 1.8 and 1.8.0 branches:

Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.513.4.7; previous revision: 1.513.4.6
done

Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.513.4.6.2.1; previous revision: 1.513.4.6
done

Comment 23

[Jay Patel [:jay]]

v.fixed on 1.8.0.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060109 Firefox/1.5.0.1, no crash with uri's testcase.