1279618 - JSObject::finalize() accesses JSClass after calling finalize on it, possibly resulting in UAF for dynamically allocated classes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(no longer active)]

SpiderNode is hitting this bug: https://github.com/mozilla/spidernode/issues/164

What's happening is that JSObject::finalize() may call the class finalizer <https://dxr.mozilla.org/mozilla-central/source/js/src/jsobjinlines.h?from=JSObject%3A%3Afinalize#82> and still access the class later (see line 84, for example, or indirectly, line 87).  These accesses may cause a UAF if the js::Class has been allocated dynamically, and the finalizer frees it.

Comment 1

[Tom S [:evilpie]]

I was always thought that deallocating JSClasses is not something we support.

Comment 2

[(no longer active)]

Attachment: Don't touch the js::Class in JSObject::finalize() after having called the finalizer on it

Comment 3

[(no longer active)]

(In reply to Tom Schuster [:evilpie] from comment #1)
> I was always thought that deallocating JSClasses is not something we support.

As in, intentionally not support?  That's a weird restriction...

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I was always thought that deallocating JSClasses is not something we support.

I explicitly asked whether it was safe to assume the JSClass is not dereferenced after finalize before writing code that depended on it, fwiw...  Should have just looked at the code that calls finalizers.  :(

Anyway, Gecko totally deallocates js::Class all the time and has for forever.  See the XPCNativeScriptableShared class, which can go away, and its js::Class member.  So deallocating js::Classes is something SpiderMonkey had better support.  It's just that Gecko doesn't deallocate the js::Class until a separate mark/sweep pass after JS GC, afaict.  We _could_ do some sort of deferred deletion of our JSClass from the finalizer in SpiderNode, I guess; it'd just be nice to avoid the complexity if we can.  But it might be safer that way; looks like it's too easy in SpiderMonkey to poke the clasp of an object without even realizing it, so we should probably do our deallocation once whatever GC makes the js::Class dead is done.  Is there a notification we can register for to do that?

Comment 5

[Jan de Mooij [:jandem]]

XPConnect does do dynamic Class allocation/deallocation, but we're not thrilled about it and we want to remove it. See bug 1132502 comment 16 and the comments after that for an example of the problems this can cause. Internally we store Class pointers in groups, shapes, ICs, etc, so it's easy to have UAFs somewhere.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK.  So let's talk about the actual embedding situation here.

For SpiderNode, we are using custom JSClasses for two reasons at the moment: 1) special toString stuff and 2) extra internal slots.  We're implementing a V8 API that basically allows specifying that an object is to be created with the given (caller-provided) number of internal slots attached to it.   We might need some other facilities at some point; unclear so far.

For Gecko, the set of classes needed is basically dynamically registered at runtime via xpt files.

I'm pretty sure I've seen other embeddings doing dynamic class allocation too.

As I see it, we have the following options:

1) Embedders just never deallocate JSClasses, allow dynamically allocated ones to effectively leak.  This might be OK for the Gecko use case, because interfaces don't really get "unregistered".  It _might_ be OK for the spidernode use case, maybe.  I'm a lot less clear on that.

2) Embedders never dynamically allocate JSClasses, route around situations that need objects to have different "JSClass-provided" information.  For SpiderNode this would mean we just have a single JSClass and reimplement toString stuff using a @@toStringTag that examines internal slots (couldn't have done this when the current SpiderNode code was written!) and reimplement internal slots by stashing an object with null proto and indexed properties in an internal slot.  The performance will obviously be much worse... but we may well end up having to do something like this anyway because of the terrible SpiderMonkey support for internal slots on proxies.  :(

3)  SpiderMonkey provides other, more per-object, ways of implementing whatever people use JSClass for now.  This seems like a big project with lots of unknowns, starting with what people use JSClass for now.

4)  We make JSClass gced (terrence's suggestion).  Either in general (still his suggestion) or optionally, which allows not rewriting existing JSClasses.  If we don't have a free JSClass flag to indicate "I am GCed", we could allow registering one of the existing flags (the USERBIT* ones, presumably) with the runtime as a "I am a GCed JSClass" indicator.  GC would then handle those JSClasses.  Note that at least for the SpiderNode use case (but iirc also the xpconnect use case) the class strings are also dynamically allocated.  For GCed classes we'd need to figure out how this part should work; either require allocation of names via JS_malloc or make them somehow gcthing pointers or have a way to flag whether the name is static or allocated or something.

Other obvious options?

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> For
> SpiderNode this would mean we just have a single JSClass and reimplement
> toString stuff using a @@toStringTag that examines internal slots

Without @@toStringTag, we could also give classes an optional hook that lets them override clasp->name.

> and
> reimplement internal slots by stashing an object with null proto and indexed
> properties in an internal slot.

You can malloc some memory for the internal slots and stash a pointer in a reserved slot on the object, with a finalizer to free it. Maybe as an optimization you can use a few different classes for the (likely most common) 0-4 or 0-8 reserved slots cases?

> but we may well end up having to do something like this anyway
> because of the terrible SpiderMonkey support for internal slots on proxies. 
> :(

I'd be happy to work on this or any other SM blockers. Maybe worth having a SpiderNode meta bug. Proxies have their own performance issues though.

> 3)  SpiderMonkey provides other, more per-object, ways of implementing
> whatever people use JSClass for now.  This seems like a big project with
> lots of unknowns, starting with what people use JSClass for now.

We could potentially add a "number of extra internal slots" argument to the NewObject APIs and store that number in the BaseShape. It would be a little unfortunate and I need to think more about how this affects perf/correctness.

> 4)  We make JSClass gced (terrence's suggestion).  Either in general (still
> his suggestion) or optionally, which allows not rewriting existing
> JSClasses.

That's an interesting option. It's a fair amount of work though.

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 8762171 [details] [diff] [review]
Don't touch the js::Class in JSObject::finalize() after having called the finalizer on it

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

I'm going to jump in and r+ this.  Thanks for fixing.

Comment 9

[(no longer active)]

(In reply to Jan de Mooij [:jandem] from comment #7)
> (In reply to Boris Zbarsky [:bz] from comment #6)
> > For
> > SpiderNode this would mean we just have a single JSClass and reimplement
> > toString stuff using a @@toStringTag that examines internal slots
> 
> Without @@toStringTag, we could also give classes an optional hook that lets
> them override clasp->name.

That would still require dynamic allocation of the classes, no?

> > and
> > reimplement internal slots by stashing an object with null proto and indexed
> > properties in an internal slot.
> 
> You can malloc some memory for the internal slots and stash a pointer in a
> reserved slot on the object, with a finalizer to free it. Maybe as an
> optimization you can use a few different classes for the (likely most
> common) 0-4 or 0-8 reserved slots cases?

Hmm, in the case of SpiderNode, we need some proper support for an arbitrary number of slots, so having a few classes like that wouldn't help, since in theory the embedder can pass any value for the number of slots.

> > but we may well end up having to do something like this anyway
> > because of the terrible SpiderMonkey support for internal slots on proxies. 
> > :(
> 
> I'd be happy to work on this or any other SM blockers. Maybe worth having a
> SpiderNode meta bug. Proxies have their own performance issues though.

So far we've been using the [v8api] whitespace tag, FWIW.

> > 3)  SpiderMonkey provides other, more per-object, ways of implementing
> > whatever people use JSClass for now.  This seems like a big project with
> > lots of unknowns, starting with what people use JSClass for now.
> 
> We could potentially add a "number of extra internal slots" argument to the
> NewObject APIs and store that number in the BaseShape. It would be a little
> unfortunate and I need to think more about how this affects perf/correctness.

That should work well for SpiderNode (assuming we have a suitable way of overriding the class name too.)

> > 4)  We make JSClass gced (terrence's suggestion).  Either in general (still
> > his suggestion) or optionally, which allows not rewriting existing
> > JSClasses.
> 
> That's an interesting option. It's a fair amount of work though.

This question may be naive, but what actual things can happen to the class after the last object with that class is finalized?  Is that not something that we can at least detect in debug build, turning potential UAFs into assertions?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a69a6628249
Don't touch the js::Class in JSObject::finalize() after having called the finalizer on it; r=jonco

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a69a6628249

Comment 12

[Jan de Mooij [:jandem]]

(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #9)
> > Without @@toStringTag, we could also give classes an optional hook that lets
> > them override clasp->name.
> 
> That would still require dynamic allocation of the classes, no?

You could for instance store the class name in an internal slot on the object and then the Class hook would take the object as argument and get the name from that slot.

> > You can malloc some memory for the internal slots and stash a pointer in a
> > reserved slot on the object, with a finalizer to free it. Maybe as an
> > optimization you can use a few different classes for the (likely most
> > common) 0-4 or 0-8 reserved slots cases?
> 
> Hmm, in the case of SpiderNode, we need some proper support for an arbitrary
> number of slots, so having a few classes like that wouldn't help, since in
> theory the embedder can pass any value for the number of slots.

Right, I meant you'd have a generic-but-less-optimal option, and then special case the (likely most common) 0-4 slots cases, just as an optimization to avoid malloc or object allocation.

> This question may be naive, but what actual things can happen to the class
> after the last object with that class is finalized?  Is that not something
> that we can at least detect in debug build, turning potential UAFs into
> assertions?

In bug 1132502 we had memory reporting code iterating over all base shapes and then looking at clasp->name, to group base shapes by Class name. That code has been disabled and I'm not aware of any similar cases, but it's hard to say. Not sure what we can do there.

Comment 13

[Terrence Cole [:terrence]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> > I was always thought that deallocating JSClasses is not something we support.
> 
> I explicitly asked whether it was safe to assume the JSClass is not
> dereferenced after finalize before writing code that depended on it, fwiw...
> Should have just looked at the code that calls finalizers.  :(
> 
> Anyway, Gecko totally deallocates js::Class all the time and has for
> forever.  See the XPCNativeScriptableShared class, which can go away, and
> its js::Class member.  So deallocating js::Classes is something SpiderMonkey
> had better support.  It's just that Gecko doesn't deallocate the js::Class
> until a separate mark/sweep pass after JS GC, afaict.  We _could_ do some
> sort of deferred deletion of our JSClass from the finalizer in SpiderNode, I
> guess; it'd just be nice to avoid the complexity if we can.  But it might be
> safer that way; looks like it's too easy in SpiderMonkey to poke the clasp
> of an object without even realizing it, so we should probably do our
> deallocation once whatever GC makes the js::Class dead is done.  Is there a
> notification we can register for to do that?

I think the JSGC_END callback is probably what you want. It is called last thing before the |ret| after the last slice of all major GCs. Indeed, looking at the users, it appears we already have at least two systems for doing deferred releases in gecko after a GC.

All that said, I've actually come around to everyone else's way of thinking and agree that we should just keep doing as this bug does and fixing it like a UAF until it's okay for the clasp to delete itself in its finalize op.

Comment 14

[Nicholas Nethercote [inactive]]

> In bug 1132502 we had memory reporting code iterating over all base shapes
> and then looking at clasp->name, to group base shapes by Class name. That
> code has been disabled and I'm not aware of any similar cases, but it's hard
> to say. Not sure what we can do there.

FWIW, after I "fixed" bug 1132502 (by merely disabling the relevant code, resulting in less informative memory reports) I filed bug 1265271 about disallowing non-static js::Class instances, because there was at least some consensus that it was a desirable thing.