Created attachment 567424 [details]
Input to trigger the crash.
The attached SVG file causes a nasty looking crash at least in Firefox 8.0. I haven't tested with other versions yet. The original file crashed at 0x4, but the address moved while looking for a simpler triggering file and ended up at 0xff000004. High crash addresses often imply security impact, so reporting this as a security bug to be on the safe side.
Crash report:
7.0.1 crashed at NS_NewAtom, address 0x1800004 -
Local debug build I'm getting a crash at:
> xul.dll!nsGenericElement::GetAttributes(aAttributes=0x0fd29d78) Line 2510 C++
`this` == 0xffffffc8
xul.dll!mozilla::SVGMotionSMILAnimationFunction::RebuildPathAndVerticesFromMpathElem(aMpathElem=0x11dea100) Line 251 C++
xul.dll!mozilla::SVGMotionSMILAnimationFunction::RebuildPathAndVertices(aTargetElement=0x0988de30) Line 309 C++
And the bug is very likely to be in this code:
which also needs a namespace check
Created attachment 567469 [details] [diff] [review]
Check node type, not just name, rev. 1
Setting tracking flags, to be sure this makes it to stable branches. (There's an aurora/beta channel/triage meeting this Thursday -- hopefully we can get approval and land this on branches on that day or the next)
Comment on attachment 567469 [details] [diff] ()).
i.e. just add something like this
genericTarget->GetNameSpaceID() == kNameSpaceID_SVG &&
either before or after the Tag() check.
IsSVG() includes some stuff that's not in the SVG namespace (I'm actually not entirely sure what, offhand), and it's probably better to be more specific. It's also more consistent with the related function "GetFirstMpathChild"
which checks the namespace explicitly.
(In reply to Daniel Holbert [:dholbert] from comment #6)
> Comment on attachment 567469 [details] [diff] [review] [diff] [details] ()).
Agreed. Other code elsewhere has namespace checks so we should be consistent here.
Created attachment 567512 [details] [diff] [review]
Check namespace, not just name, rev. 2
This is the patch without the tests for immediate committal: the test will be embargoed until this is released/made public.
Created attachment 567515 [details] [diff] [review]
Automated test
bsmedberg, is this fix ready for review? If so, please request... Also, tracking for 8 and 9, we have a patch and it's a critical bug with a trivial fix, might as well take it.
Comment on attachment 567512 [details] [diff] [review]
Check namespace, not just name, rev. 2
It's got implicit r+ (I r+'d the obsolete earlier version, and then requested this tweak).
explicitly marking r+ for clarity.
FTR, landed earlier today ;-)
Comment on attachment 567515 [details] [diff] [review]
Automated test
Just for good measure, r+ on the crashtest, too, though as bsmedberg said in comment 8, it should wait until the fix has made it to releases.
I tested it locally (with directory-targeted "make crashtests") and successfully crashed, in an unpatched build.
Excellent! Sorry I missed the earlier review on the obsoleted patch. Please request approval for the relevant branches as soon as this is looking good on m-c.
Landed on m-c:
We'll need a backport for 3.6 as well if this affects that branch.
Comment on attachment 567512 [details] [diff] [review]
Check namespace, not just name, rev. 2
---------------------------------[ Triage Comment ]---------------------------------
Approving for 8beta and 9aurora as this is a sg:crit bug filed by an external reporter and the patch looks safe.
Please land ASAP.
(In reply to Christian Legnitto [:LegNeato] from comment #16)
> We'll need a backport for 3.6 as well if this affects that branch.
Happily, this doesn't affect that branch. The feature being exploited here (SVG SMIL, animateMotion support in particular) was new in Firefox 4.
Verified fixed using the attached testcase in Firefox 8.0.1, 9.0b3, and 10.0a2