1325406 - FakeRooted doesn't work with ValueOperations

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

We have this nice system where ValueOperations/MutableValueOperations and others define interface methods for use with wrapped pointer types like Rooted, Handle etc.  For example, when you call setUndefined() on a Rooted<Value> it extracts a reference to the wrapped value and calls the method on it.

This works through mixin base classes like RootedBase, HandleBase, etc which are specialised for types like Value to provide the interface operations.  The specialisations derive from classes like MutableValueOperations which actually implement the operations.

Currently both Rooted and FakeRooted derive from RootedBase but the specialisation of RootedBase for Value derives MutableValueOperations<Rooted<Value>>.  If you use this with a FakeRooted it will cast your FakeRooted to a Rooted and access it.  These don't have the same representation and the program will crash.

The same probably applies to other combinations of wrapped / wrapper classes.

Comment 1

[Jon Coppeard (:jonco)]

Marking s-s just in case because there's some weirdness where BarrieredBaseMixins<Value> casts to WriteBarrieredBase even though it could be used with a ReadBarriered (and hence might skip the barrier).

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1325406-rooted-base-classes

What we need to do is to pass the type of the wrapper class as well as the type of the wrapped thing.

We can also get rid of one layer of indirection by making all the different FooOperation / MutableFooOperation classes into WrapperPtrOperations / MutableWrappedPtrOperations classes templated on wrapped pointer and wrapper class types.  For example, RootedBase now pulls in MutableWarppedPtrOperations by default.  This removes quite a lot of boilerplate.

Any ideas for a better name that WrapperPtrOperations (we already have proxy-related wrappers which are an entirely different concept)?

Also, It's possible that we could remove RootedBase and related classes completely and just pull in MutableWrappedPtrOperations but there are a couple of places where we do use this extra flexibility.

For the barrier issue, I removed the inheritance on BarrieredBase and put it in WriteBarrieredBase and ReadBarriered separately.  I has to put a 'using' delcaration so that use of 'value' referred to BarrieredBase::value and not WrappedPtrOperations<Value>::value.

InnerViewTable previously exposed a bunch of private methods via this mechanism that are only used from JSCompartment.  This seemed unnecessary so I got rid of this and used get() in the few places that needed it.

Comment 3

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

Comment on attachment 8821222 [details] [diff] [review]
bug1325406-rooted-base-classes

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

This is terrifying, but I like having RootedBase automatically pull in operations. And that's some awful boilerplate you've managed to kill off.

::: js/public/RootingAPI.h
@@ +113,5 @@
>  struct BarrierMethods {
>  };
>  
> +template <typename Inner, typename Outer>
> +class WrappedPtrOperations {};

The only other thing that comes to mind is

template <typename Element, typename Container> // or Containee?
class PtrContainerOperations {}; // or ContainedPtrOperations?

I used the 'element' term for the 'using ElementType = T;' declarations for the operator== stuff. Probably ought to use the same terminology, whatever it is. Hm... come to think of it, could we now lift ElementType into this class?

::: js/public/SweepingAPI.h
@@ -11,5 @@
>  
> -namespace js {
> -template <typename T>
> -class WeakCacheBase {};
> -} // namespace js

I guess we weren't using that layer.

::: js/src/ds/TraceableFifo.h
@@ -114,5 @@
> -template <typename A, size_t B, typename C>
> -class HandleBase<TraceableFifo<A,B,C>>
> -  : public TraceableFifoOperations<JS::Handle<TraceableFifo<A,B,C>>, A,B,C>
> -{
> -    using TF = TraceableFifo<A,B,C>;

\o/ That's some ugly boilerplate to be killing off.

::: js/src/jsobj.h
@@ +590,5 @@
>      void operator=(const JSObject& other) = delete;
>  };
>  
> +template <typename Outer>
> +template <typename U>

Achievement unlocked! (Two consecutive template <>s.)

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #3)

> template <typename Element, typename Container> // or Containee?
> class PtrContainerOperations {}; // or ContainedPtrOperations?

I've kind of got used to WrappedPtrOperations now.  Naming the template parameters Element and Container is a good idea, I'll do that.

> Hm... come to think of it, could we now lift ElementType into this class?

We'd need everyone who specialised the template to define this themselves though in that case (or pull it in from somewhere else via a base class).  It's probably easier to leave it as it is.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1325406-rooted-base-classes

Updated patch with changes to browser templates and a tweak to InitialShapeEntry::needsSweep() to not trigger the read barrier on the tagged proto, just like it already did for the shape.  The later was causing an assert - we now actually trigger the read barrier in a few places where we didn't before.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8824488 [details] [diff] [review]
bug1325406-rooted-base-classes

Requesting additional review for browser changes.

The patch is described in comment 2.  From the browser POV there shouldn't be any change at all.  I added the extra template parameter, removed some conversion operations that get supplied by other parts of the system already and for OwningNonNull and RefPtr changed the template that's being specialised from RootedBase to the more generic WrappedPtr so that these methods get pulled in to the other rooting classes as well.

Comment 7

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Comment on attachment 8824488 [details] [diff] [review]
bug1325406-rooted-base-classes

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

I don't understand these changes, but they look benign enough from a browser perspective.

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 8824488 [details] [diff] [review]
bug1325406-rooted-base-classes

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Extremely difficult.

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?

Everything back to 43.

If not all supported branches, which bug introduced the flaw?  

Bug 1205054.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This will require some rewriting (I think some things have been renamed) but it should be straightforward.

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

Unlikely.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8824488 [details] [diff] [review]
bug1325406-rooted-base-classes

As a sec-audit, this doesn't need sec-approval+ to land so I'm clearing the flag.

If we wanted to take it on branches, someone would have to make a case for doing so as the main bugs we port backwards are sec-high and sec-critical rated issues.

Comment 10

[Jon Coppeard (:jonco)]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Dan, do you think we should request ESR52 approval on this?

Comment 13

[Daniel Veditz [:dveditz]]

Doesn't seem like the kind of "definitely severe" vulnerability we take on ESR.