Crash with after path.pathSegList.appendItem and GC

VERIFIED FIXED in Firefox 8

Status

()

Core
SVG
--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: dholbert)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla9
x86_64
All
crash, testcase, verified-aurora, verified-beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox7- affected, firefox8+ fixed, firefox9+ fixed, firefox10+ fixed, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical][qa!], crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

6 years ago
Created attachment 559610 [details]
stack trace
(Assignee)

Comment 2

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

Comment 3

6 years ago
https://www.squarefree.com/extensions/domFuzzLite2.xpi, yeah.
(Assignee)

Comment 4

6 years ago
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).
OS: Mac OS X → All
(Assignee)

Comment 5

6 years ago
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.)
(Assignee)

Comment 6

6 years ago
(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.)
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
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).
Attachment #559646 - Flags: review?(jwatt)
(Assignee)

Comment 9

6 years ago
(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.)
Assignee: nobody → dholbert
Depends on: 669584
Whiteboard: [sg:critical]
Comment on attachment 559646 [details] [diff] [review]
fix

r=jwatt with f2f comments addressed (see upcoming commit from dholbert).
Attachment #559646 - Flags: review?(jwatt) → review+
(Assignee)

Comment 11

6 years ago
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. :))
Attachment #559646 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6806beccdeaf
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla9
(Assignee)

Updated

6 years ago
Whiteboard: [sg:critical] → [sg:critical][inbound]
https://hg.mozilla.org/mozilla-central/rev/6806beccdeaf
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical][inbound] → [sg:critical]
(Assignee)

Comment 14

6 years ago
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. :))
Attachment #560424 - Flags: approval-mozilla-beta?
Attachment #560424 - Flags: approval-mozilla-aurora?

Comment 15

6 years ago
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
status-firefox7: --- → wontfix

Updated

6 years ago
Attachment #560424 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #560424 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed https://hg.mozilla.org/releases/mozilla-aurora/rev/3d09765cc32f

Updated

6 years ago
Attachment #560424 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
(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.
status-firefox7: wontfix → affected

Updated

6 years ago
status-firefox8: --- → fixed
status-firefox9: --- → fixed
tracking-firefox7: --- → ?

Updated

6 years ago
Attachment #560424 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Updated

6 years ago
tracking-firefox7: ? → -
qa+ for verification in Firefox 8 and 9 using the attached testcase.
Whiteboard: [sg:critical] → [sg:critical][qa+]

Updated

6 years ago
status-firefox10: --- → fixed
tracking-firefox10: --- → +
tracking-firefox8: --- → +
tracking-firefox9: --- → +
(Assignee)

Comment 19

6 years ago
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.
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
(Assignee)

Comment 20

6 years ago
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
Verified fixed in 11.0a1 (2011-12-01).
Status: RESOLVED → VERIFIED
Verified fixed in Mozilla/5.0 (Windows NT 6.1; rv:9.0) Gecko/20100101 Firefox/9.0
Keywords: verified-beta
Verified fixed in Mozilla/5.0 (Windows NT 6.1; rv:10.0a2) Gecko/20111205 Firefox/10.0a2
Keywords: verified-aurora

Updated

6 years ago
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Fwiw, it's better to have testcase to not rely on extensions. It makes testing in older builds, across version, etc, easier.
(Reporter)

Comment 25

6 years ago
Martijn, how do you recommend forcing GC from a testcase?
Group: core-security
You need to log in before you can comment on or make changes to this bug.