If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Crash [@ nsSVGPathSeg::SetCurrentList] with pathSegList, appendItem and replaceItem

VERIFIED FIXED

Status

()

Core
SVG
P1
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: roc)

Tracking

(5 keywords)

Trunk
x86
Windows XP
crash, regression, testcase, verified1.9.0.7, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
wanted1.9.1 +
blocking1.9.0.7 +
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] post 1.8-branch, crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

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

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

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

10 years ago
Created attachment 322133 [details] [diff] [review]
Patch that seems to fix the problem (no crash, seglist has 1 item)
Assignee: nobody → codedread
Status: NEW → ASSIGNED
Attachment #322133 - Flags: review?(tor)

Comment 4

10 years ago
What about other implementations of replaceItem in svg content e.g. nsSVGNumberList::ReplaceItem are they affected too?

Comment 5

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

10 years ago
Created attachment 322715 [details]
Another edge case for replaceItem()

Comment 7

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

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

10 years ago
Created attachment 322727 [details] [diff] [review]
Fix crash - aligns with Batik implementation
Attachment #322133 - Attachment is obsolete: true
Attachment #322133 - Flags: review?(tor)

Comment 10

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

Updated

10 years ago
Attachment #322727 - Flags: review?(longsonr)

Comment 11

10 years ago
Comment on attachment 322727 [details] [diff] [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

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

Updated

9 years ago
Flags: blocking1.9.1?

Comment 13

9 years ago
Sorry - I do not have bandwidth to work on this in the short term.  Hopefully someone can pick up where I left off.
Assignee: codedread → nobody
Not going to block 1.9.1 but definitely wanted. It would be good to have mochitest coverage for these APIs.
Flags: blocking1.9.1? → wanted1.9.1+
(Reporter)

Comment 15

9 years ago
Still crashes in current trunk build.
Flags: blocking1.9.2?
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.
Whiteboard: [sg:critical?]

Comment 17

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

9 years ago
Comment on attachment 322727 [details] [diff] [review]
Fix crash - aligns with Batik implementation

Lets just align ourselves with Batik for now then.
Attachment #322727 - Flags: review?(longsonr) → review+

Comment 19

9 years ago
Apologies for the delay.

Updated

9 years ago
Attachment #322727 - Flags: superreview?(roc)
Attachment #322727 - Flags: superreview?(roc) → superreview+
Keywords: checkin-needed
Whiteboard: [sg:critical?] → [sg:critical?][needs landing]
We could use a crashtest here.
Pushed 6b858b6ab162
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [sg:critical?][needs landing] → [sg:critical?][needs 191 approval]
Comment on attachment 322727 [details] [diff] [review]
Fix crash - aligns with Batik implementation

Simple patch, fixes crash
Attachment #322727 - Flags: approval1.9.1?

Updated

9 years ago
Attachment #322727 - Flags: approval1.9.0.7?
Comment on attachment 322727 [details] [diff] [review]
Fix crash - aligns with Batik implementation

a191=beltzner; please do get a crashtest added as per roc's request
Attachment #322727 - Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [sg:critical?][needs 191 approval] → [sg:critical?][needs 191 landing]
Assignee: nobody → roc
This should be P1 to make sure it lands on 191 in time to make 1907
Priority: -- → P1

Comment 25

9 years ago
also thowing it on the blocking list, in addition to the wanted list, so it has less chance of  falling off the radar.
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+

Comment 26

9 years ago
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3749974eee0e
Keywords: fixed1.9.1
Whiteboard: [sg:critical?][needs 191 landing] → [sg:critical?]

Updated

9 years ago
Flags: blocking1.9.2? → blocking1.9.0.7?
Flags: blocking1.9.0.7? → blocking1.9.0.7+
Please don't land a crash test before we ship this in a 1.9.0.x release.
Attachment #322727 - Flags: approval1.9.0.7? → approval1.9.0.7+
Comment on attachment 322727 [details] [diff] [review]
Fix crash - aligns with Batik implementation

Approved for 1.9.0.7, a=dveditz for release-drivers.
(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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Checked into 1.9.0 branch.
Keywords: fixed1.9.0.7
Flags: wanted1.8.1.x-
Whiteboard: [sg:critical?] → [sg:critical?] post 1.8-branch
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.
Keywords: fixed1.9.0.7 → verified1.9.0.7
Group: core-security

Comment 32

9 years ago
crash test added
http://hg.mozilla.org/mozilla-central/rev/4f5ce5b9d525
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsSVGPathSeg::SetCurrentList]
You need to log in before you can comment on or make changes to this bug.