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

VERIFIED FIXED

Status

()

Core
Layout: View Rendering
--
critical
VERIFIED FIXED
13 years ago
3 years ago

People

(Reporter: Pavel Penaz, Assigned: Bernd)

Tracking

(4 keywords)

Trunk
x86
All
crash, testcase, verified1.7.13, verified1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature)

Attachments

(10 attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
Created attachment 163121 [details]
testcase (terminates Mozilla/Firefox)

Stripped down version
(Reporter)

Updated

13 years ago
Keywords: testcase
(Reporter)

Comment 2

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

Product -> Browser
Component: General → Browser-General
Product: Firefox → Browser
Version: unspecified → Trunk
Created attachment 163122 [details]
testcase in plain text format

extremely corrupt file
does not crash Opera/IE

attached in txt format (to prevent crash)
(Reporter)

Comment 4

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

terminates all browser windows
(Reporter)

Updated

13 years ago
Attachment #163121 - Attachment description: testcase → testcase (terminates Firefox)
(Reporter)

Updated

13 years ago
Attachment #163121 - Attachment description: testcase (terminates Firefox) → testcase (terminates Mozilla/Firefox)

Comment 5

13 years ago
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

13 years ago
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. 
Assignee: firefox → dbaron
Status: UNCONFIRMED → NEW
Component: Browser-General → Style System (CSS)
Ever confirmed: true
Keywords: crash
QA Contact: firefox.general → ian

Comment 7

13 years ago
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

13 years ago
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

13 years ago
Is this related to Bug 264944 ?
(Reporter)

Comment 10

13 years ago
(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?
Blocks: 264944
So this actually doesn't crash for me on Linux...

Comment 12

13 years ago
It does for me on Windows ME.

Talkback number:
TB1488879W
Whiteboard: talkback

Comment 13

13 years ago
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
(Reporter)

Updated

13 years ago

Comment 14

13 years ago
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.
OS: Windows XP → All
Summary: Firefox terminates immediately after visiting this page → crash [@ QBCurve::SubDivide ][@ QBCurve::MidPointDivide ]
Whiteboard: talkback

Comment 15

13 years ago
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">
This is basically the same as bug 27650 and bug 243671, no?
Blocks: 27650, 243671

Comment 17

13 years ago
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.
This now WFM on Firefox 1.0.2 pl-PL on Linux. Can anyone else confirm it's WFM?

Comment 19

13 years ago
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

13 years ago
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
(Reporter)

Comment 21

13 years ago
Talkback:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB5212695E
Summary: crash [@ QBCurve::SubDivide ][@ QBCurve::MidPointDivide ] → crash [@ QBCurve::SubDivide ][@ QBCurve::MidPointDivide ][@ nsCSSRendering::DrawTableBorderSegment]
Assignee: dbaron → roc
Component: Style System (CSS) → Layout: View Rendering

Comment 22

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

Comment 23

12 years ago
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

12 years ago
(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

12 years ago
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

12 years ago
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...
Flags: blocking1.8rc1?
(Assignee)

Comment 27

12 years ago
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

12 years ago
(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).


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

Comment 30

12 years ago
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.

Updated

12 years ago
Flags: blocking1.8rc1? → blocking1.8rc1-
(Assignee)

Comment 31

12 years ago
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,
Attachment #201239 - Flags: superreview?(roc)
Attachment #201239 - Flags: review?(roc)
Attachment #201239 - Flags: superreview?(roc)
Attachment #201239 - Flags: superreview+
Attachment #201239 - Flags: review?(roc)
Attachment #201239 - Flags: review+
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.
(Assignee)

Comment 33

12 years ago
*** Bug 291204 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 34

12 years ago
fixed on trunk
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 35

12 years ago
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.
Attachment #201239 - Flags: approval1.8rc1?
(Reporter)

Comment 36

12 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 37

12 years ago
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?
(Reporter)

Comment 38

12 years ago
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

12 years ago
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.
(Assignee)

Comment 40

12 years ago
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
Attachment #201370 - Flags: superreview?(roc)
Attachment #201370 - Flags: review?(roc)
(Assignee)

Comment 41

12 years ago
Robert as the causing issue is a floating point arithmetic problem, can you say a timescale when that cairo switch will happen. 
(Assignee)

Comment 42

12 years ago
Created attachment 201376 [details] [diff] [review]
same as above only with quality control of the approximation

this terminates the stack much earlier
(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.
Attachment #201370 - Flags: superreview?(roc)
Attachment #201370 - Flags: superreview+
Attachment #201370 - Flags: review?(roc)
Attachment #201370 - Flags: review+
(Reporter)

Comment 44

12 years ago
All testcases in this bug are fixed for me. Good work!
(Assignee)

Updated

12 years ago
Assignee: roc → bernd_mozilla
Status: REOPENED → NEW
(Assignee)

Comment 45

12 years ago
fix checked in
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 46

12 years ago
Comment on attachment 201239 [details] [diff] [review]
patch

I will make a new patch against branch for post 1.5rc1
Attachment #201239 - Flags: approval1.8rc1?
(Assignee)

Comment 47

12 years ago
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.
Attachment #201541 - Flags: approval1.8rc2?
(Reporter)

Updated

12 years ago
Status: RESOLVED → VERIFIED

Comment 48

12 years ago
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.
Attachment #201541 - Flags: approval1.8rc2? → approval1.8rc2-
(Assignee)

Comment 49

12 years ago
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

12 years ago
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.)
(Assignee)

Comment 51

12 years ago
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

12 years ago
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.
Attachment #201541 - Flags: approval1.8rc2- → approval1.8rc2?

Comment 53

12 years ago
DBaron - can you comment on risk of taking this at this point in the cycle?
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.

Updated

12 years ago
Attachment #201541 - Flags: approval1.8rc2? → approval1.8rc2+
(Assignee)

Comment 55

12 years ago
fixed on branch
Keywords: fixed1.8

Updated

12 years ago
Whiteboard: [sg:critical]

Updated

12 years ago
Flags: blocking1.8rc1- → blocking-aviary1.0.8?

Comment 56

12 years ago
no crash in firefox 1.5 rc winxp/linux
Keywords: fixed1.8 → verified1.8

Updated

12 years ago
Depends on: 316821

Updated

12 years ago
Blocks: 316894
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+
Comment on attachment 201541 [details] [diff] [review]
patch against branch

a=dveditz
Attachment #201541 - Flags: approval1.7.13+
Attachment #201541 - Flags: approval-aviary1.0.8+
Keywords: fixed-aviary1.0.8, fixed1.7.13
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.
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8
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.
Keywords: fixed1.7.13 → verified1.7.13

Comment 60

9 years ago
changeset:   25010:b0337b6287f3
user:        Jesse Ruderman <jruderman@gmail.com>
date:        Fri Feb 13 14:54:17 2009 -0800
summary:     Add crashtests
Flags: in-testsuite+
Crash Signature: [@ QBCurve::SubDivide ] [@ QBCurve::MidPointDivide ] [@ nsCSSRendering::DrawTableBorderSegment]
You need to log in before you can comment on or make changes to this bug.