1288780 - Possible crash if object containing GCPtr destroyed after failed initialisation

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

The GCPtr destructor contains this assertion as to when it's safe to destroy one of these things:

        // No prebarrier necessary as this only happens when we are sweeping or
        // before the containing object becomes part of the GC graph.
        MOZ_ASSERT(CurrentThreadIsGCSweeping() || CurrentThreadIsHandlingInitFailure());

This doesn't take account of the fact that a post-barrier may have put a pointer to this GCPtr into the storebuffer.  If we do a minor GC in this state we'll attempt to mark whatever garbage ends up at this location.

This only happens when an object containing a GCPtr is destroyed due to initialization failure (otherwise they are destroyed during sweeping which is fine).  

We already have AutoInitGCManagedObject that is supposed to take care of issues like this for us.  A possible fix is to make that put the object on a queue whose contents are destroyed after the next minor GC.  Also, we need jsapi-tests for this.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1288780-gc-ptr

Here's a failing test case.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1288780-gc-ptr v1

Here's a patch to implement the proposed fix.

It removes AutoInitGCManagedObject and replaces it with GCManagedDeletePolicy.  The latter can be used as the delete policy for a UniquePtr, or used to implement the default delete policy for the relevant classes (I've done the latter here).

The delete policy queues a thunk to delete the object after the next minor GC, by which time the store buffer will be empty.  It uses a nursery allocation for the linked list element.  If the nursery is disabled or we are not on the main thread then the problem doesn't occur anyway and we can delete the object immediately.  Similarly, we evict the nursery instead if we can't allocate (and this path is exercised by our OOM testing).

UnboxedLayout caused me a lot of problems with the assertions in the GCPtr destructor because it can theoretically be constructed and hence destroyed off main thread (I never observed this though).  This is safe as stated previously.  I relaxed the assertion to allow destruction off main thread where the wrapper doesn't contain a nursery pointer.  I feel like that's a bit of a cop-out but couldn't see a better way.

Also, UnboxedLayout is constructed when we are inside an AutoSuppressGC region.  I had to add a UniquePtr in AutoEnterAnalysis for it to use so that it was cleaned up after then end of the GC suppression.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8775250 [details] [diff] [review]
bug1288780-gc-ptr v1

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

::: js/src/gc/Barrier.h
@@ +454,5 @@
> +        // after we have just collected the nursery.
> +        bool inNursery = InternalBarrierMethods<T>::isInsideNursery(this->value);
> +        MOZ_ASSERT(CurrentThreadIsGCSweeping() ||
> +                   CurrentThreadCanSkipPostBarrier(inNursery));
> +        memset(this, JS_FREED_HEAP_PTR_PATTERN, sizeof(*this));

Nice touch! Please use the Poison function so that we have global control over when this is enabled.

::: js/src/gc/Nursery.cpp
@@ +73,5 @@
> +    uint32_t padding;
> +#endif
> +};
> +
> +

Extra whitespace.

@@ +829,5 @@
> +        runtime()->gc.evictNursery();
> +        AutoSetThreadIsSweeping threadIsSweeping;
> +        thunk(data);
> +        return;
> +    }

Neat! Is the intention that we can implement our other ad-hoc sweeping activity with this mechanism?

Comment 4

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d757a1ca6d56

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8775250 [details] [diff] [review]
bug1288780-gc-ptr v1

Ah, I forgot to get approval before landing, sorry about that.

[Security approval request comment]

How easily could an exploit be constructed based on the patch? Difficult, would require an attacker to understand the changes and work out where this problem currently manifests itself.  I'm not sure it's even an issue in the current codebase just that it has the possibility to become one.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.

Which older supported branches are affected by this flaw? Pretty much everything.

If not all supported branches, which bug introduced the flaw? Bug 1165966 was the previous attempt to address this issue but the underlying problem may be have there since bug 619558.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Shouldn't be too hard to create patches along the same lines.

How likely is this patch to cause regressions; how much testing does it need? Unlikely.

Comment 6

[Carsten Book [:Tomcat]]

backed for causing hazard failures like https://treeherder.mozilla.org/logviewer.html#?job_id=32784246&repo=mozilla-inbound

Comment 7

[Jon Coppeard (:jonco)]

Steve, I want to do something like the following in a jsapi test:

    auto heapPtr = cx->make_unique<GCPtr<JSObject*>>();
    // ...
    cx->minorGC(JS::gcreason::API);

Is there any way I can do this without the hazard analysis complaining that heapPtr is unrooted?

Comment 8

[Daniel Veditz [:dveditz]]

Let's try to get this into aurora (49) now and then work on ESR for next cycle (e.g. the one shipping with 49).

Comment 9

[Steve Fink [:sfink] [:s:]]

I guess the problem is that you're basically putting a heap pointer on the stack, or rather in UniquePtr memory, which the analysis treats as unsafe as the stack.

Which is ok here because... oh, because it's a jsapi-test. So to confirm: you want a way to avoid the analysis for a test only, right? As in, it would still be correct for the analysis to complain for the above code normally?

Assuming so, I think this seems like a totally appropriate use of AutoSuppressGCAnalysis. You're sort of intentionally setting up the situation that the analysis protects against: you're putting a GC pointer into memory that doesn't get traced, and then you GC. If your test did another GC after the minorGC, then this would be a UAF, right?

I'm working on something that would allow annotating specific variables as holding only non-GC pointers even though their type says they can have GC pointers. It's intended for use in cases where you know you only have a numeric Value or whatever, but it would be handy here. I wouldn't wait for it, though; I'm not sure how feasible it is yet.

Comment 10

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a200a9215d7

Comment 11

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=32891639&repo=mozilla-inbound

Comment 12

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1506fafba57d

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1506fafba57d

Comment 14

[Jon Coppeard (:jonco)]

Attachment: bug1288780-beta

Approval Request Comment
[Feature/regressing bug #]: Bug 619558
[User impact if declined]: Possible (though unlikely) crash / security vulnerability.
[Describe test coverage new/current, TreeHerder]: On central since 30th July
[Risks and why]: Low
[String/UUID change made/needed]:

Comment 15

[Jon Coppeard (:jonco)]

Attachment: bug1288780-esr45

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Possible (though unlikely) crash / security vulnerability.
Fix Landed on Version: 50
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8777254 [details] [diff] [review]
bug1288780-beta

Sec-mod, still early in the beta cycle, Beta49+

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8777255 [details] [diff] [review]
bug1288780-esr45

Sec-mod, ESR45+

Comment 18

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-beta/rev/186f568472e7
https://hg.mozilla.org/releases/mozilla-esr45/rev/b08f28db372e

Comment 19

[Carsten Book [:Tomcat]]

(In reply to Jon Coppeard (:jonco) from comment #18)
> https://hg.mozilla.org/releases/mozilla-beta/rev/186f568472e7
> https://hg.mozilla.org/releases/mozilla-esr45/rev/b08f28db372e

setting flags for jon