880544 - "ABORT: Tear-off objects remain in hashtable at shutdown" with watch on SVGPathSegList

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts on shutdown)

###!!! ABORT: Tear-off objects remain in hashtable at shutdown.: 'mTable.Count() == 0', file content/svg/content/src/nsSVGAttrTearoffTable.h, line 28

nsSVGAttrTearoffTable<void, mozilla::DOMSVGPathSegList>::~nsSVGAttrTearoffTable

Security-sensitive because last time I filed a bug involving this assertion (bug 872790), it was marked as a dup of a security-sensitive bug.

Comment 1

[Andrew McCreight [:mccr8]]

Marking sec-critical, because this smells like a use-after-free.  Who should look at this?  Sounds like jwatt is away for all of June.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

David, could you have a look?

Comment 3

[David Zbarsky (:dzbarsky)]

(In reply to :Ms2ger from comment #2)
> David, could you have a look?

I'll add it to my todo list.

Comment 4

[Andrew McCreight [:mccr8]]

Any progress here, David?

Comment 5

[Andrew McCreight [:mccr8]]

Is this something you could look into, Brian?  It looks like you've made the most changes to this file.  Thanks.

Comment 6

[Brian Birtles (:birtles)]

I can have a look but I'm currently travelling so it won't be until the middle of next week at the earliest. If anyone else has time before then, please have a look.

Comment 7

[Brian Birtles (:birtles)]

Can someone please CC me on bug 872774?

Comment 8

[Andrew McCreight [:mccr8]]

done

Comment 9

[Brian Birtles (:birtles)]

Thanks!

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: testcase (asserts on shutdown)

It seems like the use of Object.prototype.watch is keeping a reference to the DOMSVGPathSegList and preventing it from being GC'ed even once the tab has been closed.

If I open this testcase in a tab, close the tab, open about:memory and click GC, CC and Minimize Memory Usage, the DOMSVGPathSegList dtor is still not called. As a result the DOMSVGPathSegList object stays in the hash table, and the assertion fires on shutdown.

Comment 11

[Jonathan Watt [:jwatt]]

Okay, this comes down to the generated DOMProxyHandler::hasOwn implementation for DOMSVGPathSegList (called under obj_watch()):

bool
DOMProxyHandler::hasOwn(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id, bool* bp)
{
  MOZ_ASSERT(!xpc::WrapperFactory::IsXrayWrapper(proxy),
            "Should not have a XrayWrapper here");

  int32_t index = GetArrayIndexFromId(cx, id);
  if (IsArrayIndex(index)) {
    mozilla::DOMSVGPathSegList* self = UnwrapProxy(proxy);
    bool found;
    ErrorResult rv;
    mozilla::DOMSVGPathSeg* result;
    result = self->IndexedGetter(index, found, rv);
    rv.WouldReportJSException();
    if (rv.Failed()) {
      return ThrowMethodFailedWithDetails<true>(cx, rv, "SVGPathSegList", "getItem");
    }
    (void)result;

    *bp = found;
    return true;
  }


  JS::Rooted<JSObject*> expando(cx, GetExpandoObject(proxy));
  if (expando) {
    JSBool b = true;
    JSBool ok = JS_HasPropertyById(cx, expando, id, &b);
    *bp = !!b;
    if (!ok || *bp) {
      return ok;
    }
  }

  *bp = false;
  return true;
}

'result' is a raw pointer, so the DOMSVGPathSeg is simply leaked and, since it holds a strong reference to its DOMSVGPathSegList, that object is leaked too, causing the assertion.

Comment 12

[Jonathan Watt [:jwatt]]

DOMProxyHandler::delete_ would seem to have a similar bug:

bool
DOMProxyHandler::delete_(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id, bool* bp)
{
  MOZ_ASSERT(!xpc::WrapperFactory::IsXrayWrapper(proxy),
            "Should not have a XrayWrapper here");

  int32_t index = GetArrayIndexFromId(cx, id);
  if (IsArrayIndex(index)) {
    mozilla::DOMSVGPathSegList* self = UnwrapProxy(proxy);
    bool found;
    ErrorResult rv;
    mozilla::DOMSVGPathSeg* result;
    result = self->IndexedGetter(index, found, rv);
    rv.WouldReportJSException();
    if (rv.Failed()) {
      return ThrowMethodFailedWithDetails<true>(cx, rv, "SVGPathSegList", "getItem");
    }
    (void)result;
    if (found) {
      *bp = false;
    } else {
      *bp = true;
    }
    // We always return here, even if the property was not found
    return true;
  }


  return dom::DOMProxyHandler::delete_(cx, proxy, id, bp);
}

Comment 13

[Boris Zbarsky [:bzbarsky]]

> 'result' is a raw pointer, so the DOMSVGPathSeg is simply leaked

Uh.... an IndexedGetter is supposed to return a raw pointer.  And looking at DOMSVGPathSegList::IndexedGetter it does:

300     return ItemAt(aIndex);

where ItemAt is:

225     return mItems[aIndex].mItem;

Where is the leak, exactly?

Comment 14

[Boris Zbarsky [:bzbarsky]]

jwatt explained to me on IRC what's going on.  The leak is actually inside IndexedGetter when it does EnsureItemAt: that creates a new object with zero refcount and stores a raw pointer to it.  The assumption is that after that ItemAt() will get called and stored in a refptr, but that never happens.

I suspect the right fix here is to:

1)  Change EnsureItemAt to return an already_AddRefed<SVGPathSeg> so the caller makes
    sure to take ownership, since we depend on that.
2)  Remove the "'resultNotAddRefed': [ 'getItem' ]" from the Bindings.conf entry for
    SVGPathSegList.
3)  Change the signatures of SVGPathSegList::GetItem and SVGPathSegList::IndexedGetter to
    return an already_AddRefed<SVGPathSeg>.

Comment 15

[Andrew McCreight [:mccr8]]

If the object in the hash table is actually alive at shutdown, then it sounds like this isn't actually a security problem.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Oh, and:

4)  Audit/fix in the same way all the other SVG list classes that have
    "'resultNotAddRefed': [ 'getItem' ]" in Bindings.conf (looks like SVGLenthList,
    SVGNumberList, SVGPointList, SVGTransformList, at first glance).

Comment 17

[Jonathan Watt [:jwatt]]

#1-#4 sound fine.

Anyone working on this should also be familiar with the comments at the top of:

https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/DOMSVGAnimatedLengthList.h
https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/DOMSVGLengthList.h
https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/DOMSVGLength.h

Comment 18

[Jonathan Watt [:jwatt]]

(In reply to Andrew McCreight [:mccr8] from comment #15)
> If the object in the hash table is actually alive at shutdown, then it
> sounds like this isn't actually a security problem.

That seems to be the case. At least I see no indication that this is anything other than a memory leak.

Comment 19

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #17)
> Anyone working on this...

In fact, I'll take this.

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: patch for SVGTransformList

For no particular reason I patched SVGTransformList first. I checked that this testcase asserts without the patch.

Passed Try: https://tbpl.mozilla.org/?tree=Try&rev=133191ea54bc

I'll wait until after r+ to upload the patches for the other types since any review comments may apply to them too.

Comment 21

[David Zbarsky (:dzbarsky)]

Comment on attachment 772639 [details] [diff] [review]
patch for SVGTransformList

Review of attachment 772639 [details] [diff] [review]:
-----------------------------------------------------------------

Seems sane.

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: patch for SVGLengthList

Comment 23

[Jonathan Watt [:jwatt]]

Attachment: patch for SVGNumberList

Comment 24

[Jonathan Watt [:jwatt]]

Attachment: patch for SVGPathSegList

Comment 25

[Jonathan Watt [:jwatt]]

Attachment: patch for SVGPointsList

Comment 26

[Jonathan Watt [:jwatt]]

Hmm, looks like I forgot to |hg add| the tests for the SVGPathSegList and SVGPointsList patches.

Comment 27

[Jonathan Watt [:jwatt]]

Attachment: patch for SVGPathSegList

Comment 28

[Jonathan Watt [:jwatt]]

Attachment: patch for SVGPointsList

Comment 29

[David Zbarsky (:dzbarsky)]

Comment on attachment 773139 [details] [diff] [review]
patch for SVGPointsList

Review of attachment 773139 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/crashtests/880544-5.svg
@@ +11,5 @@
> +  //]]></script>
> +
> +  <polygon id="e" points="0,0"/>
> +
> +</svg>

Did you add the <![CDATA[ because you plan to submit these tests somewhere?  I don't think we want this for our own tests.

Comment 30

[Jonathan Watt [:jwatt]]

(In reply to David Zbarsky (:dzbarsky) from comment #29)
> Did you add the <![CDATA[ because you plan to submit these tests somewhere? 
> I don't think we want this for our own tests.

We don't; I just didn't bother cutting that out of Jesse's original testcase. It doesn't matters though, and isn't rally worth spending time updating the patches for I'd think.

Comment 31

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fbdd1b84f77
https://hg.mozilla.org/integration/mozilla-inbound/rev/32d9640c3882
https://hg.mozilla.org/integration/mozilla-inbound/rev/4674ce0eda2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaba3099e010
https://hg.mozilla.org/integration/mozilla-inbound/rev/4056151eb62b

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4056151eb62b
https://hg.mozilla.org/mozilla-central/rev/aaba3099e010
https://hg.mozilla.org/mozilla-central/rev/4674ce0eda2a
https://hg.mozilla.org/mozilla-central/rev/32d9640c3882
https://hg.mozilla.org/mozilla-central/rev/9fbdd1b84f77

Comment 33

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed assertion on shutdown, ASan FF24 2013-06-06.
Verified fixed, ASan FF25 2013-10-01.