Last Comment Bug 435209 - Crash [@ nsSVGPathSeg::SetCurrentList] with pathSegList, appendItem and replaceItem
: Crash [@ nsSVGPathSeg::SetCurrentList] with pathSegList, appendItem and repla...
Status: VERIFIED FIXED
: [sg:critical?] post 1.8-branch
: crash, regression, testcase, verified1.9.0.7, verified1.9.1
Product: Core
Classification: Components
Component: SVG
: Trunk
: x86 Windows XP
: P1 critical (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Mozilla Corporation)
: General SVG Bugs
:
:
: 363839
  Show dependency treegraph
 
Reported: 2008-05-22 08:04 PDT by Martijn Wargers [:mw22] (QA - IRC nick: mw22)
Modified: 2009-04-24 10:21 PDT (History)
12 users (show)
roc: blocking1.9.1+
roc: wanted1.9.1+
dveditz: blocking1.9.0.7+
dveditz: wanted1.8.1.x-
bclary: in‑testsuite+
See Also:
Crash Signature:
[@ nsSVGPathSeg::SetCurrentList]


Attachments
testcase (285 bytes, image/svg+xml)
2008-05-22 08:04 PDT, Martijn Wargers [:mw22] (QA - IRC nick: mw22)
no flags Details
Patch that seems to fix the problem (no crash, seglist has 1 item) (1.37 KB, patch)
2008-05-22 10:41 PDT, Jeff Schiller
no flags Details | Diff | Splinter Review
Another edge case for replaceItem() (1.00 KB, image/svg+xml)
2008-05-27 19:01 PDT, Jeff Schiller
no flags Details
Test case that proves that the crash does not happen with SVGNumberList, SVGTransformList, SVGPointList or SVGLengthList. (4.05 KB, image/svg+xml)
2008-05-27 21:09 PDT, Jeff Schiller
no flags Details
Fix crash - aligns with Batik implementation (2.19 KB, patch)
2008-05-27 21:21 PDT, Jeff Schiller
longsonr: review+
roc: superreview+
mbeltzner: approval1.9.1+
dveditz: approval1.9.0.7+
Details | Diff | Splinter Review

Summon comment box

Description Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2008-05-22 08:04:20 PDT
Created attachment 322105 [details]
testcase

See testcase, which crashes current trunk build.

This regressed between 2007-01-17 and 2007-01-18:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-01-17+04&maxdate=2007-01-18+09&cvsroot=%2Fcvsroot
Before that date, I get not implemented errors.
So I think this is a regression from bug 363839, somehow.

http://crash-stats.mozilla.com/report/index/30701cb5-280a-11dd-a593-0013211cbf8a?p=1
0  	xul.dll  	nsCOMPtr<nsIScriptContext>::operator=  	 nsCOMPtr.h:713
1 	xul.dll 	nsSVGPathSeg::SetCurrentList 	mozilla/content/svg/content/src/nsSVGPathSeg.cpp:151
2 	xul.dll 	nsSVGPathSegList::RemoveElementAt 	mozilla/content/svg/content/src/nsSVGPathSegList.cpp:344
3 	xul.dll 	nsSVGPathSegList::ReplaceItem 	mozilla/content/svg/content/src/nsSVGPathSegList.cpp:251
4 	xul.dll 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
5 	xul.dll 	XPCWrappedNative::CallMethod 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2388
Comment 1 Jeff Schiller 2008-05-22 09:49:09 PDT
This is specifically in the nsSVGPathSegList::ReplaceItem() method.  There are at least two cases that are not covered :

1) segList has {x,y}.  replaceItem(x,0) results in a segList with {x}
2) segList has {x}.  replaceItem(x,0) results in a crash

I am pasting in a fix for ReplaceItem() that I _think_ will fix it.  I am currently many months behind on my CVS local copy so building Moz is going to take hours.  If anyone else wants to test this, please feel free:

 /* nsIDOMSVGPathSeg replaceItem (in nsIDOMSVGPathSeg newItem, in unsigned long index); */
 NS_IMETHODIMP nsSVGPathSegList::ReplaceItem(nsIDOMSVGPathSeg *newItem,
                                             PRUint32 index,
                                             nsIDOMSVGPathSeg **_retval)
 {
  NS_ENSURE_NATIVE_PATH_SEG(newItem, _retval);

  if (index >= static_cast<PRUint32>(mSegments.Count())) {
    return NS_ERROR_DOM_INDEX_SIZE_ERR;
  }
  
  // get item that we will replace
  nsIDOMSVGPathSeg* segToReplace = mSegments.ObjectAt(index);
  
  // if the item we are inserting is not the same we are replacing, 
  // then we have actual work to do
  if (newItem != segToReplace) {
    InsertElementAt(static_cast<nsSVGPathSeg*>(newItem), index);
    RemoveFromCurrentList(segToReplace);
    NS_ADDREF(newItem);
  }
  
  *_retval = newItem;

  return NS_OK;
 }
Comment 2 Jeff Schiller 2008-05-22 10:02:16 PDT
Also, is there a problem with insertItemBefore via InsertElementAt() if the index is beyond the length of the current segList?  I'm not sure on how things work.  For instance,

segList has {x,y}.  insertItemBefore(z,5) - by the spec should append the seg to the list resulting in {x,y,z}.  However, the code in InsertElementAt() results in a call to 

   mSegments.InsertObjectAt(aElement, 5);

At this point, mSegments has only two items in it.  Is this a problem for nsCOMArray ?
Comment 3 Jeff Schiller 2008-05-22 10:41:10 PDT
Created attachment 322133 [details] [review]
Patch that seems to fix the problem (no crash, seglist has 1 item)
Comment 4 Robert Longson 2008-05-22 10:48:10 PDT
What about other implementations of replaceItem in svg content e.g. nsSVGNumberList::ReplaceItem are they affected too?
Comment 5 Jeff Schiller 2008-05-22 12:05:02 PDT
Ugh - I wish I would have had time for Bug 282247.  It looks like every list has their own implementation (and some are not even complete like SVGPointList).

It doesn't look like SVGNumberList has this problem.  I did not look at every List implementation yet, sorry.
Comment 6 Jeff Schiller 2008-05-27 19:01:56 PDT
Created attachment 322715 [details]
Another edge case for replaceItem()
Comment 7 Jeff Schiller 2008-05-27 19:15:40 PDT
I'm attaching another test case that exercises an edge case of SVGPathSegList::replaceItem(newItem, index).  In particular, this case covers when newItem is already a part of the list to which it is being added.

It seems the SVG spec is not clear on what 'index' means - are we to replace the item at that position BEFORE newItem is removed from its current list or the item at that position AFTER newItem is removed.  There is a difference when newItem's current position in the list is less than the value of index.

The test case has a seg list with 5 segments, for simplicity: { a, b, c, d, e }.  We then call replaceItem(b,2).  The test case has the following results:

Opera 9.27: { a, b, d, e }
Safari 3.1: { a, b, d, e }
Batik: { a, c, b, e } 
Firefox 2: replaceItem not implemented
Firefox 3 RC1: { a, c, b, e } 

I have a combined patch that fixes the crash and implements things as Opera and Safari - or I can keep our existing behavior.  The key thing is that the spec needs to be clarified, I guess.

Based on this, should I just update the patch to fix the crash and leave our implementation as is (if the spec is clarified, we can fix it under another bug)?  Or do we want to align with Opera and Safari until told otherwise?
Comment 8 Jeff Schiller 2008-05-27 21:09:12 PDT
Created attachment 322723 [details]
Test case that proves that the crash does not happen with SVGNumberList, SVGTransformList, SVGPointList or SVGLengthList.

i.e. the patch is only needed for SVGPathSegList

Note that while we do not pass this test (any of the four sub-tests, in fact) - the browser does not crash.  Neither Safari or Batik pass any of the sub-tests either - only Opera passes.
Comment 9 Jeff Schiller 2008-05-27 21:21:11 PDT
Created attachment 322727 [details] [review]
Fix crash - aligns with Batik implementation
Comment 10 Jeff Schiller 2008-05-27 21:22:48 PDT
Uploaded simple patch to fix crash.  This aligns with Batik implementation for the edge case described above.

Let me know if you want the slightly larger patch instead (that fixes crash and aligns with Opera and WebKit).
Comment 11 Robert Longson 2008-05-29 08:55:24 PDT
Comment on attachment 322727 [details] [review]
Fix crash - aligns with Batik implementation

>+  // NOTE: the new item can never be the item we will be replacing now that we removed it from its current list beforehand

Nit: Can you fold this to stay within 80 characters please.

General points:

We should ask the svg list what should happen if you do replaceItem(b,3000) where the list contains b but does not have 3000 items in it. Should b disappear from the existing list or not? What do the other implementations do?

I don't think this can make the cut for rc2 which is tomorrow as far as I know so perhaps we should give the svg wg a little more time to respond.

Without response I think that the algorithm you have gone for is reasonable although if Batik does not remove b from the list when confronted with replaceItem(b,3000) then our implementation should not do so either.
Comment 12 Jeff Schiller 2008-05-29 09:02:52 PDT
Robert - I've asked Erik Dahlstrom of Opera and the SVG WG on this.  I haven't got an official answer yet from the SVG WG but Erik's personal opinion was yes, b should be removed from the list and then the exception raised.

I believe Batik has the same behavior but I will confirm.

I'll also address the nit later tonight.
Comment 13 Jeff Schiller 2008-09-08 09:14:02 PDT
Sorry - I do not have bandwidth to work on this in the short term.  Hopefully someone can pick up where I left off.
Comment 14 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-11-16 17:38:42 PST
Not going to block 1.9.1 but definitely wanted. It would be good to have mochitest coverage for these APIs.
Comment 15 Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2009-01-06 14:27:45 PST
Still crashes in current trunk build.
Comment 16 Daniel Veditz 2009-01-08 10:41:09 PST
At the very least it appears to be dereferencing an uninitialized nsSVGPathSeg, and if the index can reach into page-controlled memory this could easily be exploitable.
Comment 17 Jeff Schiller 2009-01-08 10:59:29 PST
I don't have time to work more on this, but the patch should prevent the crashes.  The SVG WG had some action [1] to provide an answer and I'm not sure if they ever officially did [2].

[1] http://www.w3.org/2008/06/19-svg-minutes.html#item07
[2] http://lists.w3.org/Archives/Public/www-svg/2008Aug/0049.html
Comment 18 Robert Longson 2009-01-08 15:18:11 PST
Comment on attachment 322727 [details] [review]
Fix crash - aligns with Batik implementation

Lets just align ourselves with Batik for now then.
Comment 19 Robert Longson 2009-01-08 15:18:51 PST
Apologies for the delay.
Comment 20 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-01-08 16:26:09 PST
We could use a crashtest here.
Comment 21 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-01-08 16:43:27 PST
Pushed 6b858b6ab162
Comment 22 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-01-08 16:43:52 PST
Comment on attachment 322727 [details] [review]
Fix crash - aligns with Batik implementation

Simple patch, fixes crash
Comment 23 Mike Beltzner [:beltzner] 2009-01-13 18:28:22 PST
Comment on attachment 322727 [details] [review]
Fix crash - aligns with Batik implementation

a191=beltzner; please do get a crashtest added as per roc's request
Comment 24 Daniel Veditz 2009-01-23 11:26:24 PST
This should be P1 to make sure it lands on 191 in time to make 1907
Comment 25 chris hofmann 2009-01-23 15:05:36 PST
also thowing it on the blocking list, in addition to the wanted list, so it has less chance of  falling off the radar.
Comment 26 Robert Longson 2009-01-25 06:59:25 PST
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3749974eee0e
Comment 27 Samuel Sidler (old account; do not CC) 2009-01-26 11:26:13 PST
Please don't land a crash test before we ship this in a 1.9.0.x release.
Comment 28 Daniel Veditz 2009-01-28 15:42:17 PST
Comment on attachment 322727 [details] [review]
Fix crash - aligns with Batik implementation

Approved for 1.9.0.7, a=dveditz for release-drivers.
Comment 29 Tony Chung [:tchung] 2009-01-30 17:52:47 PST
(In reply to comment #8)
> Created an attachment (id=322723) [details]
> Test case that proves that the crash does not happen with SVGNumberList,
> SVGTransformList, SVGPointList or SVGLengthList.
> 
> i.e. the patch is only needed for SVGPathSegList
> 
> Note that while we do not pass this test (any of the four sub-tests, in fact) -
> the browser does not crash.  Neither Safari or Batik pass any of the sub-tests
> either - only Opera passes.

Noted testcase does not turn green as expected.  Yet the crash in both testcases attached are truly gone.

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre Ubiquity/0.1.5 and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090130 Minefield/3.2a1pre
Comment 30 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-02-03 16:51:41 PST
Checked into 1.9.0 branch.
Comment 31 Al Billings [:abillings] 2009-02-12 15:55:11 PST
The second test case does not give a green circle. As with 3.1 now, the first case does not crash the 1.9.0.7 nightly though. I'm marking this as verified as the issue with the second test case seems to be an open question.

Verified for 1.9.0.7 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009021204 GranParadiso/3.0.7pre.
Comment 32 Bob Clary [:bc:] 2009-04-24 10:21:56 PDT
crash test added
http://hg.mozilla.org/mozilla-central/rev/4f5ce5b9d525

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