372046 - Crash [@ nsSVGPathSegList::RemoveFromCurrentList] with insertItemBefore({}, 0)

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Based on the stack trace I get from a debug build of Firefox, this is [sg:critical].

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0x812e2bc1

Thread 0 Crashed:
0   <<00000000>> 	0x28028001 0 + 671252481
1   libgklayout.dylib        	0x18b6a1ad nsCOMPtr<nsISVGValue>::assign_from_helper(nsCOMPtr_helper const&, nsID const&) + 25 (nsSVGPathSeg.cpp:1304)
2   libgklayout.dylib        	0x18b6a2cd nsCOMPtr<nsISVGValue>::nsCOMPtr[in-charge](nsCOMPtr_helper const&) + 49 (nsSVGPathSeg.cpp:695)
3   libgklayout.dylib        	0x189107f5 nsSVGPathSegList::RemoveFromCurrentList(nsIDOMSVGPathSeg*) + 51 (nsSVGPathSegList.cpp:382)
4   libgklayout.dylib        	0x189108b4 nsSVGPathSegList::InsertElementAt(nsIDOMSVGPathSeg*, int) + 46 (nsSVGPathSegList.cpp:368)
5   libgklayout.dylib        	0x189109fc nsSVGPathSegList::InsertItemBefore(nsIDOMSVGPathSeg*, unsigned, nsIDOMSVGPathSeg**) + 96 (nsSVGPathSegList.cpp:240)
6   libxpcom_core.dylib      	0x0135af70 NS_InvokeByIndex + 98 (xptcinvoke_unixish_x86.cpp:179)
7   libxpconnect.dylib       	0x13046af7 XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) + 4795 (xpcwrappednative.cpp:2251)
8   libxpconnect.dylib       	0x1304e4d6 XPC_WN_CallMethod(JSContext*, JSObject*, unsigned, long*, long*) + 398 (xpcwrappednativejsops.cpp:1464)
9   libmozjs.dylib           	0x0105ceef js_Invoke + 3284 (jsinterp.c:1348)
...

Comment 1

[Jesse Ruderman]

Smaug, want to fix this?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposed patch

We must ensure that only native SVGPathSegs are used.

Comment 3

[Jesse Ruderman]

Can JavaScript objects override QueryInterface in evil ways?  I'm worried about NS_ENSURE_NATIVE_PATH_SEG and the way it's used.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Well, this is similar to bug 326778.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

And I tried with { QueryInterface: function (aIID) { dump("IID " + aIID);}}. That QI never got called.

Comment 6

[tor]

Ugh, didn't know that JS allowed you to create objects that had the right DOM interface but weren't native.  If that's the case then all the SVG*List objects will probably need similar fixes, and likely there's other SVG code that makes this assumption.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

nsSVGPathSeg* implementations inherit nsSVGPathSeg so the patch
fixes those too.
nsSVGLengthList looks to be safe.
nsSVGPointList should be safe too, though if someone uses
JS DOMSVGPoints, it may do some strange things if JS version of the
method do something unexpected.

nsSVGTransformList looks dangerous.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Checked in. I'll open a new bug for nsSVGTransformList if I find a
way to make it crash.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

I don't see this on the 1.8 branch (Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.4pre) Gecko/20070322 BonEcho/2.0.0.4pre).

Comment 10

[Daniel Veditz [:dveditz]]

I don't crash either, debug win32 build.

Smaug: why the nomination? is the code broken on the branch and it just takes a different testcase to show it?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Hmm, ok, I can't get this to crash in 1.8. I thought ::initialize would
have been evil, but apparently no. There are no NS_STATIC_CAST done
in 1.8 like there were in trunk.

Comment 12

[Jesse Ruderman]

Crashtest checked in.