686044 - Crash with after path.pathSegList.appendItem and GC

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox within a few seconds)

Security-sensitive becase:
* It's scary that GC is involved,
* the reduced testcase mysteriously requires a loop,
* I saw an (unsafe) "invalid array index" once.

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Jesse Ruderman from comment #0)
> * It's scary that GC is involved,

(BTW: presumably this requires https://www.squarefree.com/extensions/domFuzzLite.xpi to reliably trigger the crash, given the fuzzPriv.GC() call)

Comment 3

[Jesse Ruderman]

https://www.squarefree.com/extensions/domFuzzLite2.xpi, yeah.

Comment 4

[Daniel Holbert [:dholbert]]

I can repro on Linux64 with the XPI from comment 3. (Sorry for the outdated link in comment 2.)

So far, it always crashes for me when i hits 39, in the testcase's loop. (i.e. it crashes after "39" has been printed to my terminal).

Comment 5

[Daniel Holbert [:dholbert]]

So, in this chunk of code (with aValue == "M4,4")...
> 51 SVGAnimatedPathSegList::SetBaseValueString(const nsAString& aValue)
> 52 {
[...]
> 67   DOMSVGPathSegList *baseValWrapper =
> 68     DOMSVGPathSegList::GetDOMWrapperIfExists(GetBaseValKey());
> 69   if (baseValWrapper) {
> 70     baseValWrapper->InternalListWillChangeTo(newBaseValue);
> 71   }
https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGAnimatedPathSegList.cpp#67

What's happening is:
 - We do have an existing baseValWrapper.
 - We hit line 70, the call to InternalListWillChangeTo.
 - This removes the last reference to |baseValWrapper|
 - In so doing, we call RemovingFromList() on its only entry, and remove the last reference to it.  (see code below)
 - So |baseValWrapper| dies while we're still in the middle of a method running on it.

We actually have code for a kung-fu death-grip in place here, but we're not activating the death-grip because our new list's length (3) is larger than our old length (1), so we think it's unnecessary:
> 158   nsRefPtr<DOMSVGPathSegList> kungFuDeathGrip;
> 159   if (aNewValue.Length() < length) {
> 160     // RemovingFromList() might clear last reference to |this|.
> 161     // Retain a temporary reference to keep from dying before returning.
> 162     kungFuDeathGrip = this;
> 163   }
> 164 
> 165   while (index < length && dataIndex < dataLength) {
> 166     newSegType = SVGPathSegUtils::DecodeType(aNewValue.mData[dataIndex]);
> 167     if (ItemAt(index) && ItemAt(index)->Type() != newSegType) {
> 168       ItemAt(index)->RemovingFromList();

https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/DOMSVGPathSegList.cpp#158

I don't immediately remember why we think the death-grip is unnecessary in that case, though... Clearly it would help to prevent the crash in this testcase.  (maybe that's because something else is failing too, though.)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] (vacation 9/17-9/24) from comment #5)
> What's happening is:
>  - We do have an existing baseValWrapper.
>  - We hit line 70, the call to InternalListWillChangeTo.
>  - This removes the last reference to |baseValWrapper|
>  - In so doing, we call RemovingFromList() on its only entry, and remove the
> last reference to it.  (see code below)
>  - So |baseValWrapper| dies while we're still in the middle of a method
> running on it.

(Sorry, ignore the 3rd bullet-point there ("This removes the last...") -- I clarified what I meant by it in the 4th bullet point, and then forgot to delete the original un-clarified version.)

Comment 7

[Daniel Holbert [:dholbert]]

As noted in bug 669584 comment #0:
> The fix for bug 639728 forgot that items are created lazily. Checking that
> the list length is going to change to zero is not enough. The last remaining
> reference to the object may be a single item at an arbitrary index
[...NOTE: that's true here -- and the arbitrary index happens to be index 0...]
> so really we have to check whether the list length is decreasing.

In this case, there's the additional complication that we **drop** the old DOMSVGPathSeg if the segment type has changed, per line 167 quoted in comment 5.  (We don't do that for other list-types, because they don't have the "different segment types have different data lengths" complication that paths have.)

So I suspect that for DOMSVGPathSegList, we want to *always* use a death-grip, regardless of whether the list-length is decreasing or increasing.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix

(In reply to Daniel Holbert [:dholbert] (vacation 9/17-9/24) from comment #7)
> So I suspect that for DOMSVGPathSegList, we want to *always* use a
> death-grip, regardless of whether the list-length is decreasing or
> increasing.

Actually not quite "always" -- we only want the death-grip if we've got a nonzero length.  (If we deathgrip unconditionally, we delete ourselves while we're still inside in our constructor -- because we hit this code & add/remove the deathgrip & delete ourselves, all before the code that created us has gotten a chance to |NS_ADDREF| the newly-constructed object)

So -- this patch adds a deathgrip if(length).

Comment 9

[Daniel Holbert [:dholbert]]

(Marking "depends-on" bug 669584.  Absolutely not a regression from that bug, but still related, as the patch here further-relaxes an "if" check that was partially-relaxed in that bug.)

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 559646 [details] [diff] [review]
fix

r=jwatt with f2f comments addressed (see upcoming commit from dholbert).

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: fix w/ added comment [r=jwatt]

Added explanatory comment per jwatt's request.

I'll land this later today.  (It's currently going through TryServer with another patch I want to land, just as an absolute sanity-check since I can't treewatch-after-landing effectively during the F2F. :))

Comment 12

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6806beccdeaf

Comment 13

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/6806beccdeaf

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 560424 [details] [diff] [review]
fix w/ added comment [r=jwatt]

Requesting approval to land on Beta & Aurora channels.

Risk Assessment:
 - Targeted fix (one-liner, ignoring comments)
 - Basically as low-risk as it gets. As noted in bug 639728 comment 20 (which added the original code that this bug extends), this has zero expected behavior-change, aside from fixing crashes.  It just adds a kungFuDeathGrip in one (non-recursive) function, to keep something alive until the function returns.

Reward:
 - Fixes an sg:crit, which is probably unlikely to be triggered by "real" web content, but which bad guys could be poking at now that this has landed on trunk.

I know release-train-bump-day is coming up in just over a week -- if we haven't already spun up our final Firefox 7 builds (or if we have, but we end up respinning them), it would be great to get this in.

(Minor logistical complication: I'm about to leave on a camping trip in an off-the-grid backwoodsy area, so I won't be able to land this on the branches myself, once approval is granted.  I'm hoping jwatt or someone else can take care of that for me. :))

Comment 15

[LegNeato]

Unless there is a reason why we would expect this to discovered externally we will wait as we are late in the game for Firefox 7 on beta. Denying approval for beta

Comment 16

[Jonathan Watt [:jwatt]]

Pushed https://hg.mozilla.org/releases/mozilla-aurora/rev/3d09765cc32f

Comment 17

[Jonathan Watt [:jwatt]]

(In reply to Christian Legnitto [:LegNeato] from comment #15)
> Unless there is a reason why we would expect this to discovered externally
> we will wait as we are late in the game for Firefox 7 on beta. Denying
> approval for beta

It seems to me that the push to aurora makes this very discoverable externally. We're pretty sure that malicious people are monitoring pushes containing bug numbers that can't be accessed (a good indication of a security fix), and reverse engineering exploits from those pushes. Hence renominating for beta. As Daniel said, this should be zero risk.

Comment 18

[u279076]

qa+ for verification in Firefox 8 and 9 using the attached testcase.

Comment 19

[Daniel Holbert [:dholbert]]

Note: as with related bug 669584 & bug 639728, this shouldn't affect 3.6.x.  These are all bugs in SVG list code that was completely rewritten for Firefox 4.

Hence, setting status-1.9.* = unaffected.

Comment 20

[Daniel Holbert [:dholbert]]

FWIW, I tried to test this in Firefox 3.6.x to be absoulutely-positively sure of Comment 19, but I can't, because dom fuzz lite doesn't support 3.6.x.  (However, Jesse did give me a custom dom fuzz lite to try[1], with a different GC command[2], and I didn't hit the bug with that.)

From reading through the analogous 3.6.x code[3], it looks like our internal SVG list objects are the same as the heavyweight DOM objects -- whereas in Firefox >4, we create tearoffs when necessary for DOM objects.   Without the on-the-fly tearoffs being created, there's no chance of this bug happening.

So, I'm confident that 1.9.*=unaffected is correct.

[1] https://www.squarefree.com/extensions/domFuzzLite-for-firefox-3.6.xpi
[2] var evt = document.createEvent("Events"); evt.initEvent("please-gc", true, false); document.dispatchEvent(evt);
[3] http://mxr.mozilla.org/firefox/source/content/svg/content/src/nsSVGPathSegList.cpp

Comment 21

[Martijn Wargers (dead)]

Verified fixed in 11.0a1 (2011-12-01).

Comment 22

[Martijn Wargers (dead)]

Verified fixed in Mozilla/5.0 (Windows NT 6.1; rv:9.0) Gecko/20100101 Firefox/9.0

Comment 23

[Martijn Wargers (dead)]

Verified fixed in Mozilla/5.0 (Windows NT 6.1; rv:10.0a2) Gecko/20111205 Firefox/10.0a2

Comment 24

[Martijn Wargers (dead)]

Fwiw, it's better to have testcase to not rely on extensions. It makes testing in older builds, across version, etc, easier.

Comment 25

[Jesse Ruderman]

Martijn, how do you recommend forcing GC from a testcase?