338251 - [Linux/GTK2] Long text in <pre> causes scary crash [@ nsAutoDrawSpecBuffer::Flush]

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Run (a special?) GTK2 build of Firefox (naked, with gdb, or with Valgrind).
2. Load the testcase.

Result: Crash.

This is with an --enable-default-toolkit=gtk2 build (non-cairo) from May 11.  The build is optimized with symbols, and has a few other changes to make it work well with Valgrind.  Reproducing the bug does not require Valgrind.

I'm marking this as security-sensitive because it includes a stack trace and other information that suggests that this is exploitable.  Other than that, this is pretty discoverable (see all the bugs blocking bug 302294).

If this turns out to be a bug in something other than Firefox, it's still a security hole and we still have to make sure it gets fixed somewhere.

gdb backtrace:

#0  0x00000920 in ?? ()
#1  0x002fca77 in XftGlyphFontSpecCore () from /usr/X11R6/lib/libXft.so.2
#2  0x002f3156 in XftDrawGlyphFontSpec () from /usr/X11R6/lib/libXft.so.2
#3  0x00dc2979 in nsAutoDrawSpecBuffer::Flush (this=0xbffd460c)
    at /work/mozilla/builds/ff/trunk-test/mozilla/gfx/src/gtk/nsFontMetricsXft.cpp:2202
#4  0x00dc5020 in nsFontMetricsXft::DrawString (this=0x8f665e0,
    aString=0x8e7fb48 'b' <repeats 200 times>..., aLength=5026, aX=120,
    aY=285, aSpacing=0x0, aContext=0x9029cf8, aSurface=0x90293d0)
    at /work/mozilla/builds/ff/trunk-test/mozilla/gfx/src/gtk/nsFontMetricsXft.cpp:265
#5  0x00dbfbd8 in nsRenderingContextGTK::DrawString (this=0x9029cf8,
    aString=0x8e7fb48 'b' <repeats 200 times>..., aLength=5026, aX=120,
    aY=285, aSpacing=0x0)
    at /work/mozilla/builds/ff/trunk-test/mozilla/gfx/src/gtk/nsRenderingContextGTK.cpp:1321
#6  0x07a40f71 in nsTextFrame::PaintAsciiText (this=0x895a590,
    aPresContext=0x8ff8328, aRenderingContext=@0x9029cf8,
    aStyleContext=0x895a510, aTextStyle=@0xbffd7c50, dx=120, dy=120)
    at /work/mozilla/builds/ff/trunk-test/mozilla/layout/generic/nsTextFrame.cpp:3923
#7  0x07a4295f in nsTextFrame::PaintText (this=0x895a590,
    aRenderingContext=@0x9029cf8, aPt=0xbffd7d50)

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

When using outside of gdb or Valgrind, the result can vary.   Sometimes I get a crash, sometimes I get a hang, sometimes I get a crash with a message about "memory corruption" or "corrupted double-linked list" or "invalid next size" instead of a segmentation fault.

Comment 3

[Jesse Ruderman]

Attachment: valgrind output

Note "invalid write" in addition to "invalid read", and "Address 0x6453E90 is not stack'd, malloc'd or (recently) free'd".  This code is reading and writing a completely bogus memory address.

Comment 4

[Jesse Ruderman]

I can reproduce in Firefox 1.5.0.3 (which uses GTK2 as its toolkit), so this bug isn't specific to the build for Valgrind testing and isn't a recent regression.

Comment 5

[Jesse Ruderman]

http://talkback-public.mozilla.org/search/start.jsp?search=2&type=iid&id=18778772

Talkback can't resolve symbols in libXft.so.2, so it usually sees this stack:

libXft.so.2 + 0xdc63 (0x002fac63)
libXft.so.2 + 0xf9a2 (0x002fc9a2)
libXft.so.2 + 0x6156 (0x002f3156)
nsAutoDrawSpecBuffer::Flush() 
...

Comment 6

[Jesse Ruderman]

Bug 302294 comment 1 by dbaron seems relevant.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This is an implementation of bug 302294 comment 1. It adds Safe wrappers over various nsIRenderingContext string methods that break long strings up into reasonable-length substrings.

-- GetRangeWidth is not protected with a Safe wrapper. It's only used with Pango which we don't officially support. In any case, hopefully Pango gets things right and doesn't barf in this situation.

-- This may introduce spacing errors due to kerning being disabled where we cut strings up. It will also introduce errors where we cut in the middle of clusters, but we're already hopeless on those without Pango.

-- It fixes all the testcases in bugs dependent on 302294, in my cairo build, except
  -- Selection can make text disappear. This is because we call SetClipRect with a really large width (the width of some string), which appears to cause problems in cairo. It does not seem to lead to crashes, however.
  -- Inline spellchecking can crash on really long words, which I've filed as bug 340050. That is not really related to this bug but it's easy to trigger while playing with testcases.

-- I've tested on a lot of LTR testcases and some RTL, but this really needs significant per-platform testing across a variety of international scripts.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What about the other things that draw text, such as nsTextBoxFrame, nsBulletFrame (harder to trigger), etc.?

Is the GetClusterInfo doing grapheme cluster analysis?  We really probably ought to have a way of doing that internally...

How does the GetClusterInfo API interact with surrogates?  (nsFontMetricsPango::GetClusterInfo's handling of surrogates scares me; it looks broken since it uses pos to index into attrs, and that it indicates that all high surrogates are the start of a cluster.  I'd think you'd want to indicate that all low surrogates are not.)  It seems poorly documented, but the code you wrote will probably work (at least given reasonable implementations).  In fact, if it had implementations everywhere that handled surrogates, you could remove your surrogate check, but we don't.

I suspect most languages allow clusters of arbitrary size since Unicode has combining characters, although maybe not arbitrarily.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In an ideal world, we should be following http://www.unicode.org/reports/tr29/ for where we shouldn't break.)

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #8)
> What about the other things that draw text, such as nsTextBoxFrame,
> nsBulletFrame (harder to trigger), etc.?

I haven't touched them. MathML also has some calls, I think. I guess I could move these functions to nsLayoutUtils and call them from all over.

> Is the GetClusterInfo doing grapheme cluster analysis?  We really probably
> ought to have a way of doing that internally...

It is, but it's only implemented by the Pango gfx/gtk2 font engine, which we don't officially support.

So basically this patch supports clusters no better, and no worse, than the rest of our code.

> I suspect most languages allow clusters of arbitrary size since Unicode has
> combining characters, although maybe not arbitrarily.

Then if someone sends us a cluster of 500+ combining characters, we'll mangle it. I can live with that, especially given that we don't really support clusters.

This is really a branch fix, by the way, since we're hoping to change all this on trunk.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Oops, I forgot to include the Gfx changes. The Thebes and GTK parts are tested, the Windows and Mac changes have not been compiled.

The patch seems to work just as well on GTK2, in fact even better because the clipping issue does not occur.

There is one more issue that I see on both GTK2 and cairo, which is that in text inputs with very long strings, we still get overdrawn text (presumably due to some kind of wraparound). I don't know why this happens.

Another note I forgot to mention --- I haven't made a Safe wrapper for GetBoundingMetrics, instead I bypass it when we're dealing with an overlong string.

I'll revise this patch again to move the Safe* wrappers out to nsLayoutUtils, and call them from other text-displaying frames.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

Okay, this moves the safety wrappers to nsLayoutUtils and updates everything I can find to use them. I also added wrappers for GetBoundingMetrics.

Comment 13

[rbs]

Comment on attachment 224518 [details] [diff] [review]
updated patch

sr=rbs

The patch is reasonnable to me. It also solves the issue of bug 255120 -- which needed precisely such a splitting. So the hack there could be removed. But since non-cairo is are only in maintenance...

A nit:
+  PRInt32 iterations = 0;
+  while (aLength > 0) {
+    if (iterations == 0) {
...
+    }
+    aLength -= len;
+    aString += len;
+    ++iterations;
+  }

I would just use a boolean there to avoid looking after an iterator that nobody needs.
+  PRInt32 firstTime = PR_TRUE;
+  while (aLength > 0) {
+    if (firstTime) {
       firstTime = PR_FALSE;
...
+    }
+    aLength -= len;
+    aString += len;
+  }

Comment 14

[Simon Montagu :smontagu]

I'm not happy about supporting clusters no better than the rest of our code. The rest of our code sucks: see the dependencies of bug 229896. It seems this could cause rendering errors at apparently arbitrary locations in any complex text content (though I suppose text frames containing more than 500 unichars must be rare).

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #14)
> I'm not happy about supporting clusters no better than the rest of our code.
> The rest of our code sucks: see the dependencies of bug 229896.

I know. I want to get some of these branch stability issues out of the way so we can spend more time on trunk work to resolve this situation.

> It seems this could cause rendering errors at apparently arbitrary locations
> in any complex text content (though I suppose text frames containing more than 
> 500 unichars must be rare).

That's my hope. I think we're likely to go over 500 characters only in degenerate cases (like the testcases associated with these bugs) or in preformatted text that happens to be on one line.

Could we introduce a real cluster detector here, say the one defined in UAX#29 that dbaron referenced? I suppose I could implement that in intl/ somewhere.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

... if people agree that it's worth it to avoid making our broken cluster implementation worse.

Comment 17

[Simon Montagu :smontagu]

OK, I buy the argument in comment 15.

Some other miscellaneous remarks and responses:

We need a UAX#29 implementation for other bugs, but I'm not 100% certain that it's the right thing in this context: think about Arabic combining characters, which are not clusters in terms of UAX#29, but splitting text between them will break rendering. (The same goes for ligatures in Latin script, but I think we can live with that).

(In reply to comment #7)
> -- I've tested on a lot of LTR testcases and some RTL, but this really needs
> significant per-platform testing across a variety of international scripts.

Who can do this? I've played with it in Deseret, Hebrew, Devanagari and Arabic, but Linux is not the right place to test it because complex text is hardly supported anyway on non-Pango builds.

(In reply to comment #8)
> I suspect most languages allow clusters of arbitrary size since Unicode has
> combining characters, although maybe not arbitrarily.

Let's not lose any sleep over this. I've seen it suggested on the Unicode list that it is unreasonable to expect sequences of more than 30 combining characters to be processed properly.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #17)
> (In reply to comment #7)
> > -- I've tested on a lot of LTR testcases and some RTL, but this really
> > needs significant per-platform testing across a variety of international
> > scripts.
> 
> Who can do this? I've played with it in Deseret, Hebrew, Devanagari and
> Arabic, but Linux is not the right place to test it because complex text is
> hardly supported anyway on non-Pango builds.

It's not supported any better on Windows or Mac as far as I know. But I would like someone with a Windows box to test the testcases in the dependencies of bug 302294. Jesse perhaps?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

Updated to latest trunk (tree changes)

Comment 20

[Simon Montagu :smontagu]

(In reply to comment #18)
> [complex text]'s not supported any better on Windows or Mac as far as I know. 

It mostly is on WinXP, as long as text doesn't cross selection boundaries or markup boundaries, and doesn't use any of the style properties that make us try to pass it to the rendering engine codepoint by codepoint.

Comment 21

[Simon Montagu :smontagu]

Attachment: Long arabic text

From this testcase it appears that GetClusterInfo() on Pango doesn't identify Arabic connecting characters as clusters either. This surprises me, because I had thought that we couldn't fix bug 75011 on all platforms without implementing GetClusterInfo() (as bug 260663 fixed it for Pango). I wish I had known that sooner...

How does the testcase appear on Mac and Windows? (WinXP with complex text enabled -- flavours of Windows without complex text support will require a different test case).

Comment 22

[Simon Montagu :smontagu]

Comment on attachment 224639 [details] [diff] [review]
updated patch

r=smontagu in principle, but I would still prefer a higher value for MAX_GFX_TEXT_BUF_SIZE, maybe something like 2000?

>+void
>+nsLayoutUtils::SafeDrawString(nsIRenderingContext* aContext,
>+                              const char *aString, PRUint32 aLength,
>+                              nscoord aX, nscoord aY)
>+{
>+  // Since it's ASCII, we don't need to worry about clusters or RTL

Can you duplicate this comment in the char* version of SafeGetWidth() ?

Index: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
-static NS_DEFINE_CID(kWidgetCID, NS_CHILD_CID);
+// nsTreeBodyFrame methods are annotated with UNSAFE if they can be called
+// at times during which it is unsafe to run script ---
+// during painting and reflow.static NS_DEFINE_CID(kWidgetCID, NS_CHILD_CID);

This needs fixing.

>Index: layout/svg/base/src/nsSVGForeignObjectFrame.cpp

You have CVS conflicts here.

Comment 23

[rbs]

> I would still prefer a higher value for
> MAX_GFX_TEXT_BUF_SIZE, maybe something like 2000?

A good value for Windows might be 8192 (which then makes it very rare for the splitting to actually kick in). It should perhaps be #ifdef PLATFORM.

Comment 24

[Jesse Ruderman]

I hope the library/OS bugs are dependent on the number of text characters rather than the width of the rendered text.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry some other stuff got mixed into that patch, as you can tell.

(In reply to comment #24)
> I hope the library/OS bugs are dependent on the number of text characters
> rather than the width of the rendered text.

It probably is sometimes the width. I guess I should test it. We can dynamically vary the segment maximum length to compensate for that.

First I'll check this in with 8000 and then we can see where we stand across platforms.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #25)
> It probably is sometimes the width. I guess I should test it. We can

It's definitely sometimes the width.  X11 isn't happy with coordinates that overflow 16-bit integers.  I think Win95/Win98 have a similar problem, but even to a few bits smaller (14-bit signed?).

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I checked this in a few days ago, sorry I forgot to close the bug.

Now we need testing across platforms to figure out what else needs to be done for the dependencies of bug 302294.

Comment 28

[Boris Zbarsky [:bzbarsky]]

If we take this on branches, we need to take bug 341465 too.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This changed nsIRenderingContext, which also isn't good for branch.

Comment 30

[Darin Fisher]

What's the plan for the 1.8 branch?  Time is running out for FF2 beta1.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This should not go on branch. The patch in bug 237085 is better and obsoletes this one.

Comment 32

[Jesse Ruderman]

Reftests checked in based on the testcase in comment 1.  I tried to design them so they'd fail if text wasn't drawn or overlapped with itself.  (They'll fail for crashes too, of course.)

Simon, can you make a reftest based on the Arabic-text testcase in comment 21?  I don't understand what it's testing.