Last Comment Bug 686044 - Crash with after path.pathSegList.appendItem and GC
: Crash with after path.pathSegList.appendItem and GC
Status: VERIFIED FIXED
[sg:critical][qa!]
: crash, testcase, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86_64 All
: -- critical (vote)
: mozilla9
Assigned To: Daniel Holbert [:dholbert] (largely AFK until June 28)
:
Mentors:
Depends on: 669584
Blocks: 344905
  Show dependency treegraph
 
Reported: 2011-09-09 16:14 PDT by Jesse Ruderman
Modified: 2012-01-19 11:22 PST (History)
10 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
affected
+
fixed
+
fixed
+
fixed
unaffected
unaffected


Attachments
testcase (crashes Firefox within a few seconds) (474 bytes, image/svg+xml)
2011-09-09 16:14 PDT, Jesse Ruderman
no flags Details
stack trace (42.72 KB, text/plain)
2011-09-09 16:15 PDT, Jesse Ruderman
no flags Details
fix (1.16 KB, patch)
2011-09-09 18:55 PDT, Daniel Holbert [:dholbert] (largely AFK until June 28)
jwatt: review+
Details | Diff | Review
fix w/ added comment [r=jwatt] (1.89 KB, patch)
2011-09-15 11:45 PDT, Daniel Holbert [:dholbert] (largely AFK until June 28)
bugzilla: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta-
Details | Diff | Review

Description Jesse Ruderman 2011-09-09 16:14:33 PDT
Created attachment 559609 [details]
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 2011-09-09 16:15:12 PDT
Created attachment 559610 [details]
stack trace
Comment 2 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-09-09 17:01:45 PDT
(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 2011-09-09 17:09:26 PDT
https://www.squarefree.com/extensions/domFuzzLite2.xpi, yeah.
Comment 4 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-09-09 17:30:11 PDT
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] (largely AFK until June 28) 2011-09-09 17:55:19 PDT
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] (largely AFK until June 28) 2011-09-09 17:56:52 PDT
(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] (largely AFK until June 28) 2011-09-09 18:13:18 PDT
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] (largely AFK until June 28) 2011-09-09 18:55:52 PDT
Created attachment 559646 [details] [diff] [review]
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] (largely AFK until June 28) 2011-09-09 19:07:23 PDT
(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] (Away Jun. 27 - Jul. 13) 2011-09-15 11:16:47 PDT
Comment on attachment 559646 [details] [diff] [review]
fix

r=jwatt with f2f comments addressed (see upcoming commit from dholbert).
Comment 11 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-09-15 11:45:05 PDT
Created attachment 560424 [details] [diff] [review]
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] (largely AFK until June 28) 2011-09-15 15:45:46 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6806beccdeaf
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-16 07:26:56 PDT
https://hg.mozilla.org/mozilla-central/rev/6806beccdeaf
Comment 14 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-09-17 09:11:35 PDT
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 christian 2011-09-19 14:23:56 PDT
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] (Away Jun. 27 - Jul. 13) 2011-09-23 04:49:36 PDT
Pushed https://hg.mozilla.org/releases/mozilla-aurora/rev/3d09765cc32f
Comment 17 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-09-23 04:55:44 PDT
(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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 11:18:26 PDT
qa+ for verification in Firefox 8 and 9 using the attached testcase.
Comment 19 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-10-26 16:24:49 PDT
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] (largely AFK until June 28) 2011-10-26 17:12:04 PDT
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 [:mwargers] (not working for Mozilla) 2011-12-05 08:53:14 PST
Verified fixed in 11.0a1 (2011-12-01).
Comment 22 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-12-05 08:56:24 PST
Verified fixed in Mozilla/5.0 (Windows NT 6.1; rv:9.0) Gecko/20100101 Firefox/9.0
Comment 23 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-12-05 09:01:09 PST
Verified fixed in Mozilla/5.0 (Windows NT 6.1; rv:10.0a2) Gecko/20111205 Firefox/10.0a2
Comment 24 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-12-05 09:04:46 PST
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 2011-12-05 09:19:05 PST
Martijn, how do you recommend forcing GC from a testcase?

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