128855 - buffer overrun with smallcaps [@ nsTextFrame::RenderString]

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[yevgen voronenko]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.8+) Gecko/20020301
BuildID:    2002030116

This bug also applies to 0.9.8 release.
Upgrading to recent build (2002030116) did not help.

I run linux.


Reproducible: Always
Steps to Reproduce:
1. Open URL


Actual Results:  Crash

Expected Results:  Page view ;)

Comment 1

[Mats Palmgren (inactive)]

Crashed for me too, 2002-03-02-23 on Suse 7.3/i386.
Talkback id: TB3623342X

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase

Crash seems to be caused by style="FONT-VARIANT: small-caps"

Comment 3

[Mats Palmgren (inactive)]

-> Layout

Comment 4

[Boris Zbarsky [:bzbarsky]]

#0  0x41b960f7 in nsTextFrame::RenderString (this=0x87aa850, 
    aRenderingContext=@0x862b9b0, aStyleContext=0x87aa81c,
aTextStyle=@0xbfffe0b4, 
    aBuffer=0x8737782, aLength=66, aX=2688, aY=0, aWidth=8128, aDetails=0x0)
    at nsTextFrame.cpp:2711
#1  0x41b96c38 in nsTextFrame::PaintTextSlowly (this=0x87aa850,
aPresContext=0x8783aa8, 
    aRenderingContext=@0x862b9b0, aStyleContext=0x87aa81c,
aTextStyle=@0xbfffe0b4, dx=0, 
    dy=0) at nsTextFrame.cpp:2963
#2  0x41b925a1 in nsTextFrame::Paint (this=0x87aa850, aPresContext=0x8783aa8, 
    aRenderingContext=@0x862b9b0, aDirtyRect=@0xbfffe13c, 
    aWhichLayer=eFramePaintLayer_Overlay, aFlags=0) at nsTextFrame.cpp:1449

(gdb) frame 0
#0  0x41b960f7 in nsTextFrame::RenderString (this=0x87aa850, 
    aRenderingContext=@0x862b9b0, aStyleContext=0x87aa81c,
aTextStyle=@0xbfffe0b4, 
    aBuffer=0x8737782, aLength=66, aX=2688, aY=0, aWidth=8128, aDetails=0x0)
    at nsTextFrame.cpp:2711
2711        *bp++ = ch;
(gdb) p bp
$1 = (PRUnichar *) 0x30
(gdb) p *bp
Cannot access memory at address 0x30

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: full stack

Comment 6

[Boris Zbarsky [:bzbarsky]]

*** Bug 128794 has been marked as a duplicate of this bug. ***

Comment 7

[Andrew Schultz]

The problem is that sp0 is using spacingMem as a buffer when aLength is too
large (183>100).  This is because smallcaps does not trigger the "spacing" flag.

It seems that aTextStyle.mSmallCaps should be added to the list on line
2623-2624. This fixes the crash.

I'm not really sure why sp0 is never allocated more than TEXT_BUF_SIZE when
spacing is 0.  It seems like this crash would show up whenever spacing is 0 and
aLength is large.

Also, there might be problems when (ch==kSZLIG) gets triggered on line 2661. 
the bp0 buffer is allocated at least 2*aLength when smallcaps are used because
of this possibility.  Why doesn't sp0 also get 2*aLength?  This is not an issue
in this particular case because ch is never kSZLIG.

The Russian characters are not neccessary to crash, only a sufficient amount of
text within the smallcaps tag.

Comment 8

[Chris Petersen]

Changing Priority to P1.

Comment 9

[Andrew Schultz]

sp0 is not allocated additional space when spacing is 0 because sp0 is not
needed.  But, in the current implementation, sp0 is still written to (bad).  So,
if we did

if (spacing) *sp++ = glyphWidth;

we would avoid part of the problem. But I still think we should ensure sp0 has
aLength*2 when SmallCaps and spacing are both active.

Comment 10

[Andrew Schultz]

Attachment: patch

ensure at least aLength*2 for sp0 with SmallCaps, and only write to sp when
spacing is on.

Comment 11

[Andrew Schultz]

changing summary in an attempt to get noticed.

Comment 12

[Marc Attinasi]

Comment on attachment 73343 [details] [diff] [review]
patch

sr=attinasi

Comment 13

[Brendan Eich [:brendan]]

How about an r= and then propose the patch to drivers for 1.0?  Marc, can you
give this bug to someone if you're overloaded?  Bz, maybe you'll take it?

/be

Comment 14

[Chris Waterson]

Comment on attachment 73343 [details] [diff] [review]
patch

>Index: nsTextFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsTextFrame.cpp,v
>retrieving revision 1.359
>diff -u -r1.359 nsTextFrame.cpp
>--- nsTextFrame.cpp	2002/03/05 23:23:09	1.359
>+++ nsTextFrame.cpp	2002/03/09 03:42:34
>@@ -2611,24 +2611,26 @@
> {
>   PRUnichar buf[TEXT_BUF_SIZE];
>   PRUnichar* bp0 = buf;
>+
>+  nscoord spacingMem[TEXT_BUF_SIZE];
>+  PRIntn* sp0 = spacingMem; 

Uh, this is a weird aliasing. Don't you want |sp0| to be declared as
an |nscoord *|? (Likewise with |sp|, below?)

>+  PRBool spacing = (0 != aTextStyle.mLetterSpacing) ||
>+    (0 != aTextStyle.mWordSpacing) || aTextStyle.mJustifying;
>+
>   //German 0x00df might expand to "SS", but no need to count it for speed reason
>   if (aTextStyle.mSmallCaps) {
>-     if (aLength*2 > TEXT_BUF_SIZE)
>+     if (aLength*2 > TEXT_BUF_SIZE) {
>        bp0 = new PRUnichar[aLength*2];
>+       if (spacing) sp0 = new nscoord[aLength*2];

Move the assignment to a separate line.

>+     }
>   }
>   else if (aLength > TEXT_BUF_SIZE) {
>     bp0 = new PRUnichar[aLength];
>+    if (spacing) sp0 = new nscoord[aLength];

Ibid.

>   }
>-  PRUnichar* bp = bp0;
> 
>-  PRBool spacing = (0 != aTextStyle.mLetterSpacing) ||
>-    (0 != aTextStyle.mWordSpacing) || aTextStyle.mJustifying;
>-  nscoord spacingMem[TEXT_BUF_SIZE];
>-  PRIntn* sp0 = spacingMem; 
>-  if (spacing && (aLength > TEXT_BUF_SIZE)) {
>-    sp0 = new nscoord[aLength];
>-  }
>+  PRUnichar* bp = bp0;
>   PRIntn* sp = sp0;

Why isn't |sp| an |nscoord *|?

> 
>   nsIFontMetrics* lastFont = aTextStyle.mLastFont;
>@@ -2661,7 +2663,7 @@
>       if (ch == kSZLIG)   //add an additional 'S' here.
>       {
>         *bp++ = upper_ch;
>-        *sp++ = glyphWidth;
>+        if (spacing) *sp++ = glyphWidth;

Move assignment to the next line.

>         width += glyphWidth;
>       }
>       ch = upper_ch;
>@@ -2711,7 +2713,7 @@
>       lastFont = nextFont;
>     }
>     *bp++ = ch;
>-    *sp++ = glyphWidth;
>+    if (spacing) *sp++ = glyphWidth;

Ibid.

>     width += glyphWidth;
>   }
>   pendingCount = bp - runStart;

Comment 15

[Marc Attinasi]

Over to waterson - thanks!

Comment 16

[Andrew Schultz]

> >   PRIntn* sp = sp0;
> 
> Why isn't |sp| an |nscoord *|?

I hadn't noticed that, but it does seem a bit strange.  Recompiling updated CVS
with a new patch...

Comment 17

[Andrew Schultz]

Attachment: patch v2

split lines and use nscoord.

Comment 18

[Chris Waterson]

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

sr=waterson. thanks!

Comment 19

[Marc Attinasi]

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

r=attinasi

Comment 20

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 21

[Chris Waterson]

Fix checked in. Thanks Andrew!

Comment 22

[Andrew Schultz]

*** Bug 144636 has been marked as a duplicate of this bug. ***

Comment 23

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/54417ebbaea2