Closed Bug 1024250 Opened 5 years ago Closed 5 years ago

Fix several places where we are not calling ExposeToActiveJS

Categories

(Core :: JavaScript: GC, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed
firefox-esr31 --- wontfix
b2g-v1.4 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.1 --- fixed
b2g-v2.2 --- unaffected

People

(Reporter: terrence, Assigned: terrence)

References

Details

(Keywords: sec-moderate)

Attachments

(6 files, 6 obsolete files)

2.10 KB, patch
bzbarsky
: review+
terrence
: checkin+
Details | Diff | Splinter Review
196.54 KB, patch
billm
: feedback+
Details | Diff | Splinter Review
5.23 KB, text/plain
Details
36.03 KB, patch
billm
: review+
terrence
: checkin+
Details | Diff | Splinter Review
22.30 KB, patch
mccr8
: review-
jonco
: review+
Details | Diff | Splinter Review
13.49 KB, patch
mccr8
: review+
jonco
: review+
Details | Diff | Splinter Review
Now that we have the ability to trivially add solid assertions from bug 1017650, I am tracking down the places where we are missing ExposeToActiveJS. There are quite a few.
Attachment #8438820 - Flags: feedback?(wmccloskey)
Using the read barrier for exposing worked out pretty well. There are only a couple cases now where we need a specific ExposeToActiveJS.
Attachment #8438820 - Attachment is obsolete: true
Attachment #8438820 - Flags: feedback?(wmccloskey)
Attachment #8438846 - Flags: feedback?(wmccloskey)
Note, this is still missing something: it's falling over after about 10 minutes on mochitest-plain. Will debug more tomorrow and hopefully work through the rest of the issues.
Comment on attachment 8438846 [details] [diff] [review]
fix_assertions_from_rooted-v1.diff

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

Looks pretty good. When you're ready to ask for review, please put it all in one big patch with the stuff from public bug.

::: js/public/RootingAPI.h
@@ +945,5 @@
> +class RootedPreservingColor : private Rooted<T>
> +{
> +  public:
> +    explicit RootedPreservingColor(JSContext *cx) : Rooted<T>(cx) {}
> +    void set(const T &t) { this->ptr = t; }

Any reason not to have this support the same interface as Rooted (operator= and a constructor that takes a T*)?

::: js/src/jscompartment.cpp
@@ +401,5 @@
>          return true;
>      }
>  
>      RootedObject proto(cx, TaggedProto::LazyProto);
> +    RootedObject existing(cx);

I'm pretty sure we want to use RootedPreserveColor here.

::: js/src/jsinfer.cpp
@@ +2331,5 @@
>          }
>      }
>  
>      for (gc::ZoneCellIter i(cx->zone(), gc::FINALIZE_SCRIPT); !i.done(); i.next()) {
> +        JS::RootedPreservingColor<JSScript *> script(cx);

This really should be a bare pointer. If we ever GC here, we're in serious trouble. We already assert there can be no allocation during ZoneCellIter.

::: js/src/jspropertytree.cpp
@@ +174,5 @@
>              JS_ASSERT(parent->isMarked());
>              parent->removeChild(existingShape);
>              existingShape = nullptr;
> +        } else if (existingShape->isMarked(gc::GRAY)) {
> +            /* Shape may have been marked gray through a separate lineage. */

...or the same lineage. Just remove that clause.

::: js/src/vm/Shape.cpp
@@ +620,5 @@
>          Rooted<UnownedBaseShape*> nbase(cx);
>          if (last->base()->matchesGetterSetter(getter, setter) && !indexed) {
> +            /* |unowned| may have been marked gray through a separate lineage. */
> +            UnownedBaseShape *unowned = last->base()->unowned();
> +            JS::ExposeGCThingToActiveJS(unowned, JSTRACE_BASE_SHAPE);

If everything else is correct, I don't think this should ever be gray. |unowned| is reachable from |obj|, which should be black. If |nbase| is coming out gray, then please see if |obj| is gray and why.

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +578,5 @@
>      // Create a waiver in the new compartment. We know there's not one already
>      // because we _just_ transplanted, which means that |newobj| was either
>      // created from scratch, or was previously cross-compartment wrapper (which
>      // should have no waiver). CreateXrayWaiver asserts this.
> +    RootedObject newWaiver(cx, WrapperFactory::CreateXrayWaiver(cx, newobj));

Is there any purpose here besides being nice to use Rooted? It seems like it should be a new object and therefore always black.
Attachment #8438846 - Flags: feedback?(wmccloskey) → feedback+
(In reply to Bill McCloskey (:billm) from comment #3)
> Comment on attachment 8438846 [details] [diff] [review]
> ::: js/public/RootingAPI.h
> @@ +945,5 @@
> > +class RootedPreservingColor : private Rooted<T>
> > +{
> > +  public:
> > +    explicit RootedPreservingColor(JSContext *cx) : Rooted<T>(cx) {}
> > +    void set(const T &t) { this->ptr = t; }
> 
> Any reason not to have this support the same interface as Rooted (operator=
> and a constructor that takes a T*)?

No good reason. Was less typing when I was initially testing and I never went back to clean up.

> ::: js/src/jscompartment.cpp
> @@ +401,5 @@
> >          return true;
> >      }
> >  
> >      RootedObject proto(cx, TaggedProto::LazyProto);
> > +    RootedObject existing(cx);
> 
> I'm pretty sure we want to use RootedPreserveColor here.

Ah, good catch! Maybe that hopefully explains the M-oth failures here:
https://tbpl.mozilla.org/?tree=Try&rev=05442e0826de

> ::: js/src/vm/Shape.cpp
> @@ +620,5 @@
> >          Rooted<UnownedBaseShape*> nbase(cx);
> >          if (last->base()->matchesGetterSetter(getter, setter) && !indexed) {
> > +            /* |unowned| may have been marked gray through a separate lineage. */
> > +            UnownedBaseShape *unowned = last->base()->unowned();
> > +            JS::ExposeGCThingToActiveJS(unowned, JSTRACE_BASE_SHAPE);
> 
> If everything else is correct, I don't think this should ever be gray.
> |unowned| is reachable from |obj|, which should be black. If |nbase| is
> coming out gray, then please see if |obj| is gray and why.

In that case it may be a total pain to hunt down where this should have gotten unmarked; iirc, this only asserted after 10+ minutes on mochitest-browser.

> ::: js/xpconnect/wrappers/WrapperFactory.cpp
> @@ +578,5 @@
> >      // Create a waiver in the new compartment. We know there's not one already
> >      // because we _just_ transplanted, which means that |newobj| was either
> >      // created from scratch, or was previously cross-compartment wrapper (which
> >      // should have no waiver). CreateXrayWaiver asserts this.
> > +    RootedObject newWaiver(cx, WrapperFactory::CreateXrayWaiver(cx, newobj));
> 
> Is there any purpose here besides being nice to use Rooted? It seems like it
> should be a new object and therefore always black.

Two birds, one stone. I handlified RemapWrappers since I needed to change one of the Rooted declarations anyway. |newWaiver| gets passed to the newly-handlifed method below, so this is just moving the Rooted above the call.
Attachment #8438846 - Attachment is obsolete: true
Attachment #8441071 - Flags: review?(wmccloskey)
Comment on attachment 8441071 [details] [diff] [review]
auto_expose_to_active_js_from_heap-v4.diff

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

Note: this does /not/ include the new assertions that I used to find most of the errors. I'm afraid there may still be quite a long tail of these hazards to find and fix and I don't want to hold up plugging the holes we know about now to find rarer ones. I also had substantial trouble stabilizing this change, even without the assertions; not surprising, considering that it changes how read barriers and gray marking work at a fundamental level.

Hopefully, I didn't mess up anything too badly when applying the last round of comments:
https://tbpl.mozilla.org/?tree=Try&rev=a8cd48ba13a3

::: js/public/GCAPI.h
@@ +436,5 @@
>   */
>  extern JS_FRIEND_API(bool)
>  UnmarkGrayGCThingRecursively(void *thing, JSGCTraceKind kind);
>  
> +namespace detail {

I used "detail" because it was less cumbersome that leaving JS to enter js::gc::. I can change this if you'd like.

@@ +442,2 @@
>  static MOZ_ALWAYS_INLINE void
>  ExposeGCThingToActiveJS(void *thing, JSGCTraceKind kind)

I changed the name (with a namespace) to make sure we were null-checking all of the callers, then realized that there was only one user of the fully generic method, so kept the name change.

::: js/public/RootingAPI.h
@@ +682,5 @@
>  struct GCMethods<T *>
>  {
>      static T *initial() { return nullptr; }
>      static bool poisoned(T *v) { return JS::IsPoisonedPtr(v); }
> +    static bool isMarkedGray(T *v) { return v && JS::GCThingIsMarkedGray(v); }

This is not quite adequate and will need to be expanded when I re-enable the assertions. Specifically, this does not deal with SHAPE_COLLISION and SHAPE_REMOVED [(Shape*)1] and LAZY_SINGLETON [(types::TypeObject*)1]. I'm not entirely sure how to specialize this without adding |<1| checks to all T* here. Any ideas would be appreciated.

@@ +699,5 @@
> +    static bool isMarkedGray(JSObject *v) { return v && JS::GCThingIsMarkedGray(v); }
> +    static void exposeToActiveJS(JSObject *v) {
> +        if (v)
> +            JS::ExposeObjectToActiveJS(v);
> +    }

Note: there is no exposeToActiveJS on GCMethods<T*>, only on GCMethods<JSObject*>. This is because there are currently only a couple of users that directly put a Heap<T> into a Rooted<T>. Most users go through an accessor that returns a raw pointer.

@@ +718,5 @@
>  /* This helper allows us to assert that Rooted<T> is scoped within a request. */
>  extern JS_PUBLIC_API(bool)
>  IsInRequest(JSContext *cx);
> +#else
> +static bool IsInRequest(JSContext *cx) { return true; }

So we can kill the #ifdef JS_DEBUG below. I'm not sure why JS_DEBUG does not overlap perfectly with MOZ_ASSERT. Seems fishy to me, but that's not this bug.

::: js/public/Value.h
@@ +1296,5 @@
> +        return ExposeObjectToActiveJS(&v.toObject());
> +    else if (v.isString())
> +        return ExposeStringToActiveJS(v.toString());
> +    JS_ASSERT(!v.isMarkable());
> +}

Since isMarkable != isGCThing, I thought this form would be a bit clearer. Now that I'm looking at it again, I'm not sure anymore. Thoughts?

::: js/src/jsapi-tests/testWeakMap.cpp
@@ +181,5 @@
>       */
>      RootedObject object(cx);
>      {
>          JSAutoCompartment ac(cx, destZone);
> +        object = JS_NewObject(cx, nullptr, JS::NullPtr(), JS::NullPtr());

Caused the build to fail with the new test added because it changed the unified split.

::: js/src/jsapi.h
@@ +2957,5 @@
> +    static bool isMarkedGray(const JSPropertyDescriptor &desc) {
> +        return (desc.obj && JS::GCThingIsMarkedGray(desc.obj)) ||
> +               (desc.attrs & JSPROP_GETTER && desc.getter && JS::GCThingIsMarkedGray((void *)desc.getter)) ||
> +               (desc.attrs & JSPROP_SETTER && desc.setter && JS::GCThingIsMarkedGray((void *)desc.setter)) ||
> +               (desc.value.isGCThing() && JS::GCThingIsMarkedGray(desc.value.toGCThing()));

I don't actually need to check anything about desc.value before calling. Will remove the extra check.

::: js/src/jsfriendapi.cpp
@@ +989,5 @@
>      if (!ptr)
>          return;
>  
> +    if (kind == JSTRACE_STRING && StringIsPermanentAtom(static_cast<JSString *>(ptr)))
> +        return;

We're calling IncrementalReferenceBarrier on a larger set of things; this and the inclusion of JSTRACE_JITCODE are in support of that.

::: js/src/vm/Stack.cpp
@@ +370,5 @@
>      }
>      if (IS_GC_MARKING_TRACER(trc))
>          script()->compartment()->zone()->active = true;
> +    if (hasReturnValue())
> +        gc::MarkValueUnbarriered(trc, &rval_, "rval");

returnValue() returns a MutableHandle, so this would assert if rval_ happens to be gray. The new layout happens to be closer to the style of the rest of this trace hook anyway.

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +577,5 @@
>      // Create a waiver in the new compartment. We know there's not one already
>      // because we _just_ transplanted, which means that |newobj| was either
>      // created from scratch, or was previously cross-compartment wrapper (which
>      // should have no waiver). CreateXrayWaiver asserts this.
> +    RootedObject newWaiver(cx, WrapperFactory::CreateXrayWaiver(cx, newobj));

For RemapAllWrapeprsForObject, which wants a Handle now.
The majority of the missing ExposeToActiveJS were in SM, but there were some in Gecko too.
Attachment #8441085 - Flags: review?(bzbarsky)
Comment on attachment 8441085 [details] [diff] [review]
more_expose_to_active_js_in_browser-v0.diff

> nsDOMFileReader::GetResult(JSContext* aCx, JS::MutableHandle<JS::Value> aResult)

Shouldn't there be an ExposeToActiveJS call somewhere here?  It's not clear to me why the new code is more correct than the old, and if it's not clear to me it won't be clear to other people reading the code either...

>+++ b/dom/bindings/DOMJSProxyHandler.h
>+    JS::ExposeValueToActiveJS(v);
>     if (v.isObject()) {

Can we move that expose call into the isObject() case here?  I'd think we can.

And maybe the second call in this function only in the !v.isUndefined() case?  This code is a bit perf-sensitive, so if we know we don't have an object we shouldn't waste time trying to expose the value.

r=me with that, I think
Attachment #8441085 - Flags: review?(bzbarsky) → review+
I'm going to mark this sec-high.  Does that seem reasonable to you, Terrence, or should it just be sec-moderate?  I don't know how easy to exploit this would actually be.
Keywords: sec-high
(In reply to Boris Zbarsky [:bz] from comment #8)
> Comment on attachment 8441085 [details] [diff] [review]
> more_expose_to_active_js_in_browser-v0.diff
> 
> > nsDOMFileReader::GetResult(JSContext* aCx, JS::MutableHandle<JS::Value> aResult)
> 
> Shouldn't there be an ExposeToActiveJS call somewhere here?  It's not clear
> to me why the new code is more correct than the old, and if it's not clear
> to me it won't be clear to other people reading the code either...

The expose happens in MutableHandle<JSObject*>::set(Heap<JSObject*>). We could potentially also have it in MutableHandle<Value>::setObject(Heap<T>) -- I didn't because it would have to shadow MutableValueOperations<MutableHandle<Value>>::SetObject(JSObject&). This would be weird because the shadow is a different indirection level and thus would still allow incorrect usage: e.g. aResult.setObject(result) would compile, work normally, and be correct while aResult.setObject(*result) would /also/ compile, work fine most of the time, but still be totally incorrect.

I'm not terribly pleased with the situation either, but at least we'll be able to bandage it with assertions soon. Not great, but a step in the right direction.
(In reply to Andrew McCreight [:mccr8] from comment #9)
> I'm going to mark this sec-high.  Does that seem reasonable to you,
> Terrence, or should it just be sec-moderate?  I don't know how easy to
> exploit this would actually be.

I'd have thought sec-crit. It's not going to be particularly easy to exploit, but none of the techniques are new ground. Before, even we didn't know where to look, but now that we know it's a black->gray edge out of the weak caches, it should be pretty easy to construct one at will. From there it's just a matter of controlling gc/cc timing.
> The expose happens in MutableHandle<JSObject*>::set(Heap<JSObject*>)

There is no MutableHandle<JSObject*>::set(Heap<JSObject*>) being called here, though.  There's Rooted<JSObject*>::set(), being called, and MutableHandle<Value>::setObject()...

Also, aren't there now browser callsites that have redundant Expose*ToActiveJS calls?
(In reply to Terrence Cole [:terrence] from comment #11)
> (In reply to Andrew McCreight [:mccr8] from comment #9)
> > I'm going to mark this sec-high.  Does that seem reasonable to you,
> > Terrence, or should it just be sec-moderate?  I don't know how easy to
> > exploit this would actually be.
> 
> I'd have thought sec-crit. It's not going to be particularly easy to
> exploit, but none of the techniques are new ground. Before, even we didn't
> know where to look, but now that we know it's a black->gray edge out of the
> weak caches, it should be pretty easy to construct one at will. From there
> it's just a matter of controlling gc/cc timing.

We still don't have any evidence that black to gray edges are worse than a null pointer dereference. The GC won't collect objects that are marked gray at all. And the CC "frees" object by nulling out their outgoing pointers. There might be mistakes elsewhere that are somehow triggered by black to gray edges, but we don't know of any right now. So sec-crit seems overly conservative to me.
Well, that's true to an extent, but it turns out that CSS style stuff appears to rely on the correctness of CC to prevent UAFs.  See bug 1011391.  But yeah, I was thinking sec-high because it isn't entirely clear how to exploit this.  In practical terms, the main difference is how much haranguing there is about the bugs, and this one is being actively worked on, so it shouldn't matter too much.
Good points, let's stick with sec-high and see how the crash-stats respond as we fix issues here.
(In reply to Boris Zbarsky [:bz] from comment #12)
> > The expose happens in MutableHandle<JSObject*>::set(Heap<JSObject*>)
> 
> There is no MutableHandle<JSObject*>::set(Heap<JSObject*>) being called
> here, though.  There's Rooted<JSObject*>::set(), being called, and
> MutableHandle<Value>::setObject()...
> 
> Also, aren't there now browser callsites that have redundant
> Expose*ToActiveJS calls?

I expect there are not so many of these. I looked at 4 ExposeToActiveJS, selected randomly, and was not able to simply remove any of them. I think 2 of them I was able to remove after punching the Heap<T> through the accessor, one took a bit of refactoring, and the last was needed even with the automatic exposing.
Comment on attachment 8441071 [details] [diff] [review]
auto_expose_to_active_js_from_heap-v4.diff

Gah! With a bit more testing, the current read barrier seems to not be quite adequate. Specifically we need to unmark gray even when !needsBarrier; however, try was orange with leaks when not checking needsBarrier first. It seems there must be a mismatch between what the two needsBarrier implementations are doing.
Attachment #8441071 - Flags: review?(wmccloskey)
> I expect there are not so many of these.

What about my other question in comment 12?

> I looked at 4 ExposeToActiveJS, selected randomly, and was not able to simply remove any
> of them

Hmm.  Maybe I don't understand when they can be removed.

Can ImageData::GetData just do aData.set(mData) without needing the ExposeObject bit it's picking up from GetDataObject() right now, for example?
(In reply to Boris Zbarsky [:bz] from comment #12)
> > The expose happens in MutableHandle<JSObject*>::set(Heap<JSObject*>)
> 
> There is no MutableHandle<JSObject*>::set(Heap<JSObject*>) being called
> here, though.  There's Rooted<JSObject*>::set(), being called, and
> MutableHandle<Value>::setObject()...

Sorry, yes, I meant Rooted<JSObject*>::set. Basically, loading mResultArrayBuffer onto the stack as an object rather than a Value forces us to use set rather than setObject. Note: resultValue.set(ObjectValue(*mResultArrayBuffer)) would have also worked.
 
(In reply to Boris Zbarsky [:bz] from comment #18)
> 
> Hmm.  Maybe I don't understand when they can be removed.
> 
> Can ImageData::GetData just do aData.set(mData) without needing the
> ExposeObject bit it's picking up from GetDataObject() right now, for example?

No, that call can not yet be removed. In that case, mData is a Heap<T>, but GetDataObject returns a JSObject*. This invokes |Heap<JSObject*>::operator T() const|, returning the raw JSObject* on the stack. Next we get the normal |MutableHandle<JSObject*>::set(const JSObject*&)|, which behaves as normal.

In order to remove this ExposeToActiveJS, we'd simply need to make GetDataObject return some variant of JS::Heap<JSObject*> so that we end up invoking the new MutableHandle<JSObject*>::set(const Heap<JSObject*>&). Of course, I would like to have the assertions present before doing this so that there is less danger of accidentally missing a call to the accessor that does not immediately root.
> Basically, loading mResultArrayBuffer onto the stack as an object rather than a Value
> forces us to use set rather than setObject

OK.  Can you add a comment to that effect so someone doesn't "improve" the code later?

> No, that call can not yet be removed. In that case, mData is a Heap<T>, but GetDataObject
> returns a JSObject*.

I'm talking about changing GetData() to not call GetDataObject() at all.  Was "aData.set(mData)" unclear?
Flags: needinfo?(terrence)
(In reply to Boris Zbarsky [:bz] from comment #20)
> > Basically, loading mResultArrayBuffer onto the stack as an object rather than a Value
> > forces us to use set rather than setObject
> 
> OK.  Can you add a comment to that effect so someone doesn't "improve" the
> code later?

   if (mDataFormat == FILE_AS_ARRAYBUFFER) {
     JS::Rooted<JSObject*> result(aCx);
     if (mReadyState == nsIDOMFileReader::DONE && mResultArrayBuffer) {
-      result.set(mResultArrayBuffer);
+      // Copying the object to the stack causes Rooted
+      // to automatically ExposeObjectToActiveJS for us.
+      result = mResultArrayBuffer;
     }
     if (!JS_WrapObject(aCx, &result)) {
       return NS_ERROR_FAILURE;
     }

> > No, that call can not yet be removed. In that case, mData is a Heap<T>, but GetDataObject
> > returns a JSObject*.
>
> I'm talking about changing GetData() to not call GetDataObject() at all. 
> Was "aData.set(mData)" unclear?

Yeah, wow, sorry. No, not unclear at all; the problem is all on this side.
Flags: needinfo?(terrence)
>+      // Copying the object to the stack causes Rooted
>+      // to automatically ExposeObjectToActiveJS for us.

Thanks.

So the aData.set(mData) thing would be safe?

I guess things that do MutableHandleValue::setObject(*myHeapObject) would still need the explicit expose calls, right?
But e.g. most ExposeValueToActiveJS could likely go way?
(In reply to Boris Zbarsky [:bz] from comment #22)
> >+      // Copying the object to the stack causes Rooted
> >+      // to automatically ExposeObjectToActiveJS for us.
> 
> Thanks.
> 
> So the aData.set(mData) thing would be safe?

Yes.

> I guess things that do MutableHandleValue::setObject(*myHeapObject) would
> still need the explicit expose calls, right?

The more I think about, the more I think we need to make MutableHandle<Value>::setFoo(Heap<Value>) just do the right thing somehow. I'll experiment.

(In reply to Boris Zbarsky [:bz] from comment #23)
> But e.g. most ExposeValueToActiveJS could likely go way?

Yes, I think with a bit of simple work the majority could be removed. The one case I'm worried about is non GCing regions that touch JSAPI. If the code isn't handlifed, we'll have no reason to expose or, critically, make the assertions at all.
(In reply to Terrence Cole [:terrence] from comment #24)
> (In reply to Boris Zbarsky [:bz] from comment #23)
> > I guess things that do MutableHandleValue::setObject(*myHeapObject) would
> > still need the explicit expose calls, right?
> 
> The more I think about, the more I think we need to make
> MutableHandle<Value>::setFoo(Heap<Value>) just do the right thing somehow.
> I'll experiment.

Thinking about it /even/ more, this whole trip is just wrong. What we really want is a read barrier on Heap<T> to automatically ExposeToActiveJS. Moreover, the condition we want to check is not "greyness" as such, but "is touched between GC and CC". In effect this should just be a phase, explicitly exposed for inlining in JS::shadow::runtime, to allow us to make a very-well-predicted bool test instead of an unpredictable (and likely uncached) math, mask, and test on the mark bits.

I think the only hard part here is going to be finding all the places where we need getPreserveColor to avoid the barrier -- I'm guessing I can just grep -i for preservecolor to get the majority of them.

Andrew, do you see any obvious holes in this plan? I'm assuming I'll want to enter the "between GC and CC state" as soon as we've done gray marking, where do I need to leave it? Before or after graph building? Would you mind if I also added GC states for "in the middle of CC" and "between CC slices" too? I really want to add assertions that we are interlocking our GC and CC state properly.
Flags: needinfo?(continuation)
Bill, does comment 25 seem like a reasonable thing to do?
Flags: needinfo?(wmccloskey)
Tried this out today, appears to work well, except for workers. They seem to have gray things, but GetGCRuntime(thing)->needsGrayBarrier_ is false. Will debug this more Monday, now that I'm fairly certain this is the right path to be on.
The flag could certainly be wrong for workers.  I could imagine that we forget to change it when the CC was enabled.
I like the idea of moving the Expose calls to getters on Heap, assuming it doesn't expose too much. I'm guessing you'll have to play around with that to see how well it works.

I don't think that an "is between GC and CC" flag is workable, though. Gray bits are always supposed to be valid (except in the middle of a GC). Even after we're done CCing, we can CC again without an intervening GC.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(continuation)
(In reply to Bill McCloskey (:billm) from comment #29)
> I like the idea of moving the Expose calls to getters on Heap, assuming it
> doesn't expose too much. I'm guessing you'll have to play around with that
> to see how well it works.
> 
> I don't think that an "is between GC and CC" flag is workable, though. Gray
> bits are always supposed to be valid (except in the middle of a GC). Even
> after we're done CCing, we can CC again without an intervening GC.

You are quite right. Andrew walked through the various cases with me on IRC. What we really want is: always unmark gray things except when we are in certain special regions where we want to actually observe gray bits. Having a global "observing gray bits" flag will allow us to (1) remove all of the per-type "preserving color" custom accessors and (2) most of the existing ExposeToActiveJS. The new patch contains the same complete assertions and does not require any of the ~10 followup patches I had in my queue to add various missing ExposeToActiveJS.

I think I still need to rebase the weakmap and debugger changes forward, however, it should already be greenish on M(): https://tbpl.mozilla.org/?tree=Try&rev=4cfe7744d6a7
> except when we are in certain special regions where we want to actually observe gray bits

Or in situations where we know the object is not gray?  Unmarking gray bits is not that fast.  :(
We check if something is gray first, so hopefully this won't be /too/ bad. We can easily add a getKnownBlack or somesuch to get and assert the color on relevant hot paths.
Depends on: 1033020
Attachment #8441071 - Attachment is obsolete: true
Attachment #8449620 - Flags: feedback?(continuation)
Comment on attachment 8449620 [details] [diff] [review]
make_heapt_readbarriered-wip2.diff

As I said in IRC, I think disabling unmark gray in try hooks is better than this RAII thing, but I guess we'll see how that shakes out.
Attachment #8449620 - Flags: feedback?(continuation)
(In reply to Andrew McCreight [:mccr8] from comment #34)
> Comment on attachment 8449620 [details] [diff] [review]
> make_heapt_readbarriered-wip2.diff
> 
> As I said in IRC, I think disabling unmark gray in try hooks is better than
> this RAII thing, but I guess we'll see how that shakes out.

It is a ton of code because we also have to cover JS::Class::trace and JS::Class::finalize hooks. Also, there are a bunch of really complex corner cases in the wrapper cache and wrapper maps. However, once done, it will give us complete correctness assertions of all Heap<T> usage. This will make it almost impossible to leak something without triggering an assertion at the site of the leak. I think on balance this is probably worth the extra effort, so I will continue hacking in this direction.
I've had this essentially green on multiple occasions now only to be regressed by ongoing work. I think I'm going to have to land this in multiple pieces.
This implements the argument exposing strategy we discussed on IRC a couple weeks ago. This uses ExposeToActiveJS directly -- there are no Heap<T> for read barriers here -- so is orthogonal to the rest of the patch.
Attachment #8441085 - Attachment is obsolete: true
Attachment #8449620 - Attachment is obsolete: true
Attachment #8462651 - Flags: review?(bzbarsky)
Keywords: leave-open
https://tbpl.mozilla.org/?tree=Try&rev=322b899e1c0c

This patch does 3 things. (1) It automatically does UnmarkGray on gray things when reading from a ReadBarriered pointer. (2) It adds several missing ExposeToActiveJS found via the assertions I'll be adding later. (3) It renames needsBarrier to needsIncrementalBarrier, as it is going to be extremely confusing when we have 3 barriers and one of them is unlabeled.
Attachment #8462916 - Flags: review?(wmccloskey)
May be faster than explaining it all over IRC.
Attachment #8462918 - Flags: feedback?(wmccloskey)
Comment on attachment 8462651 [details] [diff] [review]
exposetoactivejs_on_bindings_args-v0.diff

So this will double-expose things like ImageData.data, in general.

Can we not avoid that somehow?  Do we have a concrete measurement for how long Expose*ToActiveJS takes, so we can decide how much of a problem doing it twice is in practice?

Would it make sense to define a new function which has two overloads: one that takes a Rooted<Value>& and does nothing and one that takes a Value and does JS::ExposeValueToActiveJS and then use it here?
Flags: needinfo?(terrence)
(In reply to Boris Zbarsky [:bz] from comment #40)
> Comment on attachment 8462651 [details] [diff] [review]
> exposetoactivejs_on_bindings_args-v0.diff
> 
> So this will double-expose things like ImageData.data, in general.

Ouch! I did not realize that both Rooted and raw Value flowed through there.

> Can we not avoid that somehow?  Do we have a concrete measurement for how
> long Expose*ToActiveJS takes, so we can decide how much of a problem doing
> it twice is in practice?

I'll try to get some numbers.

> Would it make sense to define a new function which has two overloads: one
> that takes a Rooted<Value>& and does nothing and one that takes a Value and
> does JS::ExposeValueToActiveJS and then use it here?

I'd be fine with that, but it seems a little fragile. Maybe there is a better way to do the codegen?
Flags: needinfo?(terrence)
We could try to punch through to this point why we're doing the conversion and condition on that, I guess...
Attached file etajs-assembly.txt
In a loop with a single JSObject* that is not in the nursery and is not grey and where the runtime has needsIncrementalBarrier set to false: I'm seeing 0.8ns per iteration.

The C++ loop is inline, and the timed portion of clang-3.4's output is attached. The assembly, while a bit opaque, does not appear to be doing anything substantially insane to get that performance.

BEGIN_TEST(testGCExposeToActiveJS)
{
    long count = 10000000000;
    JS::RootedObject child1(cx, JS_NewObject(cx, nullptr, JS::NullPtr(), JS::NullPtr()));
    JS_GC(rt);
    JSObject *obj = child1;
    uint64_t before = PRMJ_Now();
    for (long i = 0; i < count; ++i)
        JS::ExposeObjectToActiveJS(obj);
    uint64_t total = (PRMJ_Now() - before) * 1000;

    fprintf(stderr, "Took %ld ns for %ld iterations: %f ns-per\n", total, count, double(total) / double(count)); 
    return true;
}
END_TEST(testGCExposeToActiveJS)
Comment on attachment 8462651 [details] [diff] [review]
exposetoactivejs_on_bindings_args-v0.diff

We can probably just live with that much overhead.

Please remove the leading blank line from "head" in the isObject() and nullable() case.  r=me
Attachment #8462651 - Flags: review?(bzbarsky) → review+
Attachment #8462651 - Flags: checkin+
No movement on dromaeo dom or css, so I think we're good.
Neither of those tests would exercise this codepath, so no movement on them is expected.
Comment on attachment 8462916 [details] [diff] [review]
unmark_gray_on_readbarrier-v0.diff

I think I found a way to split this up without too much trouble.
Attachment #8462916 - Flags: review?(wmccloskey)
Splitting this up was successful. For my own future reference, I duped the patch in my queue, used vim to s/eedsIncrementalBarrier/eedsBarrier/g, making all the needsBarrier changes no-ops; qreffed to strip these, then pushed the dup, rejecting all the non-needsBarrier changes. Needed just a handful of manual fixups where the patches intersected.

The attached patch now only adds UnmarkGray to the read barrier. I'll open a non-sec bug for the trivial rename.
Attachment #8462916 - Attachment is obsolete: true
Attachment #8464359 - Flags: review?(wmccloskey)
Comment on attachment 8464359 [details] [diff] [review]
unmark_gray_on_readbarrier-v1.diff

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

Thanks. Sorry I've been so slow.

::: js/src/jsinfer.cpp
@@ +3912,5 @@
>          newTypeObjects.lookupForAdd(TypeObjectWithNewScriptSet::Lookup(clasp, proto, fun));
>      if (p) {
>          TypeObject *type = p->object;
> +        if (isJSContext())
> +            JS::ExposeGCThingToActiveJS(type, JSTRACE_TYPE_OBJECT);

p->object is a ReadBarrieredTypeObject. Why do you need this?

::: js/src/jsobjinlines.h
@@ +667,5 @@
>      return *compartment()->maybeGlobal();
>  }
>  
> +inline js::GlobalObject *
> +JSObject::unsafeUnbarrieredGlobal() const

I don't think we need this. The reason JSCompartment::global is read barriered is because someone might pull out the global when iterating over compartments or something. However, if we already have our hands on a JSObject (which is presumably black), then the global should be black to (since it's reachable from the object via parent pointers). Therefore, I think we can make JSObject::global() call JSCompartment::unsafeUnbarrieredGlobal() (with a comment). And then we won't need a special method on JSObject.

@@ +680,5 @@
> +     * isOwnGlobal is not a read, so avoid triggering the read barrier. Also,
> +     * it needs to be used from the trace hook, where entering the read barrier
> +     * would be incorrect.
> +     */
> +    return unsafeUnbarrieredGlobal() == this;

Can remove this too.

::: js/src/vm/SavedStacks.cpp
@@ +578,5 @@
>  JSObject *
>  SavedStacks::getOrCreateSavedFramePrototype(JSContext *cx)
>  {
> +    if (savedFrameProto) {
> +        JS::ExposeObjectToActiveJS(savedFrameProto);

I'd much prefer that SavedStacks::savedFrameProto just be a ReadBarriered.
Attachment #8464359 - Flags: review?(wmccloskey) → review+
Comment on attachment 8462918 [details] [diff] [review]
make_heapt_readbarrier-wip3.diff

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

Overall this looks like a good approach to me.
Attachment #8462918 - Flags: feedback?(wmccloskey) → feedback+
(In reply to Bill McCloskey (:billm) from comment #51)
> ::: js/src/jsinfer.cpp
> @@ +3912,5 @@
> >          newTypeObjects.lookupForAdd(TypeObjectWithNewScriptSet::Lookup(clasp, proto, fun));
> >      if (p) {
> >          TypeObject *type = p->object;
> > +        if (isJSContext())
> > +            JS::ExposeGCThingToActiveJS(type, JSTRACE_TYPE_OBJECT);
> 
> p->object is a ReadBarrieredTypeObject. Why do you need this?

Whoops! Looks like I forgot to remove this expose after making read-barriered unmark.

> ::: js/src/vm/SavedStacks.cpp
> @@ +578,5 @@
> >  JSObject *
> >  SavedStacks::getOrCreateSavedFramePrototype(JSContext *cx)
> >  {
> > +    if (savedFrameProto) {
> > +        JS::ExposeObjectToActiveJS(savedFrameProto);
> 
> I'd much prefer that SavedStacks::savedFrameProto just be a ReadBarriered.

Oh, good idea!
The try run looks more or less green:
https://tbpl.mozilla.org/?tree=Try&rev=e69802f1d252

This patch does more marking, so I'd expect failures to show up as leaks, OOMs, or timeouts. What failures there are on that try run look more normal than that, so I'm going ahead with the patch. I've already triggered a few rebuilds for the existing failures, just to be sure I'm exercising adequate paranoia here if m-i shows up more bustage.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2986d8d21bc5
Depends on: 1052386
https://hg.mozilla.org/mozilla-central/rev/2986d8d21bc5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Should this have had sec-approval first? Also, how far back does this affect?
Keywords: leave-open
Oh, hey, I thought we had decided on sec-medium for this. Yes, I should have requested approval. Not that it matters much in this case -- would be extremely hard to work out where the exact problems were from the patch.

Also, there's a ton more to land for this still.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Attachment #8464359 - Flags: checkin+
Yeah, sec-moderate is probably fine.  It would be extremely difficult to write an exploit based on this, I think.  It isn't really worth backporting any of this.
Comment on attachment 8464359 [details] [diff] [review]
unmark_gray_on_readbarrier-v1.diff

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It would be extremely difficult to craft an exploit with just the patch. It might give a hacker who is extremely knowledgeable in how our GC and CC interact some insight into what sort of problem it is; however, the solution covers all read-barriers, where only a couple actually demonstrate the problem. I don't think any of these are web exposed, even. 

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?
All of them.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No, we do not. It would be difficult, but possible, to backport, but there is a high risk that we would introduce leaks and crashes.

How likely is this patch to cause regressions; how much testing does it need?
Likely and considerable.
Attachment #8464359 - Flags: sec-approval?
Comment on attachment 8464359 [details] [diff] [review]
unmark_gray_on_readbarrier-v1.diff

mccr8 changed this to sec-moderate, so that obviates the need for sec-approval :)
Attachment #8464359 - Flags: sec-approval?
Now that the JS half has landed successfully, let's do a quick recap as we start into arc 2:

The general way grayness works is:
  1) GC runs, setting up our initial gray state.
  2) The mutator runs, potentially creating new edges to gray things.
  3) CC runs, using the gray bits to limit the number of objects it visits. Some of the C++ gray things will be unlinked, but the other gray things are still validly gray.
  4) The mutator runs some more, potentially creating new edges to gray things.
  5) Goto 1 or 3.

Summary:
  The mutator needs to UnmarkGrayGCThing (e.g. ExposeToActiveJS) anything it touches whereas the GC and CC need to read and write the gray bits freely. The GC and CC are (relatively) well constrained sub-systems and the mutator is everything else.

The Problem in this Bug:
  Forgetting to call ExposeToActiveJS while the mutator runs results in a sec-moderate UAF bug as well as general system instability. Moreover, accidentally calling ExposeToActiveJS while the CC or GC run results in a leak. Since ExposeToActiveJS is currently done more-or-less manually at every site that touches a GC pointer, we currently have tons of UAF and leaks, particularly in rarely used subsystems.

The Solution:
  Move the ExposeToActiveJS into a read-barrier on Heap<T>, making it automatic. Unfortunately, if this is done naively, we simply leak everything all the time because of the automatic black marking. Thus, we additionally have to add getPreservingColor for use from inside the CC and GC.

  Since the CC has many entry points, can call into JS, can call the GC directly, and is highly recursive and abstract in nature, deciding what is "in the CC" is actually quite a bit of a challenge. To address this, Andrew and I decided to add RAII guards that set a DEBUG-only bit in the runtime. This way the various getters can assert that they are always called in the right mode, and conversely, that we are in the right mode for the called code.

  With these assertions in place, we will have totally eliminated an entire class of UAF and leak bugs and, more importantly, will have a means of immediately detecting regressions in same.

-----

This patch adds the RAII guards JS::AutoObservingGrayBits and JS::AutoEnterJSUnderCC for use by the GC and CC. Subsequent patches will make use of these guards to enforce the various constraints detailed above. I will be adding these constraints one at a time, as trying to stabilize all of them at once has been a total, ongoing nightmare.

Jon, please review the JS bits. Andrew, please verify that these guard placements make sense -- note: I basically just placed them where the assertions told me they needed to go. I'd be happy to verify and/or collect stacks if any of the placements seem off to you.
Attachment #8471931 - Flags: review?(jcoppeard)
Attachment #8471931 - Flags: review?(continuation)
The CC's forget-skippable phase runs both under the CC and sometimes independently, so we have to lose a few of our guard assertions if we make the guards handle both cases. Luckily there is a better solution.

In bug 910937, Andrew and Ms2ger requested an explicit wrapper for this case, rather than a comment. Sadly, that ended up not happening, but if we add it now, we can just have both cases under AutoObservingGrayBits, clarify the code, and save ourselves a branch for good measure.
Attachment #8471951 - Flags: review?(jcoppeard)
Attachment #8471951 - Flags: review?(continuation)
Comment on attachment 8471931 [details] [diff] [review]
guard_gray_usage_regions-v0.diff

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

All looks good to me.
Attachment #8471931 - Flags: review?(jcoppeard) → review+
Attachment #8471951 - Flags: review?(jcoppeard) → review+
Comment on attachment 8471951 [details] [diff] [review]
unmark_for_cc-v0.diff

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

I never really liked this style of implicit black marking. ;)

r=me for the non-js/public/ stuff

::: js/xpconnect/src/nsXPConnect.cpp
@@ +281,5 @@
>  {
>      nsCOMPtr<nsIXPConnectWrappedJS> wjs = do_QueryInterface(aWrappedJS);
>      if (wjs) {
> +        JS::UnmarkGrayObjectForCC(
> +            static_cast<nsXPCWrappedJS*>(wjs.get())->GetJSObjectPreserveColor());

Please split the cast into a new declaration here.  You can call it rawWrappedJS or something.  This line is a bit ungainly now.
Attachment #8471951 - Flags: review?(continuation) → review+
Comment on attachment 8471931 [details] [diff] [review]
guard_gray_usage_regions-v0.diff

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

Thanks for doing all of this work!  I know it has been quite a slog to work through interactions with a ton of code you are unfamiliar with.  Let me know if you have any trouble with my suggested changes and I can walk you through it.

I have two major concerns I'd like to be addressed.  The first is that I think that destructors (and a few other things) need to be treated as mutator code, and should not observe gray.  The second is that I'd like the observe gray for BeginCollection to be much more fine grained.  I think I've outlined exactly what will work, so hopefully that won't be too much of a pain.

Also, please check that these patches don't wreck Dromeo etc. performance.  But I guess it looks like you are only making the slow path slower?  That should be ok.

::: content/media/webaudio/AudioBufferSourceNode.cpp
@@ +26,5 @@
>      // AudioNode's Unlink implementation disconnects us from the graph
>      // too, but we need to do this right here to make sure that
>      // UnregisterAudioBufferSourceNode can properly untangle us from
>      // the possibly connected PannerNodes.
> +    AutoSafeJSContext cx;

With my suggested changes, this and the other audio unlink changes shouldn't be needed.

::: dom/promise/Promise.cpp
@@ +1068,5 @@
>    // already), so don't init with it.
>    jsapi.Init();
>    JSContext* cx = jsapi.cx();
> +
> +  // If we are running under the CC, do required setup to run JS.

It seems odd that you need this here.  Is it called during Unlink or a destructor?

::: js/public/GCAPI.h
@@ +489,5 @@
>  /*
> + * Temporarily disable the automatic unmark gray that normally happens when
> + * reading a gray thing out of a Heap<T>.
> + */
> +class AutoObserveGrayBits

Maybe mark this as a stack-only class?

::: js/src/jspubtd.h
@@ +181,5 @@
> +     * Some operations, such as ForgetSkippable, may be triggered by CC
> +     * as well as under other conditions, so must be re-entrant, requiring
> +     * a counter.
> +     */
> +    unsigned observingGrayBits_;

Is the type "unsigned" really okay in JS land?  I thought something like uint32_t is more normal in Gecko code.  Also, I'd suggest giving up a bit and just using a signed type to make it more error resistant, though I guess you do assert in the one place you decrement it.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +732,5 @@
> +     * Any JS implemented observers running here need to receive implicit
> +     * unmark gray. C++ implemented observers will re-enter for tracing
> +     * as needed.
> +     */
> +    JS::AutoEnterJSUnderCC doNotObserveGray(Runtime());

With the finer-grained BeginCollection() gray entering, I think this isn't needed.

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +58,5 @@
>  }
>  
>  nsAutoCompleteController::~nsAutoCompleteController()
>  {
> +  mozilla::AutoSafeJSContext cx;

With my suggested changes to make us not observe gray when calling dtors, I think this isn't needed any more.

::: xpcom/base/nsCycleCollector.cpp
@@ +331,5 @@
>  };
>  #endif
>  
> +// Initialize a JS::ObservingGrayBits RAII guard if we have a JSRuntime.
> +class AutoObserveGrayBitsOnRuntime

Please mark this as a stack only class.

@@ +334,5 @@
> +// Initialize a JS::ObservingGrayBits RAII guard if we have a JSRuntime.
> +class AutoObserveGrayBitsOnRuntime
> +{
> +public:
> +  AutoObserveGrayBitsOnRuntime(CycleCollectedJSRuntime *aRuntime) {

* with the type.

@@ +2624,5 @@
>    }
>  
>    ~SnowWhiteKiller()
>    {
> +    AutoObserveGrayBitsOnRuntime observeGray(mCollector->mJSRuntime);

This would be better if we could observeGray for the Trace() but not for the DeleteCycleCollectable.  The former does really need to look at gray bits, and the latter is calling destructors, which do all sorts of weird stuff, so it would be nice to split.  It seems like the perf overhead of rapidly entering and exiting observeGray isn't so bad?  (If it is, this could be changed to iterate over the purple buffer twice, once with and one without gray observing.)

@@ +3254,5 @@
>  
>    whiteNodes.SetCapacity(mWhiteNodeCount);
>    uint32_t numWhiteGCed = 0;
>  
> +  AutoObserveGrayBitsOnRuntime observeGray(mJSRuntime);

Does CollectWhite really need to observe gray?  As you've found out, destroying things is going to run all sorts of ridiculous random code, so it seems like it would be better to not have it observing gray.  The CC itself doesn't need to as far as I can recall.

@@ +3722,5 @@
>    MOZ_ASSERT(mIncrementalPhase == IdlePhase);
>  
>    mCollectionStart = TimeStamp::Now();
>  
> +  AutoObserveGrayBitsOnRuntime observeGray(mJSRuntime);

In BeginCollection(), this could be made finer grained.

- I think BeginCycleCollectionCallback() only looks at gray bits in nsCCUncollectableMarker::Observe, so you could add an AutoObserveGray thing in there.
- Setting up the CC logger shouldn't look at any gray bits.
- For FixGrayBits, you could add an AutoObserveGray thing to CycleCollectedJSRuntime::FixWeakMappingGrayBits().
- FreeSnowWhite should be covered by ~SnowWhiteKiller.
- mListener->Begin(), mGraph.Init() and mResults.Init() shouldn't look at gray JS.
- ShouldMergeZones() calls CycleCollectedJSRuntime::UsefulToMergeZones(), so that could get an AutoObserveGray thing added to it.
- new CCGraphBuilder shouldn't look at gray JS.
- For mJSRuntime->TraverseRoots, CycleCollectedJSRuntime::TraverseRoots needs to observe gray, so you could move it into there.
- nsPurpleBuffer::SelectPointers needs to observe gray, so you can add something to that method.

With those changes, I think you should be able to remove the observe gray from BeginCollection.  As you can see, BeginCollection() is a big pile of random junk, so I would prefer that the places that need to observe gray are the ones that actually enter the observe gray section.  Plus arbitrary code can register itself as an observer for the begin-cycle-collection event, which does not make me feel good about it.
Attachment #8471931 - Flags: review?(continuation) → review-
Makes sense, I'll post stacks as I run into problem cases.
Promises MaybeReportRejected is called from both Unlink and the destructor.
We don't need to use the more restrictive component-specific security groups unless a bug is sec-high or sec-critical
Group: javascript-core-security → core-security
Why is this marked to leave open? Is this fixed at this point?
Flags: needinfo?(terrence)
It isn't fixed yet.  There were just some preliminary patches that have landed.
Flags: needinfo?(terrence)
We probably shouldn't have the 'status-firefox34' set to 'fixed' then.
(In reply to Andrew McCreight [:mccr8] from comment #70)
> It isn't fixed yet.  There were just some preliminary patches that have
> landed.

Actually, the patches here do fix all the real outstanding issues that I uncovered while working on the larger issue: barriering these edges to make mistakes impossible. Since the actual exploitable bugs have been plugged and uplifted, I think we probably should actually close this and work on barriers in a non-sec bug. Does that make sense, Andrew?
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: needinfo?(continuation)
Resolution: --- → FIXED
(In reply to Terrence Cole [:terrence] from comment #72)
> Actually, the patches here do fix all the real outstanding issues that I
> uncovered while working on the larger issue: barriering these edges to make
> mistakes impossible. Since the actual exploitable bugs have been plugged and
> uplifted, I think we probably should actually close this and work on
> barriers in a non-sec bug. Does that make sense, Andrew?

Ah, ok, I didn't realize you'd gotten things to the point where you were confident we'd fixed the exiting issues.  Closing this bug and filing a new one sounds like the right idea then.
Flags: needinfo?(continuation)
This was addressed around 2.2 CC, but the bug is changing status after that. For now I am assuming that the 2.2 branch isn't affected due to the preliminary patches that already landed. Please update the status-b2g-v2.2 flag accordingly if I am mistaken.
Flags: needinfo?(terrence)
I meant 2.1 CC, of course. m(
Both b2g 2.1 and 2.2 have all the patches in this bug.
Flags: needinfo?(terrence)
Group: core-security → core-security-release
Group: core-security-release
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.