WeakMaps with keys from another compartment are possible and incorrect

RESOLVED FIXED in Firefox 17

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

(Blocks 1 bug, {csectype-uaf, sec-critical})

unspecified
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15 wontfix, firefox16 wontfix, firefox17+ fixed, firefox18+ fixed, firefox19+ fixed, firefox-esr10 unaffected, firefox-esr1720+ fixed)

Details

(Whiteboard: [adv-track-main17+])

Attachments

(4 attachments, 6 obsolete attachments)

18.90 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
11.01 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
5.22 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
4.34 KB, patch
billm
: review+
Details | Diff | Splinter Review
This is a regression from bug 673468. Somehow I convinced myself this was okay, but I'm now pretty sure that we can have problems from this. I'll try to get a testcase on Monday.
I figured out how to make this crash. The cross-compartment key needs to be in its own chunk, which is then deallocated. That way we crash when trying to check the mark bit when sweeping the weakmap.

var w = new WeakMap();
var g = newGlobal();
var k = g.eval('for (var i=0;i<100000;i++) new Object(); var q = new Object(); dumpObject(q); q');
dumpObject(k);
w.set(k, {});
k = null;

print('first gc...');
gc();
gc();
g.eval('q = null');
print('comp gc');
gc(g);
gc(g);
gc(g);
gc(g);
gc(g);
gc(g);
print('last gc');
gc();
Here's what's going on. Let's say we have a weakmap M in compartment CM. Then we have an object K in compartment CK. We have a wrapper for K in compartment CM called KW. Now we use KW as a key in M.

Currently, we will unwrap KW to get K before adding it to the weakmap. Now let's say we lose the reference we had to KW and do a GC. That will cause KW to be finalized, so its entry in the cross-compartment map will go away. Then let's say we do a compartment GC of compartment CK. It's possible that K will be collected by this GC because the weakmap has no way to keep it alive. However, K will still be an entry in the weakmap and it won't be swept, since CM is not being collected.

Now we have a weakmap with a ptr to K even though K has been finalized. The test I posted earlier makes this crash by ensuring that K gets allocated in its own chunk and then ensuring that that chunk is freed. Later, if we GC compartment CM, we'll crash when sweeping M because we need to check the mark bit of K, and its chunk has been freed. I'm guessing that the difficulty of generating this crash is why we never found this before.

This patch doesn't fix anything. It just makes it easier to find such crashes by adding an assertion that we never check the mark bit of an object that's in an arena that's been released. The tricky part here is that we *do* check the mark bits of shapes after their arena has been finalized, and it's safe to do so. So I added an isMarkedUnchecked version to use in such cases.
Attachment #669354 - Flags: review?(jcoppeard)
Posted patch bugfix patch (obsolete) — Splinter Review
This fixes the problem by avoiding the unwrapping and changing how weakmap marking works. I still need to do the CC work, but that will be a separate patch.

Bobby, I'm asking for your feedback because I added a new ProxyHandler hook. The context for this bug is in bug 673468. I'm just fixing it in a different way.

I'm a little worried how things will work when people start using the new DOM proxies as keys. They're not IndirectProxyHandlers and I don't know whether they support the preserve wrapper mechanism. I guess we'll have to figure that out later.
Attachment #669364 - Flags: review?(continuation)
Attachment #669364 - Flags: feedback?(bobbyholley+bmo)
Posted patch bugfix patch, v2 (obsolete) — Splinter Review
Sorry, I forgot to qref.
Attachment #669364 - Attachment is obsolete: true
Attachment #669364 - Flags: review?(continuation)
Attachment #669364 - Flags: feedback?(bobbyholley+bmo)
Attachment #669365 - Flags: review?(continuation)
Attachment #669365 - Flags: feedback?(bobbyholley+bmo)
The new DOM binding stuff, if that's what you are referring to, involves nsWrapperCache stuff, so it should support wrapper preservation.
Comment on attachment 669365 [details] [diff] [review]
bugfix patch, v2


>+JSObject *
>+BaseProxyHandler::weakmapKeyDelegate(JSTracer *trc, JSObject *proxy)

Please add a nice big fat comment somewhere explaining what this trap is all about.


>+static JSObject *
>+proxy_WeakmapKeyDelegate(JSTracer *trc, RawObject obj)
>+{
>+    JS_ASSERT(obj->isProxy());
>+    if (!obj->getSlot(JSSLOT_PROXY_HANDLER).isUndefined())
>+        return GetProxyHandler(obj)->weakmapKeyDelegate(trc, obj);

When can a proxy ever have an undefined handler?

f=bholley with those changes
Attachment #669365 - Flags: feedback?(bobbyholley+bmo) → feedback+
Comment on attachment 669354 [details] [diff] [review]
add assertion to catch this in the future

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

The current Cell::isMarked, and the new isMarkedUnchecked, call AssertValidColor, which winds up calling ArenaHeader::allocated() for non-black colours only.

So for gray marking only, isMarked is currently checked and isMarkedUnchecked would actually check too.  Kind of messy, although I can't immediately see a way to fix it.

Maybe AssertValidColor could be different - in practice, are there any objects that don't support gray marking?
Comment on attachment 669365 [details] [diff] [review]
bugfix patch, v2

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

This is sort of amusing because you are essentially implementing wrapper preservation for JS proxies. I wonder if this is more generally useful or not.

Are there other places that need their Class updated? js::NormalArgumentsObjectClass, js::StrictArgumentsObjectClass, js::ArrayClass, js::SlowArrayClass, IMPL_TYPED_ARRAY_FAST_CLASS don't use JS_NULL_CLASS_EXT, though maybe they should.  XPC_WN_NoHelper_JSClass also looks like it might need updating.

Maybe there are some others.  You can use this search to find them:
http://mxr.mozilla.org/mozilla-central/search?string=isWrappedNative&filter=[Ii]sWrappedNative

r- mainly because of the Class stuff.  I'm not exactly sure how that should work...

Nice patch, though. Did you come across crashes like this, or did you have an epiphany?

::: js/src/jsclass.h
@@ +255,5 @@
>       * WeakMaps use this to override the wrapper disposal optimization.
>       */
>      bool                isWrappedNative;
> +
> +    JSWeakmayKeyDelegateOp weakmapKeyDelegateOp;

Like bholley said, this really needs a comment, as it isn't really obvious what this is for just from its name.

::: js/src/jsproxy.cpp
@@ +353,5 @@
>  {
>  }
>  
> +JSObject *
> +BaseProxyHandler::weakmapKeyDelegate(JSTracer *trc, JSObject *proxy)

Why is it okay to just return nothing here?  Does the base proxy not wrap anything?

@@ +355,5 @@
>  
> +JSObject *
> +BaseProxyHandler::weakmapKeyDelegate(JSTracer *trc, JSObject *proxy)
> +{
> +    return false;

null, not false

@@ +556,5 @@
>  
> +JSObject *
> +IndirectProxyHandler::weakmapKeyDelegate(JSTracer *trc, JSObject *proxy)
> +{
> +    return GetProxyTargetObject(proxy);

Do you not have to worry about the target being another proxy?

@@ +2869,5 @@
> +{
> +    JS_ASSERT(obj->isProxy());
> +    if (!obj->getSlot(JSSLOT_PROXY_HANDLER).isUndefined())
> +        return GetProxyHandler(obj)->weakmapKeyDelegate(trc, obj);
> +    return NULL;

maybe reverse these branches?  eg if isUndefined(), early return with NULL.  Just to remove a negation. If this is just the way it is done, then that's okay.

::: js/src/jsproxy.h
@@ +120,5 @@
>      virtual bool regexp_toShared(JSContext *cx, JSObject *proxy, RegExpGuard *g);
>      virtual bool defaultValue(JSContext *cx, JSObject *obj, JSType hint, Value *vp);
>      virtual bool iteratorNext(JSContext *cx, JSObject *proxy, Value *vp);
>      virtual void finalize(JSFreeOp *fop, JSObject *proxy);
> +    virtual JSObject *weakmapKeyDelegate(JSTracer *trc, JSObject *proxy);

Should all of these delegate functions use JSRawObject as args and returns to match the type in JSWeakmayKeyDelegateOp?

::: js/src/jsweakmap.h
@@ +169,5 @@
>          for (Range r = Base::all(); !r.empty(); r.popFront())
>              markValue(trc, &r.front().value);
>      }
>  
> +    bool keyNeedsMark(JSTracer *trc, JSObject *key) {

Should trc have type GCMarker*? You should at least assert IS_GC_MARKER(trc) somewhere.

@@ +172,5 @@
>  
> +    bool keyNeedsMark(JSTracer *trc, JSObject *key) {
> +        if (JSWeakmayKeyDelegateOp op = key->getClass()->ext.weakmapKeyDelegateOp) {
> +            JSObject *delegate = op(trc, key);
> +            if (!delegate)

I don't know if early return is better here or not.  It is a shame we don't have a better way to code this particular pattern for these various callbacks.

@@ +195,5 @@
>                      markedAny = true;
>                  if (prior != e.front().key)
>                      e.rekeyFront(e.front().key);
> +            } else if (IS_GC_MARKING_TRACER(trc) && keyNeedsMark(trc, e.front().key)) {
> +                gc::Mark(trc, const_cast<Key *>(&e.front().key), "WeakMap key");

a more specific description than "WeakMap key" would be nice.  Maybe something like "proxy-preserved WeakMap key"?
Attachment #669365 - Flags: review?(continuation) → review-
Posted patch assertion, v2 (obsolete) — Splinter Review
This moves around some of the assertions to avoid duplication.

The reason AssertValidColor was conditional was because of the shape issue mentioned earlier. However, now that we have isMarkedUnchecked for the shape case, AssertValidColor can check the color unconditionally.

As an aside, the reason we have AssertValidColor is because we only have room in the mark bitmap for two colors. There's one bit in the bitmap for every 8 bytes of GC thing. On 32-bit machines, the smallest object is 16 bytes, so there are only two bits available for it. AssertValidColor is just checking that we never use 2 or above as colors.
Attachment #669354 - Attachment is obsolete: true
Attachment #669354 - Flags: review?(jcoppeard)
Attachment #670616 - Flags: review?(jcoppeard)
Posted patch bugfix patch, v3 (obsolete) — Splinter Review
> When can a proxy ever have an undefined handler?

You're right, it can't. I cargo-culted that from the finalize hook. I've removed the possibility in both cases. I imagine it was to deal with a partially-created proxy object, but that can't happen.

> Are there other places that need their Class updated?
> js::NormalArgumentsObjectClass, js::StrictArgumentsObjectClass, js::ArrayClass,
> js::SlowArrayClass, IMPL_TYPED_ARRAY_FAST_CLASS don't use JS_NULL_CLASS_EXT, though
> maybe they should.  XPC_WN_NoHelper_JSClass also looks like it might need updating.

In C, if you leave out a field of a global struct definition, it will be filled in with NULL. And when we dynamically allocate classes, we always memset them to zero. So in don't think there's anything to worry about here.

> Why is it okay to just return nothing here?  Does the base proxy not wrap anything?

Yeah, base proxies don't wrap anything as far as I know. The only proxies we have that aren't IndirectProxyHandlers are from the new DOM bindings. We'll need to come up with a way to handle those, but it sounds like wrapper preservation will work. (Note that even before my patch, I don't think we handle the new DOM bindings.)

> Do you not have to worry about the target being another proxy?

Good point. I switched it to an UnwrapObject call.
Attachment #669365 - Attachment is obsolete: true
Attachment #670643 - Flags: review?(continuation)
Comment on attachment 670643 [details] [diff] [review]
bugfix patch, v3

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

Looks good!
Attachment #670643 - Flags: review?(continuation) → review+
Blocks: 790338
Comment on attachment 670643 [details] [diff] [review]
bugfix patch, v3

Did you r+ the wrong patch by mistake, Jon?
Attachment #670643 - Flags: review?(continuation)
Comment on attachment 670643 [details] [diff] [review]
bugfix patch, v3

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

> In C, if you leave out a field of a global struct definition, it will be filled in with NULL.

Thanks, good to know. I would have thought initializing things without your explicit declaration would be a little too hand-holdey for C. I mean, think of all the cycles you could potentially be wasting if there are fields you know don't need to be initialized!

Thanks for the tests.

::: js/xpconnect/tests/chrome/test_weakmap_keys_preserved.xul
@@ +28,5 @@
> +  let map = sandbox.WeakMap();
> +  let obj = {};
> +  map.set(obj, {});
> +
> +  window.QueryInterface(Ci.nsIInterfaceRequestor)

I think you can just do Cu.forceGC() here instead of this QIing fun.

@@ +32,5 @@
> +  window.QueryInterface(Ci.nsIInterfaceRequestor)
> +        .getInterface(Ci.nsIDOMWindowUtils)
> +        .garbageCollect();
> +
> +  is(map.has(obj), true, "Weakmap still contains our wrapper!");

You can do ok(..., "foo") instead of is(..., true, "foo")
Attachment #670643 - Flags: review?(continuation) → review+
Comment on attachment 670643 [details] [diff] [review]
bugfix patch, v3

[Security approval request comment]
How easily can the security issue be deduced from the patch?
The test makes it fairly clear, but I could land without that. Otherwise it looks like a refactoring.

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

Which older supported branches are affected by this flaw?
It goes back to FF 13.

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

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

How likely is this patch to cause regressions; how much testing does it need?
There is a small possibility of regressions if the patch is incorrect. Most likely, it would manifest itself in a way where things were unexpectedly dropped from weakmaps.
Attachment #670643 - Flags: sec-approval?
Comment on attachment 670643 [details] [diff] [review]
bugfix patch, v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 673468
User impact if declined: Crashes are unlikely from the issue, but it could be exploited.
Testing completed (on m-c, etc.): Tryserver
Risk to taking this patch (and alternatives if risky): See previous comment.
String or UUID changes made by this patch: None.
Attachment #670643 - Flags: approval-mozilla-beta?
Attachment #670643 - Flags: approval-mozilla-aurora?
Comment on attachment 670643 [details] [diff] [review]
bugfix patch, v3

Sec-approval+ for landing without the test. We should have a separate patch for that and land the test once we release with the fix.
Attachment #670643 - Flags: sec-approval? → sec-approval+
Posted patch test for this bug (obsolete) — Splinter Review
I folded the tests in with the assertion patch. We can land them both later. I'm going to assume that Jon meant to r+ this patch rather than the other one.
Attachment #670616 - Attachment is obsolete: true
Attachment #670616 - Flags: review?(jcoppeard)
Attachment #670892 - Flags: review+
You aren't going to land this until you fix the CC stuff, right?
(In reply to Andrew McCreight [:mccr8] from comment #18)
> You aren't going to land this until you fix the CC stuff, right?

Waiting on this before approving for branches (don't know how critical that change is).
Yeah, it isn't actually ready to land anywhere yet.
(In reply to Andrew McCreight [:mccr8] from comment #20)
> Yeah, it isn't actually ready to land anywhere yet.

Where should we be tracking progress on that so we know when to expect this bug to be ready to go?
I expect the rest will be done in a day or two.
(In reply to Bill McCloskey (:billm) from comment #22)
> I expect the rest will be done in a day or two.

Sorry, I should have been more specific - are there bugs tracking what needs to be resolved before this can land?  Or we are we just waiting for a comment in this bug saying "all clear"?
I'm planning to post another patch in this bug, get it reviewed, ask for approval for that as well, and then everything can land I guess.
Comment on attachment 670643 [details] [diff] [review]
bugfix patch, v3

The approval flags can probably be cleared for now to avoid coming up in queries...
Attachment #670643 - Flags: approval-mozilla-beta?
Attachment #670643 - Flags: approval-mozilla-aurora?
While debugging the CC patch, I ended up making some changes to this one, so asking for another review. The changes are as follows:

1. Remove the JSTracer* param from the weakmap key delegate hook, since the tracer is unable to supply one and I wasn't using it anyway.

2. Previously, when doing weakmap marking, if we found a delegate for a key, we would check delegate->isMarked(gcmarker->getMarkColor()). If that was true, we would mark the key and the value. However, this caused problems if the weakmap was gray and the delegate was black. In that case, we wouldn't call markIteratively on the weakmap until the mark color was gray. And then delegate->isMarked(GRAY) would be false, so we wouldn't do anything. This is wrong. I fixed it by changing it to IsMarked, which just checks if it's black. I think this is correct.

3. I removed some comments about weakmaps that no longer apply.
Attachment #670643 - Attachment is obsolete: true
Attachment #671624 - Flags: review?(continuation)
This adds support to the cycle collector for these delegates. It's pretty straightforward.
Attachment #671625 - Flags: review?(continuation)
Here's the gross test for the previous patch. I had to add support to Cu.nondeterministicGetWeakMapKeys for weakmaps in other compartments.
Attachment #671626 - Flags: review?(continuation)
Blocks: 801516
Comment on attachment 671624 [details] [diff] [review]
bugfix patch, v4

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

> And then delegate->isMarked(GRAY) would be false, so we wouldn't do anything.

Wow, nice catch! I didn't think about that at all, but your fix sounds right to me. This stuff is so tricky. Could you please throw a brief comment into keyNeedsMark about this?  Something along the lines of "Check if the delegate is marked with any color to properly handle gray marking when the key's delegate is black and the map is gray".  This is subtle enough that I could see people looking at the code later and thinking IsObjectMarked is a mistake, or maybe just not being able to figure out why it is what it is.

::: js/src/jsweakmap.h
@@ +142,5 @@
> +        if (JSWeakmapKeyDelegateOp op = key->getClass()->ext.weakmapKeyDelegateOp) {
> +            JSObject *delegate = op(key);
> +            if (!delegate)
> +                return false;
> +            return gc::IsObjectMarked(&delegate);

You could change this to |return delegate && gc::IsObjectMarked(...)|

@@ +147,5 @@
> +        }
> +        return false;
> +    }
> +
> +    bool keyNeedsMark(JSTracer *trc, gc::Cell *cell) {

This function appears to be unused.
Attachment #671624 - Flags: review?(continuation) → review+
Comment on attachment 671625 [details] [diff] [review]
bugfix for CC, v1

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

Looks good!

::: content/base/src/nsContentUtils.cpp
@@ +6373,5 @@
>    NS_IMETHOD_(void) NoteNextEdgeName(const char* name)
>    {
>    }
>  
> +  NS_IMETHOD_(void) NoteWeakMapping(void* map, void* key, void* kdelegate, void* val)

Ugh, sorry about this.  I finally filed bug 802604 for making the base traverse class empty so we don't need to keep patching these empty methods.

::: xpcom/base/nsCycleCollector.cpp
@@ +1907,5 @@
>  
>      WeakMapping *mapping = mWeakMaps.AppendElement();
>      mapping->mMap = map ? AddWeakMapNode(map) : nullptr;
>      mapping->mKey = key ? AddWeakMapNode(key) : nullptr;
> +    mapping->mKeyDelegate = kdelegate ? AddWeakMapNode(kdelegate) : mapping->mKey;

Ah, nice. I was thinking offhand the else case here would be nullptr, but you are right this has to be handled like this.

@@ +2143,5 @@
>              // All non-null weak mapping maps, keys and values are
>              // roots (in the sense of WalkFromRoots) in the cycle
>              // collector graph, and thus should have been colored
>              // either black or white in ScanRoots().
> +            MOZ_ASSERT(mColor != grey, "Uncolored weak map");

Thanks for changing these to MOZ_ASSERT!

@@ +2145,5 @@
>              // collector graph, and thus should have been colored
>              // either black or white in ScanRoots().
> +            MOZ_ASSERT(mColor != grey, "Uncolored weak map");
> +            MOZ_ASSERT(kColor != grey, "Uncolored weak map key");
> +            MOZ_ASSERT(kdColor != grey, "Uncolored weak map key");

nit: "key delegate" in here instead of "key".

@@ +2148,5 @@
> +            MOZ_ASSERT(kColor != grey, "Uncolored weak map key");
> +            MOZ_ASSERT(kdColor != grey, "Uncolored weak map key");
> +            MOZ_ASSERT(v->mColor != grey, "Uncolored weak map value");
> +
> +            if (mColor == black && kColor != black && kdColor == black) {

Oh, this is tricky. I was thinking the |mColor == black| part was wrong, but then I realized that in the GC we never run the special weak map key code unless the map is alive, so you are right.

A test case would be something hideous like:
(1) the key delegate is gray and alive
(2) the key somehow reaches the map without passing through its delegate, and nothing refers to it
(3) the map is gray, and part of a garbage cycle passing through DOM.

I'm not sure (2) is actually possible right now, as I have the vague impression that anything with a delegate only has its wrappee as an outgoing edge. So we don't really need to try to construct a test case for this.

But anyways, if you could set that up, then the GC would trace from a gray root to the map, triggering the weak map key marking code, which would end up marking the key gray.

During cycle collection, the key delegate would get marked black. Then, in my erroneous version of this code, during weak map marking we'd end up marking the key black, which would cause the map to get marked black.  The GC and CC would agree that the map is alive, so I don't think we would have anything dangerous, but we would end up leaking the weak map, and anything it held alive.

Oh, maybe I thought of a way to construct condition (2). Can a proxy be the delegate of another proxy?  I guess not, because you fully unwrap to compute the delegate. Never mind then.

Anyways, in summary, I'm glad you wrote this part and not me.
Attachment #671625 - Flags: review?(continuation) → review+
Comment on attachment 671626 [details] [diff] [review]
test for CC patch

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

::: js/src/jsweakmap.cpp
@@ +282,5 @@
>      ObjectValueMap *map = GetObjectMap(obj);
>      if (map) {
>          for (ObjectValueMap::Base::Range r = map->all(); !r.empty(); r.popFront()) {
> +            RootedObject key(cx, r.front().key);
> +            if (!JS_WrapObject(cx, key.address()))

I assume wrap is smart enough to not wrap if it is same compartment?

::: js/xpconnect/tests/chrome/test_weakmap_keys_preserved2.xul
@@ +23,5 @@
> +  let Cu = Components.utils;
> +  let Ci = Components.interfaces;
> +
> +  window.QueryInterface(Ci.nsIInterfaceRequestor)
> +       .getInterface(Ci.nsIDOMWindowUtils)

nit: I think you can do SpecialPowers.DOMWindowUtils.garbageCollect().

@@ +31,5 @@
> +    return document.getElementById("testelem");
> +  };
> +
> +  let wrappers_as_keys_test = function () {
> +    let e = document.createEvent("MessageEvent");

Please add a comment here that you are creating an unoptimizable edge from DOM to JS, to create a live gray JS object.

@@ +57,5 @@
> +    live_dom.xyzabc = {wrappee: wrappee, m: map, sb: sandbox};
> +
> +    let key = Cu.nondeterministicGetWeakMapKeys(map)[0];
> +    let value = map.get(key);
> +    is(value.children.length, 1, "children have wrong lengthA");

nit: "lengthA"

@@ +67,5 @@
> +    let live_dom = window.eeeevent.data.dummy;
> +    let live_map = live_dom.xyzabc.m;
> +    let sandbox = live_dom.xyzabc.sb;
> +    is(Cu.nondeterministicGetWeakMapKeys(live_map).length, 1,
> +      "Map should not be empty.");    

nit: trailing whitespace

@@ +73,5 @@
> +    let value = live_map.get(key);
> +    is(value.children.length, 1, "children have wrong length");
> +  }
> +
> +  dump('collecting...\n');

nit: Do you mean for these two |dump|s to be here?

@@ +85,5 @@
> +        .getInterface(Ci.nsIDOMWindowUtils)
> +        .garbageCollect();
> +  dump('cc done...\n');
> +
> +  check_wrappers_as_keys();

So this fails without your CC patch and passes with it?  It is interesting that you don't need precise GC stuff.  I wonder if Luke's scoping work has made stuff hang around less.

@@ +87,5 @@
> +  dump('cc done...\n');
> +
> +  check_wrappers_as_keys();
> +
> +  ok(true, "");

nit: Does this |ok| do anything?
Attachment #671626 - Flags: review?(continuation) → review+
Updated test, which needs to be landed when this bug opens.
Attachment #670892 - Attachment is obsolete: true
Attachment #672628 - Flags: review+
Flags: in-testsuite?
Comment on attachment 671625 [details] [diff] [review]
bugfix for CC, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 673468
User impact if declined: Crashes are unlikely from the issue, but it could be exploited.
Testing completed (on m-c, etc.): Tryserver
Risk to taking this patch (and alternatives if risky): See previous comment.
String or UUID changes made by this patch: None.
Attachment #671625 - Flags: approval-mozilla-beta?
Attachment #671625 - Flags: approval-mozilla-aurora?
Comment on attachment 672628 [details] [diff] [review]
test for this bug, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 673468
User impact if declined: Crashes are unlikely from the issue, but it could be exploited.
Testing completed (on m-c, etc.): Tryserver
Risk to taking this patch (and alternatives if risky): See comment 14.
String or UUID changes made by this patch: None.
Attachment #672628 - Flags: approval-mozilla-beta?
Attachment #672628 - Flags: approval-mozilla-aurora?
Please flag the patch for sec-approval? and fill out the comment form so that we can make sure we're aligned with the security group in timing the landing here.
(In reply to Lukas Blakk [:lsblakk] from comment #37)
> Please flag the patch for sec-approval? and fill out the comment form so
> that we can make sure we're aligned with the security group in timing the
> landing here.

I did that in comments 14 and 16. I revised the patch after that, but it seems like nothing materially changed that would affect the approval. Should I have done something differently? What are these approvals based on, anyway?
Comment on attachment 671625 [details] [diff] [review]
bugfix for CC, v1

No, you did it right - thanks for pointing out that you already got the + (it wasn't visible to me as it was on obsolete patch).  That approval means we're OK to land to branches, so it's good enough for me - approving uplift.
Attachment #671625 - Flags: approval-mozilla-beta?
Attachment #671625 - Flags: approval-mozilla-beta+
Attachment #671625 - Flags: approval-mozilla-aurora?
Attachment #671625 - Flags: approval-mozilla-aurora+
That said - did the patch you got sec-approval+ on include the test?  I know we sometimes hold back on checking in tests - can someone confirm if it's ok to land this bug's test so early?
Attachment #672628 - Flags: approval-mozilla-beta?
Attachment #672628 - Flags: approval-mozilla-aurora?
Comment on attachment 671624 [details] [diff] [review]
bugfix patch, v4

Sorry, I asked for approval on the wrong patch. This is the one that needs to go to branches.
Attachment #671624 - Flags: approval-mozilla-beta?
Attachment #671624 - Flags: approval-mozilla-aurora?
Comment on attachment 671624 [details] [diff] [review]
bugfix patch, v4

Approved.  Also, disregard my statements about sec-approval - I was misunderstanding the process, it's for timing landings of sec fixes on trunk, not on branches.
Attachment #671624 - Flags: approval-mozilla-beta?
Attachment #671624 - Flags: approval-mozilla-beta+
Attachment #671624 - Flags: approval-mozilla-aurora?
Attachment #671624 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-track-main17+]
Depends on: 812380
Depends on: 819131
This doesn't appear to have made it into ESR17 yet. Is B2G also affected?
It was apparently fixed on 18, so it should be fixed on b2g18, too.
Thanks Andrew. ni? on billm for to get an uplift request to mozilla-esr17.
Flags: needinfo?(wmccloskey)
It was fixed on 17, so it's in esr17. I verified that it's in the esr17 repo.
Flags: needinfo?(wmccloskey)
Group: core-security
Bill, could you look into landing the tests for this bug?  I just noticed they hadn't landed.
Flags: needinfo?(wmccloskey)
Tests for this bug:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdf89c928543
Flags: needinfo?(wmccloskey)
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.