Last Comment Bug 265736 - crash [@ QBCurve::SubDivide ][@ QBCurve::MidPointDivide ][@ nsCSSRendering::DrawTableBorderSegment]
: crash [@ QBCurve::SubDivide ][@ QBCurve::MidPointDivide ][@ nsCSSRendering::D...
Status: VERIFIED FIXED
[sg:critical]
: crash, testcase, verified1.7.13, verified1.8
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: x86 All
: -- critical with 3 votes (vote)
: ---
Assigned To: Bernd
: Hixie (not reading bugmail)
Mentors:
: 291204 293560 (view as bug list)
Depends on: 316821
Blocks: 27650 243671 Zalewski iexploder
  Show dependency treegraph
 
Reported: 2004-10-23 04:00 PDT by Pavel Penaz
Modified: 2014-04-26 03:17 PDT (History)
14 users (show)
dveditz: blocking1.7.13+
dveditz: blocking‑aviary1.0.8+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (terminates Mozilla/Firefox) (248 bytes, text/html)
2004-10-23 04:18 PDT, Pavel Penaz
no flags Details
testcase in plain text format (14.16 KB, text/plain)
2004-10-23 04:31 PDT, Peter van der Woude [:Peter6]
no flags Details
Partial backtrace (13.90 KB, text/plain)
2004-10-23 05:35 PDT, Ben Fowler
no flags Details
Smaller testcase (42 bytes, text/html)
2004-10-28 08:59 PDT, Nathan Strom
no flags Details
Another simplified test case, using an iframe (crashes FF 1.5 beta) (134 bytes, text/html)
2005-10-21 12:29 PDT, Thomas Stromberg
no flags Details
debug help to illustrate the issue (965 bytes, patch)
2005-10-28 00:35 PDT, Bernd
no flags Details | Diff | Splinter Review
patch (6.06 KB, patch)
2005-10-29 00:20 PDT, Bernd
roc: review+
roc: superreview+
Details | Diff | Splinter Review
fix the other holes (5.54 KB, patch)
2005-10-30 12:50 PST, Bernd
roc: review+
roc: superreview+
Details | Diff | Splinter Review
same as above only with quality control of the approximation (6.82 KB, patch)
2005-10-30 13:49 PST, Bernd
no flags Details | Diff | Splinter Review
patch against branch (9.15 KB, patch)
2005-11-01 11:48 PST, Bernd
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
mtschrep: approval1.8rc2+
Details | Diff | Splinter Review

Description Pavel Penaz 2004-10-23 04:00:34 PDT
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 2004-10-23 04:18:43 PDT
Created attachment 163121 [details]
testcase (terminates Mozilla/Firefox)

Stripped down version
Comment 2 Pavel Penaz 2004-10-23 04:30:12 PDT
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] 2004-10-23 04:31:22 PDT
Created attachment 163122 [details]
testcase in plain text format

extremely corrupt file
does not crash Opera/IE

attached in txt format (to prevent crash)
Comment 4 Pavel Penaz 2004-10-23 04:36:25 PDT
Comment on attachment 163121 [details]
testcase (terminates Mozilla/Firefox)

terminates all browser windows
Comment 5 Ben Fowler 2004-10-23 05:35:54 PDT
Created attachment 163125 [details]
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 2004-10-23 06:10:44 PDT
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 2004-10-23 07:34:54 PDT
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 2004-10-23 09:02:31 PDT
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 2004-10-23 09:07:32 PDT
Is this related to Bug 264944 ?
Comment 10 Pavel Penaz 2004-10-23 16:23:26 PDT
(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+2 (mostly busy through August 4; review requests must explain patch) 2004-10-23 17:59:41 PDT
So this actually doesn't crash for me on Linux...
Comment 12 Jacek Piskozub 2004-10-24 01:33:40 PDT
It does for me on Windows ME.

Talkback number:
TB1488879W
Comment 13 Hermann Schwab 2004-10-24 08:43:29 PDT
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 2004-10-27 23:51:58 PDT
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 2004-10-28 08:59:57 PDT
Created attachment 163706 [details]
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 [:bz] 2004-10-29 09:14:39 PDT
This is basically the same as bug 27650 and bug 243671, no?
Comment 17 Jacek Piskozub 2004-10-29 09:45:36 PDT
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] 2005-03-24 06:31:55 PST
This now WFM on Firefox 1.0.2 pl-PL on Linux. Can anyone else confirm it's WFM?
Comment 19 Nathan Strom 2005-03-24 07:33:37 PST
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 2005-03-28 23:08:39 PST
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 22 timeless 2005-05-11 19:30:13 PDT
*** Bug 293560 has been marked as a duplicate of this bug. ***
Comment 23 Nathan Strom 2005-09-09 06:24:23 PDT
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 2005-09-09 06:25:32 PDT
(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 2005-10-21 12:29:03 PDT
Created attachment 200375 [details]
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 2005-10-27 09:20:28 PDT
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 2005-10-28 00:35:18 PDT
Created attachment 201098 [details] [diff] [review]
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 2005-10-28 04:37:40 PDT
(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] 2005-10-28 08:16:02 PDT
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 2005-10-28 09:32:34 PDT
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 2005-10-29 00:20:28 PDT
Created attachment 201239 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2005-10-29 04:47:39 PDT
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 2005-10-29 07:00:23 PDT
*** Bug 291204 has been marked as a duplicate of this bug. ***
Comment 34 Bernd 2005-10-29 11:00:22 PDT
fixed on trunk
Comment 35 Bernd 2005-10-29 11:04:42 PDT
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 2005-10-30 02:58:04 PST
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 2005-10-30 05:02:21 PST
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 2005-10-30 05:29:05 PST
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 2005-10-30 07:45:02 PST
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 2005-10-30 12:50:39 PST
Created attachment 201370 [details] [diff] [review]
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 2005-10-30 13:20:02 PST
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 2005-10-30 13:49:23 PST
Created attachment 201376 [details] [diff] [review]
same as above only with quality control of the approximation

this terminates the stack much earlier
Comment 43 Stephen Donner [:stephend] 2005-10-30 15:27:55 PST
(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 2005-10-31 22:39:59 PST
All testcases in this bug are fixed for me. Good work!
Comment 45 Bernd 2005-11-01 03:10:21 PST
fix checked in
Comment 46 Bernd 2005-11-01 03:11:22 PST
Comment on attachment 201239 [details] [diff] [review]
patch

I will make a new patch against branch for post 1.5rc1
Comment 47 Bernd 2005-11-01 11:48:51 PST
Created attachment 201541 [details] [diff] [review]
patch against branch

checking against for array boundaries is pretty safe, btw. this is the last known Mangleme chrash.
Comment 48 Mike Schroepfer 2005-11-01 14:29:43 PST
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 2005-11-02 22:05:46 PST
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 2005-11-03 02:45:40 PST
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 2005-11-03 09:12:17 PST
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 2005-11-03 11:13:56 PST
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 2005-11-03 11:46:19 PST
DBaron - can you comment on risk of taking this at this point in the cycle?
Comment 54 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2005-11-03 13:34:20 PST
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 2005-11-04 13:25:16 PST
fixed on branch
Comment 56 Bob Clary [:bc:] 2005-11-10 08:39:21 PST
no crash in firefox 1.5 rc winxp/linux
Comment 57 Daniel Veditz [:dveditz] 2006-02-06 09:54:58 PST
Comment on attachment 201541 [details] [diff] [review]
patch against branch

a=dveditz
Comment 58 Marcia Knous [:marcia - use ni] 2006-02-10 17:33:28 PST
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 - use ni] 2006-03-07 13:32:31 PST
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:] 2009-03-31 06:48:22 PDT
changeset:   25010:b0337b6287f3
user:        Jesse Ruderman <jruderman@gmail.com>
date:        Fri Feb 13 14:54:17 2009 -0800
summary:     Add crashtests

Note You need to log in before you can comment on or make changes to this bug.