897678 - (CVE-2013-5602) ASAN SEGV on unknown address in Worker::SetEventListener

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nils]

Attachment: testcase (crashes firefox)

The attached testcase crashes the ASAN build of Firefox with the attached testcase.

It involves event handlers and exchange of __proto__ attributes, similar to Bug 831090.

Comment 1

[Nils]

Attachment: ASAN stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

This looks like exactly the issue in bug 831090, but with a testcase that reproduces in a current build.

Jeff, now you should be able to reproduce....

Comment 3

[Jesse Ruderman]

Attachment: cleaner testcase (crashes Firefox)

Comment 4

[Jesse Ruderman]

Assertion failure: JSID_IS_INT(aIdval), at dom/workers/Worker.cpp:206

Comment 5

[Jesse Ruderman]

I see why I missed this.  I filed bug 899046.

Nils, how did your fuzzer get past bug 899046?  Did you teach it specifically about workers?

Comment 6

[Nils]

Jesse, yes it knows about workers as part of one of it's modules.

Comment 7

[Jeff Walden [:Waldo]]

(gdb) bt
#0  0x00007ffff1382879 in (anonymous namespace)::Worker::SetEventListener (
    aCx=0x7fffd98dfe70, aObj=(JSObject * const) 0x7fffe48f9800 [object Text], 
    aIdval=$jsid("onmessage"), aStrict=0, aVp=JSVAL_NULL)
    at /home/jwalden/moz/slots/dom/workers/Worker.cpp:207
#1  0x00007ffff35ee7c0 in js::CallJSPropertyOpSetter (cx=0x7fffd98dfe70, 
    op=0x7ffff1382805 <(anonymous namespace)::Worker::SetEventListener(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JSBool, JS::MutableHandle<JS::Value>)>, 
    obj=(JSObject * const) 0x7fffe48f9800 [object Text], id=$jsid("onmessage"), strict=0, 
    vp=JSVAL_NULL) at /home/jwalden/moz/slots/js/src/jscntxtinlines.h:309
#2  0x00007ffff35ee983 in js::CallSetter (cx=0x7fffd98dfe70, 
    obj=(JSObject * const) 0x7fffe48f9800 [object Text], id=$jsid("onmessage"), 
    op=0x7ffff1382805 <(anonymous namespace)::Worker::SetEventListener(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JSBool, JS::MutableHandle<JS::Value>)>, attrs=65, shortid=0, 
    strict=0, vp=JSVAL_NULL) at /home/jwalden/moz/slots/js/src/jscntxtinlines.h:335
#3  0x00007ffff36027bf in js::baseops::SetPropertyHelper (cx=0x7fffd98dfe70, 
    obj=(JSObject * const) 0x7fffe48f9800 [object Text], 
    receiver=(JSObject * const) 0x7fffe48f9800 [object Text], id=$jsid("onmessage"), 
    defineHow=0, vp=JSVAL_NULL, strict=0) at /home/jwalden/moz/slots/js/src/jsobj.cpp:4558
#4  0x00007ffff33693be in JSObject::setGeneric (cx=0x7fffd98dfe70, 
    obj=(JSObject * const) 0x7fffe48f9800 [object Text], 
    receiver=(JSObject * const) 0x7fffe48f9800 [object Text], id=$jsid("onmessage"), 
    vp=JSVAL_NULL, strict=0) at /home/jwalden/moz/slots/js/src/jsobj.h:868
#5  0x00007ffff3630e8b in js::DirectProxyHandler::set (
    this=0x7ffff66766e0 <js::CrossCompartmentWrapper::singleton>, cx=0x7fffd98dfe70, 
    proxy=(JSObject * const) 0x7fffe48f9840 [object Proxy], 
    receiver=(JSObject * const) 0x7fffe48f9800 [object Text], id=$jsid("onmessage"), 
    strict=false, vp=JSVAL_NULL) at /home/jwalden/moz/slots/js/src/jsproxy.cpp:576
#6  0x00007ffff3698109 in js::CrossCompartmentWrapper::set (
    this=0x7ffff66766e0 <js::CrossCompartmentWrapper::singleton>, cx=0x7fffd98dfe70, 
    wrapper=(JSObject * const) 0x7fffe48f9840 [object Proxy], 
    receiver=(JSObject * const) 0x7fffe48f9840 [object Proxy], id=$jsid("onmessage"), 
    strict=false, vp=JSVAL_NULL) at /home/jwalden/moz/slots/js/src/jswrapper.cpp:314
#7  0x00007ffff363cd77 in js::Proxy::set (cx=0x7fffd98dfe70, 
    proxy=(JSObject * const) 0x7fffe48f9840 [object Proxy], 
    receiver=(JSObject * const) 0x7fffe48f9840 [object Proxy], id=$jsid("onmessage"), 
    strict=false, vp=JSVAL_NULL) at /home/jwalden/moz/slots/js/src/jsproxy.cpp:2553
#8  0x00007ffff363e479 in proxy_SetGeneric (cx=0x7fffd98dfe70, 
    obj=(JSObject * const) 0x7fffe48f9840 [object Proxy], id=$jsid("onmessage"), 
    vp=JSVAL_NULL, strict=0) at /home/jwalden/moz/slots/js/src/jsproxy.cpp:2871
#9  0x00007ffff35f76b6 in JSObject::nonNativeSetProperty (cx=0x7fffd98dfe70, 
    obj=(JSObject * const) 0x7fffe48f9840 [object Proxy], id=$jsid("onmessage"), 
    vp=JSVAL_NULL, strict=0) at /home/jwalden/moz/slots/js/src/jsobj.cpp:1742
#10 0x00007ffff3369391 in JSObject::setGeneric (cx=0x7fffd98dfe70, 
    obj=(JSObject * const) 0x7fffe48f9840 [object Proxy], 
    receiver=(JSObject * const) 0x7fffe48f9840 [object Proxy], id=$jsid("onmessage"), 
    vp=JSVAL_NULL, strict=0) at /home/jwalden/moz/slots/js/src/jsobj.h:867

The basic problem is in this code in js::baseops::SetPropertyHelper:

    RootedObject pobj(cx);
    RootedShape shape(cx);
    if (!LookupPropertyWithFlags(cx, obj, id, cx->resolveFlags, &pobj, &shape))
        return false;
    if (shape) {
        if (!pobj->isNative()) {
            if (pobj->is<ProxyObject>()) {
                AutoPropertyDescriptorRooter pd(cx);
                if (!Proxy::getPropertyDescriptor(cx, pobj, id, &pd, JSRESOLVE_ASSIGNING))
                    return false;

                if ((pd.attrs & (JSPROP_SHARED | JSPROP_SHADOWABLE)) == JSPROP_SHARED) {
                    return !pd.setter ||
                           CallSetter(cx, receiver, id, pd.setter, pd.attrs, pd.shortid, strict,
                                      vp);
                }

We look up the property on |frameText| from the testcase.  That's a proxy, so we go through proxy handling and look it up on the underlying Text object.

Looking up on that, we recur into |w.__proto__|, which is from the original page (not the iframe page) and so is a proxy/wrapper.  Lookups on proxies return |pobj == the proxy object| and |shape| is set to a bogus, non-null value by |MarkNonNativePropertyFound|.

So we have |shape|, |!pobj->isNative()|, and |pobj->is<ProxyObject>()|.  We descend into the block containing the getPropertyDescriptor and such.  The returned descriptor includes the actual JSStrictPropertyOp setter to be called.

JSStrictPropertyOp has a couple requirements.  You have to pass in |shape->userid()| as id, not whatever was used for the lookup, so that the op gets what it expects (the shortid, not the id used for the lookup).  That's the assert being hit here.  In theory, if |pd.attrs & JSPROP_SHORTID|, |pd.shortid| will be used by |CallSetter| and all will be well.  But 1) |pd.attrs == 0x41| and |JSPROP_SHORTID == 0x100| so no match, and 2) |pd.shortid == 0| -- and the underlying shape's propid_ is "onmessage", despite having setter be Worker::SetEventListener.  I'm not sure why that is, exactly -- conceivably that's the bug, maybe?  That said, if memory serves, propertyop getters/setters can't assume they're passed an object of the kind they expect -- so GetEventListener/SetEventListener may need fixing in that case even if this (tiny|short)id nonsense needs fixing.

More investigation needed, not clearing needinfo yet.

Comment 8

[Boris Zbarsky [:bzbarsky]]

No matter what we do on the JS engine side here, we should just stop using propertyops (with shortid at that!) on the worker side, imo...

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That means converting everything to WebIDL and/or ditching the event implementation?  We plan to do all of that.

If it means something more complex than that then ...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Well, converting to WebIDL will certainly do that.  The question is whether there's a simpler switch to JSNative getters/setters we can do before then, and whether it's worthwhile.

Comment 11

[Jeff Walden [:Waldo]]

Agreed about not using propertyops -- that holds everywhere, not just here.  I also think we shouldn't be using shortids/tinyids, which would fall out of not using propertyops here, but which could be done independent of that (although I'm not sure it'd fix this bug, exactly).  Exactly what set of changes will fix this, as minimally as possible, remains unclear.  Ergo, more investigation.

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Possible patch

Building this now to test.  Written against a slightly-oldish tree, so probably already needs rebasing due to include/JSBool/etc. refactoring work.  Good times.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Convert onerror/onmessage to accessors from propertyops

Okay, this really seems to work.  (Without crashing on shutdown because I called |worker->SetEventListener("onmessage", listener, rv)| rather than |worker->SetEventListener("message", listener, rv)|.  Ugh.  That should *really* be fixed so you're not specifying a string you could get wrong, via typos or whatever, at some point.)

The original code shouldn't have used propertyops and tinyids, but I'm not actually sure the JS engine's doing nothing wrong with this.  I'll probably try to investigate more before declaring this patch a complete success/solution (to all possible instances of the problem, not just this specific one, if the ultimate issue is not specific to this worker code).  But it seems worth getting the review-clock going while that happens.  (Even if this can't land for a good long while because of uplift-cycle timing.  Sigh.)

Comment 14

[Jeff Walden [:Waldo]]

I considered investigating further to figure out whether the JS engine's at fault here or not.  But it seems like it might be easier to search for all instances of PropertyOp/StrictPropertyOp properties and audit them:

http://mxr.mozilla.org/mozilla-central/search?string=JSOP_WRAPPER

What makes this relatively simple -- assuming I understand the uses sufficiently, and I need people to double-check me on this -- is that not all instances are actually exposed to untrusted code.  Looking down the list of files in question:

js/src/ctypes/CTypes.cpp: trusted, not exposed to web -- no changes needed

js/src/perf/jsperf.cpp: trusted, not exposed to web -- no changes needed

js/src/builtin/RegExp.cpp: none of the instances here touch |obj| at all -- they go straight to RegExpStatics -- so no changes needed
js/src/shell/js.cpp: trusted, not exposed to web -- no changes needed

dom/workers/WorkerScope.cpp:
dom/workers/Exceptions.cpp:
dom/workers/File.cpp:
dom/workers/Events.cpp:
These are untrusted.  But they *only* implement fields visible within workers.  Workers can never observe a main-thread global object.  Is there any way for them to observe some other global object, to trigger this cross-compartment issue?  If there isn't, I don't think these need any changes.

There's one other possibility for the dom/workers/ hits -- that they can be triggered on an object using something like |new Proxy(relevantObjectInsideWorker, {})| and an access of one of the properties at issue.  That needs investigation.

So I think there's only the direct-proxy issue to investigate, to determine that the fix posted here, is adequate to address every instance of the problem in Gecko -- regardless whether SpiderMonkey is or is not doing the right thing here.  I'll want a double-check on this logic from some other JS engine person, of course, before declaring any sort of victory on this front, tho.  :-)

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 787864 [details] [diff] [review]
Convert onerror/onmessage to accessors from propertyops

Blake has kindly offered to review this for me.

Comment 16

[Andrew McCreight [:mccr8]]

Marking based on the flags in bug 831090.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> if memory serves, propertyop getters/setters can't assume they're passed an
> object of the kind they expect -- so GetEventListener/SetEventListener may
> need fixing in that case even if this (tiny|short)id nonsense needs fixing.

Both the getter and setter here check that the class of the passed-in object is the expected class, so they should be fine.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> I considered investigating further to figure out whether the JS engine's at
> fault here or not. 

The JS engine is definitely at fault here. More in a bit...

> js/src/shell/js.cpp: trusted, not exposed to web -- no changes needed

For what it's worth, this is a bit of a touchy subject. At least one distro offers the JS shell as a package for running JS. I don't know if that's still the case, though.

> There's one other possibility for the dom/workers/ hits -- that they can be
> triggered on an object using something like |new
> Proxy(relevantObjectInsideWorker, {})| and an access of one of the
> properties at issue.  That needs investigation.

This definitely needs to be tested, but from my understanding of the problem here, these cases almost certainly need to be fixed. Also, the code at http://mxr.mozilla.org/mozilla-central/source/gfx/skia/src/xml/SkJSDisplayable.cpp#183 might be affected by this.

> So I think there's only the direct-proxy issue to investigate, to determine
> that the fix posted here, is adequate to address every instance of the
> problem in Gecko -- regardless whether SpiderMonkey is or is not doing the
> right thing here.  I'll want a double-check on this logic from some other JS

I'm wary of this approach, in part because it makes the SpiderMonkey API even harder to reason about than it already is. I'm all for simplifying the worker code to not use tinyids anymore, but doing only Worker.cpp seems incomplete and prone to error.

There are a couple of problems that lead to the assertion here: firstly, whether or not a js::Shape has a tinyid is not stored in its attrs but rather in its flags (!). This is why the attrs in comment 7 didn't include JSPROP_SHORTID. Furthermore, js::DirectProxyHandler::getPropertyDescriptor uses jsapi.cpp:GetPropertyDescriptorById, which doesn't copy the tinyid from the shape *and* doesn't copy the information about the tinyid out of the shape's attrs anyway. Fixing that might be simple (and possibly enough) except that reading through JS_DefineProperties, I think every property defined ends up with Shape::HAS_SHORTID, so we'd probably want to fix that as well (unless I'm misreading the code). 

I also find it kind of weird that we have two entirely different ways of calling setters: js::Shape::set and js::CallSetter. It was only through reading both of those methods that I noticed the attrs/flags distinction.

So I'd rather either ween workers off of tinyids entirely or just fix the immediate problems with tinyids here and be done with it.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 787864 [details] [diff] [review]
Convert onerror/onmessage to accessors from propertyops

Clearing request based on comment 17.

Comment 19

[Jeff Walden [:Waldo]]

I'll wean them off tinyids entirely, then -- should be fairly quick, I think.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Please at least file a followup on fixing tinyids or nuking them entirely. I don't fully understand the reluctance to fix the tinyid bug here, though.

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Fix all the workers code using propertyops

The GetterOnlyJSNative bit (a native mimicking the behavior of js_GetterOnlyPropertyStub) is partly paranoia, partly motivated by existing tests that think that setting some of these properties *should* throw errors even outside strict mode code.  We can remove it on trunk, but it'll require fixing a bunch of tests.  Not something to do now, for something we'd want to backport.

I forgot that Property<SLOT_loaded>::Get will work even with old gcc, where GetProperty<SLOT_loaded> won't.  So that at least lets me get rid of N code-different function definitions to placate the JS_PSG macro-calls in gcc 4.4.

I originally did all the functions without JS::CallNonGenericMethod.  But doing so happened to trigger some fairly obscure "foo called on incompatible Proxy" errors in our own test suite.  Given that, it seemed fairly risky to only spot-fix the ones needed to make mochitests pass, when it might break sites during a backport.  (The old PropertyOp code didn't need this non-generic thing, because property ops semi-automatically get the unwrapped-y thing passed into them, not the proxy object.)

Double-checks of the IsFoo static methods are probably the most important thing -- if those over-accept stuff that's the easiest way for anything here to go off the rails.

The JS_GetConstructor usage is hackitudinous, but given the precise scoping of it to immediately after the JS_InitClass, it shouldn't be able to hit anything dodgy.

The WorkerScope.cpp getters technically should accept primitive |this| values, and work out the global object themselves in that case.  But we're "saved" here because SpiderMonkey wrongly boxes primitives up into objects for implicit calls to accessor functions -- bug 603201, bug 869098 -- so these don't need to do extra work now to work correctly.  When those bugs are fixed, we'll have to circle back and fix this code accordingly.

This is a large-ish patch, but it's mostly pretty simple.  I have more faith in its being correct, than I do in my understanding the existing interactions of tinyids in the JS engine, and "fixing" how tinyids work wrt proxies and proxied gets and stuff.

This passes try:

https://tbpl.mozilla.org/?tree=Try&rev=c107d0fd95ec
https://tbpl.mozilla.org/?tree=Try&rev=bef3335c33c5

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 796399 [details] [diff] [review]
Fix all the workers code using propertyops

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

Please file followups on getting rid of propertyops in the other places.

Comment 23

[Jeff Walden [:Waldo]]

I'm working in branch backports now.  Enough has changed at the periphery that blindly importing the patch here triggers a bunch of rejects, so I'm trying to get the branch-targeted work done before moving ahead on trunk.

Comment 24

[Jeff Walden [:Waldo]]

At a glance, the vast majority of the differences are the result of s/JSBool/bool/g on trunk.  Converting the few JSBool to bool in context, then reapplying the patch so the unchanged parts are picked up, appears as if it'll get the job done (of course, with another post-step afterward to revert bool->JSBool again).

Comment 25

[Jeff Walden [:Waldo]]

Attachment: Potential aurora backport, building locally now, off to try when that completes

Comment 26

[Jeff Walden [:Waldo]]

Attachment: Potential beta backport (the aurora patch with two conflicts fixed)

Comment 27

[Jeff Walden [:Waldo]]

Attachment: Real aurora backport

The various Is* methods needed to accept const Value&, not a handle.  Also maybe a typo or two beyond that.

Comment 28

[Jeff Walden [:Waldo]]

Attachment: Real beta backport

Same extra set of fixes, plus some tweaks so that two uses of DOMException will work.  (BindingUtils.h gets added to Exceptions.cpp, but that pulls in domstubs.h, which forward-declares a DOMException class.  Then the two DOMException::* calls near the end of that file are ambiguous between the anonymous-namespaced DOMException in the file, and the global forward-declaration of DOMException.  I added anonymous-namespaced forwarding methods to resolve the ambiguity.  Bleh.  No idea why this issue doesn't exist on alpha/trunk, no real point finding out, either.)

The aurora backport patch built locally.  The beta patch has built past dom/workers now.  So these are probably good to go, but I'm going to finish this build and try them before declaring success.

Comment 29

[Jeff Walden [:Waldo]]

Aurora: https://tbpl.mozilla.org/?tree=Try&rev=3558b49106b8
Beta: https://tbpl.mozilla.org/?tree=Try&rev=b900dba68f86

Comment 30

[Jeff Walden [:Waldo]]

...and even still those aren't quite entirely right -- beta needs MOZ_STATIC_ASSERT still -- but it's close enough to declare success.  Requesting approvals momentarily...

Comment 31

[Jeff Walden [:Waldo]]

Attachment: Backported patch from bug 908898 for aurora

The trunk patch depends on bug 908898, so that has to be backported as well.  It's a much simpler backport than the main patch here did.

Comment 32

[Jeff Walden [:Waldo]]

Attachment: Backported patch for aurora

Comment 33

[Jeff Walden [:Waldo]]

Attachment: Backported patch from bug 908898 for beta

Comment 34

[Jeff Walden [:Waldo]]

Attachment: Backported patch for beta

Comment 35

[Jeff Walden [:Waldo]]

Comment on attachment 796399 [details] [diff] [review]
Fix all the workers code using propertyops

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not super-easily.  There's trickery to getting pointers lined up "just so" to avoid crashing, then to avoid it in usefully-exploitable ways.  But this might be something heap-spraying would eat for breakfast, at cursory thought: I've never done the exercise of writing an exploit with invalid heap-memory use as vector, so I don't know for sure.

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

Which older supported branches are affected by this flaw?
All of them.

If not all supported branches, which bug introduced the flaw?
Probably dates back to the use of tinyids in this stuff in the first place.  I suspect tinyids have been busted in the through-a-proxy case ever since proxies were introduced.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Aurora/beta backports are finished.  I don't have a b2g18 backport or an esr17 backport yet.  None of this code has meaningfully changed in some time, but the JSAPI has changed substantially at the surface level -- JSBool->bool, adding handles to things, and so on -- such that creating backport patches involves a fair bit of tedium to apply the trunk patch's changes to older branches.  Risk of such changes is pretty low, tho -- if they pass mochitests, they should be good.

How likely is this patch to cause regressions; how much testing does it need?
Just for sheer quantity of changes, probably the branch-targeted backports want more than the two weeks of testing they'd get if landed now.  :-\  I think, given there's no hint of the issue in the patch, we might land this September 17 (just after uplift) on trunk, then on branches a week or so later, for settling time.  But up to you if you think it's worth pushing on this sooner.

Comment 36

[Matt Wobensmith [:mwobensmith][:matt:]]

Al is PTO. In his absence, perhaps Dan can approve.

Comment 37

[Daniel Veditz [:dveditz]]

I do appreciate the work to create the branch patches proactively, but that's a much bigger chunk of change than we're comfortable taking on beta this close to release. And thankfully the patch is opaque so we're not forced to take it by landing on M-C.

We will want esr-17 fixes for 25 (don't land until after the merge). Not sure about b2g18--this won't make the 1.1 release and I think 1.2 is based on a further branch point, but we'll almost certainly have at least one 1.1 update worth of fixes (1.1.1?) before 1.2

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 796399 [details] [diff] [review]
Fix all the workers code using propertyops

sec-approval+

Comment 39

[Jeff Walden [:Waldo]]

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

I filed bug 914433 on removing tinyids completely.

Comment 40

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/mozilla-central/rev/afa6f48ffe9c

Comment 41

[Alex Keybl [:akeybl]]

Hey Waldo, any reason to wait till later in Beta 25 to uplift?

Comment 42

[Jeff Walden [:Waldo]]

I think we can land this early without issues.

Comment 44

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on FF26, 2013-08-25.
Verified fix on FF26, 2013-09-22.

Comment 45

[Jeff Walden [:Waldo]]

Attachment: Bug 908898 patch, for current beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: unclear, hard to say how far the issue here can be levered; sec-critical at worst
Testing completed (on m-c, etc.): landed on m-c, uplifted to aurora
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 46

[Jeff Walden [:Waldo]]

Attachment: Fix all the workers code using propertyops, backported to current beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): probably dates to initial DOM workers code landing or so
User impact if declined: unclear, hard to say how far the issue here can be levered; sec-critical at worst
Testing completed (on m-c, etc.): landed on m-c, uplifted to aurora
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

A note: tryservering these patches doesn't work -- I think because try-infrastructure isn't geared to build as-old-as-beta code.  :-\

https://tbpl.mozilla.org/?tree=Try&rev=20cbf831f091

I'm doing a local build now as the second-best double-check.

Comment 47

[Alex Keybl [:akeybl]]

Comment on attachment 814401 [details] [diff] [review]
Bug 908898 patch, for current beta

Low risk sec-crit fix. Approving for Beta 25.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0592732e6f9a

Is this going to need esr17/b2g18 uplift still?

Comment 49

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

Yes, we need this in ESR24 and ESR17, as well as B2G.

Comment 50

[Naveed Ihsanullah [:naveed]]

Jeff is working on the backporting patches now.

Comment 51

[Jeff Walden [:Waldo]]

esr17-targeted: https://tbpl.mozilla.org/?tree=Try&rev=cfa8f5c81988
b2g18-targeted: https://tbpl.mozilla.org/?tree=Try&rev=c8669b9ad988
esr24-targeted: https://tbpl.mozilla.org/?tree=Try&rev=4fa0796171be

Odds on those actually working on modern try infrastructure, anyone?  ;-)  (Last I suspect works, the other two seem doubtful to me.)

Comment 52

[Andrew McCreight [:mccr8]]

Yeah, it looks like only esr24 even sort of worked.  The unfortunate reality of landing on old branches is that you have to use the branch itself as try server...

Comment 53

[Jeff Walden [:Waldo]]

Attachment: esr17-targeted patch

This builds locally and passes dom/workers mochitests.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined: sec-critical
Fix Landed on Version: in everything bug esr{17,24} and b2g18 now, incl. aurora/beta
Risk to taking this patch (and alternatives if risky): patch size not ideal, but safer than trying to change JSPropertyOp calling code, I think
String or UUID changes made by this patch: none

Comment 54

[Jeff Walden [:Waldo]]

Attachment: esr24-targeted patch

This patch passed try, so one better than esr17.  :-)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined: sec-critical
Fix Landed on Version: everywhere except esr{17,24} and b2g18 now
Risk to taking this patch (and alternatives if risky): see above
String or UUID changes made by this patch: none

Comment 55

[Jeff Walden [:Waldo]]

Attachment: b2g18-targeted patch

Still building/running mochitests, but worth getting out there now for approval pending those results.  I'm building the b2g18 tree, but honestly at this point I don't know if I'm supposed to be working with that tree or some v1.blah.blah tree -- clarification of where this should land there would be helpful.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): there forever
User impact if declined: sec-critical
Testing completed: will run dom/workers mochitests locally before landing
Risk to taking this patch (and alternatives if risky): big but relatively straightforward, better than the alternative of changing fundamental property-access code in SpiderMonkey
String or UUID changes made by this patch: none

Oh, to be clear: I'm only posting one patch each for approvals, but I'll be landing backports of bug 908898 (which are relatively trivial to construct) as well as these patches (which require those backports).

Comment 56

[Jeff Walden [:Waldo]]

esr17:

https://hg.mozilla.org/releases/mozilla-esr17/rev/c1d8ffd42618
https://hg.mozilla.org/releases/mozilla-esr17/rev/53a6b94f2d6f

Comment 57

[Jeff Walden [:Waldo]]

The b2g18 patches are golden on dom/workers mochitests.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #55)
> Still building/running mochitests, but worth getting out there now for
> approval pending those results.  I'm building the b2g18 tree, but honestly
> at this point I don't know if I'm supposed to be working with that tree or
> some v1.blah.blah tree -- clarification of where this should land there
> would be helpful.

b2g18 is the correct (and only) tree for this to be landing on. Thanks for asking.

Comment 59

[Jeff Walden [:Waldo]]

b2g18, as RyanVM tells me there's a blanket approval to land there given the tracking flags being set to 25+:

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8ab36ae7129b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e4cb5a852e3d

Will keep an eye on it to make sure it greenifies correctly.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/53542d252c2d
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/e4cb5a852e3d

Comment 61

[bhavana bajaj [:bajaj]]

Comment on attachment 818908 [details] [diff] [review]
b2g18-targeted patch

already uplifted ! had spoken offline with RyanVM to uplift any b2g18 affected.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/a3719ea29154
https://hg.mozilla.org/releases/mozilla-esr24/rev/fb01012aee26

Comment 63

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed in 24esr, 25, ASan builds from 2013-10-21.

Comment 64

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified 17esr, 2013-10-23.