265736 - crash [@ QBCurve::SubDivide ][@ QBCurve::MidPointDivide ][@ nsCSSRendering::DrawTableBorderSegment]

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[Pavel Penaz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041022 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041022 Firefox/1.0

After visiting the URl Firefox terminates without any warning.

Reproducible: Always
Steps to Reproduce:
1. go to the URL: http://www.home.karneval.cz/10000565/test.html


Actual Results:  
Firefox terminates

Expected Results:  
Firefox should not terminate

I found this page using the mangle.cgi script. I'll attach a testcase soon.

Comment 1

[Pavel Penaz]

Attachment: testcase (terminates Mozilla/Firefox)

Stripped down version

Comment 2

[Pavel Penaz]

The same thing happens with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.8a5) Gecko/20041023

Product -> Browser

Comment 3

[Peter van der Woude [:Peter6]]

Attachment: testcase in plain text format

extremely corrupt file
does not crash Opera/IE

attached in txt format (to prevent crash)

Comment 4

[Pavel Penaz]

Comment on attachment 163121 [details]
testcase (terminates Mozilla/Firefox)

terminates all browser windows

Comment 5

[Ben Fowler]

Attachment: Partial backtrace

There are roughly 1070 similar lines at the head of this trace, followed by
a BAD_ACCESS fault.

This is a True Bill.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041023
Firefox/0.9.1+

Comment 6

[Ben Fowler]

See
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSRendering.cpp#3943

(My lines are out - backtrace is from my own tree)

Whilst smag is > 1.0, the function QBCurve::SubDivide( ) is called
recursively. On the page in question, smag gets stuck at 16.0; in
short, QBCurve::SubDivide( ) is not guaranteed to terminate. 

Comment 7

[Ben Fowler]

A day I don't learn something is a day wasted. My previous post was not
altogether wrong, but is highly misleading: EXC_BAD_ACCESS does not (necessarily)
mean what I thought it did, at least on Mach systems like Mac OS X.

I am fairly sure that all that this signal means is that the stack is
exhausted (gdb which caught the Mach exception) shows that that all the
variables have reasonable values.

I suspect that I need to look at least one level up, and there may not be
a portable way of protecting QBCurve::SubDivide( ) from excessive
recursion.

Comment 8

[Ben Fowler]

O.K. The function GetPath( ) 
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSRendering.cpp#3871
creates a polygon from a Bezier curve, and if it needs to, it subdivides the
path. So far as I can tell, the points of the (new) path are stored in an array
on the stack which can grow. I think that I have found that during normal
browsing, the path can become as long as 68 elements (it is normally only 3 or 4),
but whilst trying to render the page in question, it gets to just over 2100
elements before the kernel(?) detects that that the stack has overflowed
and sends Firefox a lethal signal.

I have picked a nice round number between 68 and 2100, and added this spackle
to layout/html/style/src/nsCSSRendering.cpp :


@@ -3928,9 +3952,15 @@
 void 
 QBCurve::SubDivide(nsIRenderingContext *aRenderingContext,nsPoint
aPointArray[],PRInt32 *aCurIndex)
 {
 QBCurve   curve1,curve2;
 float     fx,fy,smag;
 
+  // Spackle
+  if( *aCurIndex > 255 ) {
+    NS_WARNING( "QBCurve::SubDivide( ) Premature return to avoid excessive
recursion" );
+    return;
+  }
+  
   // divide the curve into 2 pieces
   MidPointDivide(&curve1,&curve2);
 
which appears to fix the presenting problem.

See also http://it.slashdot.org/it/04/10/19/0236213.shtml and many similar
pages

Comment 9

[Ben Fowler]

Is this related to Bug 264944 ?

Comment 10

[Pavel Penaz]

(In reply to comment #9)
> Is this related to Bug 264944 ?

It is related since I came across this crash using the mangle.cgi script
provided by Zalewski. Should this be set as blocking bug 264944?

Comment 11

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

So this actually doesn't crash for me on Linux...

Comment 12

[Jacek Piskozub]

It does for me on Windows ME.

Talkback number:
TB1488879W

Comment 13

[Hermann Schwab]

tested on the testcase, Win98:

TB1492724  QBCurve::SubDivide f10b57f3 Stack overflow
QBCurve::SubDivide line 3935, line 3946 ... line 3946

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

tested on the URL, WinME (comment 12)

TB1488879W  MSVCRT.DLL + 0x2ee1b (0x7802ee1b) a2c53108 Stack overflow
QBCurve::SubDivide line 3984, line 3990 ... line 3990

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

Comment 14

[Adam Hauner]

FF1.0RC1/W2K -> TB1567914Q, TB1567897H

There are also crashes on Thunderbird (TB1295106, TB1336886) and on Mac OS X
(TB1404203, TB1336415). Incident from Linux not found.

Comment 15

[Nathan Strom]

Attachment: Smaller testcase

Smaller testcase. Crashes for me on:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.3) Gecko/20041026
Firefox/1.0RC1

Testcase is simply:
<HTML>
<HR WIDTH=4444444 COLOR="#000000">

Comment 16

[Boris Zbarsky [:bzbarsky]]

This is basically the same as bug 27650 and bug 243671, no?

Comment 17

[Jacek Piskozub]

Maybe basically it is but the testcase of this very bug crashes Firefox 1.0rc1
(WindowsME) while the "webtv" kibo's page (bug 27650 and bug 243671) and the
simplified testcase in one of them WFM.

Comment 18

[Marek Stępień [:marcoos, inactive]]

This now WFM on Firefox 1.0.2 pl-PL on Linux. Can anyone else confirm it's WFM?

Comment 19

[Nathan Strom]

I just tried attachment 163706 [details] with Mozilla/5.0 (Windows; U; Windows NT 5.0;
en-US; rv:1.7.6) Gecko/20050317 Firefox/1.0.2 and got a still got a crash,
TB4571361X.

Comment 20

[Charles Fenwick]

Crashed on both the testcase and reduced testcase using Mozilla/5.0 (Windows; U;
Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050328 

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=4676626#id

and

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=4676687#id

Comment 21

[Pavel Penaz]

Talkback:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB5212695E

Comment 22

[timeless]

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

Comment 23

[Nathan Strom]

FWIW, still crashes with 1.5 beta --> TB9179277E

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

Comment 24

[Nathan Strom]

(In reply to comment #23)
> FWIW, still crashes with 1.5 beta --> TB9179277E
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908
Firefox/1.4

Different stack signature, though: nsCSSSelector::ToStringInternal 8163ab69

Comment 25

[Thomas Stromberg]

Attachment: Another simplified test case, using an iframe (crashes FF 1.5 beta)

Here's another simplified test case that I made today with iExploder. Here's
the part that breaks Firefox 1.4.1 (1.5 beta) on both Windows and Mac OS X:

<iframe style="border-top-width: 31378748; -moz-border-radius-bottomright:
23895784; ">

Comment 26

[M. LaPlante]

Just tested this with the 1.5 RC1 test build out today, and the simplified test case above still crashes FF. As per Asa's request (http://weblogs.mozillazine.org/asa/archives/2005/10/zarro_boogs_n_1.html) I'm nominating this since it's a 'critical' issue that hasn't been addressed.  This sort of seems like the kind of bug that may wind up on Secunia if it stays around too long...

Comment 27

[Bernd]

Attachment: debug help to illustrate the issue

I think http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/base&command=DIFF_FRAMESET&file=nsCSSRendering.cpp&rev2=3.49&rev1=3.48
is crap. Yes I have written also bad code but this code is not tolerable. It does not check for array boundaries, it does not use voidArray.
It got checked in in the good old days 1999-04-18 22:27 without review and sr

Comment 28

[Ben Fowler]

(In reply to comment #27)
> Created an attachment (id=201098) [edit]
> debug help to illustrate the issue
> 
> I think ... this code is not tolerable. It does not check for array 
> boundaries, it does not use voidArray. ...

More or less a year ago to the day, within a matter of hours of the report,
and in full view of the Slashdot gallery, three people commented on this
bug and a backtrace and patch (for a file within the layout code) were posted.

O.K. I am not part of the Mozilla crew, but I am sure that it would be helpful
if you could add a note to this bug as to whether we should have done
less (left the layout and potential security problems to the experts) or
more (if so what).

Comment 29

[Stephen Donner [:stephend] Not actively reading bugmail]

roc told me this problem will go away when Cairo lands, but I haven't heard when that's planning to happen...

Comment 30

[Bernd]

Ben, sorry for my ignorance. I had a good share of the 264944 blocking bugs. I looked that it still blocks 1.5rc1 and thought ohh that is still open. Before that I could not reproduce it. But with testcase from oct. 21 I could. Then I looked at the attachments, no patch, so I skipped what I thought is noise. I am really sorry. What you should have done differnetly is to attach a patch and to mark the attachment as a patch and ask for review. 

However I stand for my comment that this code is prone to crash and needs to be a) removed or b) rewritten.

Comment 31

[Bernd]

Attachment: patch

array boundary protection
Robert one could of course just place a test in SubDivide for aCurIndex >= MAXPOLYPATHSIZE - 20 and thats it,

Comment 32

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

Ben, I'm sorry this slipped through the cracks. What bernd said in comment #30... Plus, if you're feeling frustrated, an emailed complaint to me would have got my attention.

Comment 33

[Bernd]

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

Comment 34

[Bernd]

fixed on trunk

Comment 35

[Bernd]

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

This might be good for branch as it fixes a bug that made us bad publicity after 1.0 and is low risk.

Comment 36

[Pavel Penaz]

Firefox still crashes on the testcases except on the iframe testcase -> REOPENING

Talkback: http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB11243010K

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051030 Firefox/1.6a1 ID:2005103001

Comment 37

[Bernd]

the talkback says as build ID 2005102905 while the checkin was only 2005-10-29 06:40 are you certain that the build has the patch already included?

Comment 38

[Pavel Penaz]

Strange, now I tested again with a new build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051030 Firefox/1.6a1 ID:2005103004

I crashed again, talkback: http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB11246101X

The weird thing is that talkback says build ID is 2005103001 while the build is actually 2005103004.

Nevertheless Firefox still crashed so I guess something is still wrong..

Comment 39

[Ben Fowler]

The build ID is established by a perl script which is set to run
when gecko is configured. In my hands it does not get run when the 
config is up to date, but I presume that it always is when doing a 
CLOBBER build.

Comment 40

[Bernd]

Attachment: fix the other holes

Robert it seems that I overlooked some places in the first patch, if that does help we need to limit the calling stack as Ben proposed.

The problem its floating point arithmetic  where we expect infinite precision but work with large numbers

Comment 41

[Bernd]

Robert as the causing issue is a floating point arithmetic problem, can you say a timescale when that cairo switch will happen. 

Comment 42

[Bernd]

Attachment: same as above only with quality control of the approximation

this terminates the stack much earlier

Comment 43

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #42)
> Created an attachment (id=201376) [edit]
> same as above only with quality control of the approximation
> 
> this terminates the stack much earlier

I patched my local tree and this patch fixes all of the testcases in this bug.

Comment 44

[Pavel Penaz]

All testcases in this bug are fixed for me. Good work!

Comment 45

[Bernd]

fix checked in

Comment 46

[Bernd]

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

I will make a new patch against branch for post 1.5rc1

Comment 47

[Bernd]

Attachment: patch against branch

checking against for array boundaries is pretty safe, btw. this is the last known Mangleme chrash.

Comment 48

[Mike Schroepfer]

Comment on attachment 201541 [details] [diff] [review]
patch against branch

Closing down for the 1.5/1.8 release at this point.  Given this is a specific testcase with no known security issues we'll defer this for the next release.

Comment 49

[Bernd]

Jesse could you please verify Mikes security assessment.  I have doubts that it is correct.I do not have enough knowledge distinguish a crash from a security issue. What we have here is stack overflow due to recursion and past boundary array access. 
The crash here is public as  502701_7-FireFox-1.4.1-Crash.html      from the iexploder crash test galery (http://toadstool.se/software/iexploder/testcases/)

Comment 50

[Jesse Ruderman]

Stack overflows (excessive recursion) are not security holes, just crashes.

Can you give me more detail about the other issue in this bug?  (Writing past the end of an array is usually a serious security hole, while reading past the end of an array can cause privacy leaks.  It looks like the array in question is a static variable in a function, which may or may not affect the severity of reading/writing past the end of the array.)

Comment 51

[Bernd]

the array in question is defined at http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSRendering.cpp#3300 and since we can increase the index without boundary we might access for writing array elements beyond MAXPOLYPATHSIZE 

Comment 52

[Jesse Ruderman]

Comment on attachment 201541 [details] [diff] [review]
patch against branch

Writing past the end of an array sounds like a likely security hole to me.

Comment 53

[Mike Schroepfer]

DBaron - can you comment on risk of taking this at this point in the cycle?

Comment 54

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

This looks pretty low risk -- for the array access cases, it's only changing behavior in cases where we would actually write past the end of an array.  The stack overflow case is a little less trivial to analyze, but it looks reasonably safe as well -- as long as rounded borders and radio buttons and such still look OK, we should be fine.

Comment 55

[Bernd]

fixed on branch

Comment 56

[Bob Clary [:bc] (inactive)]

no crash in firefox 1.5 rc winxp/linux

Comment 57

[Daniel Veditz [:dveditz]]

Comment on attachment 201541 [details] [diff] [review]
patch against branch

a=dveditz

Comment 58

[Marcia Knous [:marcia]]

verified on the 1.0.1 branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20060210 Firefox/1.0.7. I don't crash with the first testcase listed (https://bugzilla.mozilla.org/attachment.cgi?id=163121). Adding verified keyword.

Comment 59

[Marcia Knous [:marcia]]

verified on the 1.7 branch using Mozilla 1.7.13 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060302. This testcase - https://bugzilla.mozilla.org/attachment.cgi?id=163121 does not crash.

Comment 60

[Bob Clary [:bc] (inactive)]

changeset:   25010:b0337b6287f3
user:        Jesse Ruderman <[e-mail]>
date:        Fri Feb 13 14:54:17 2009 -0800
summary:     Add crashtests