x = (function() {}) __proto__ = x Object.freeze(wrap(x)) arguments asserts js debug shell on m-c changeset 82df3654cd80 with --no-baseline --ion-offthread-compile=off --ion-eager at Assertion failure: shape->hasSlot(), at vm/Interpreter-inl.h and crashes js opt shell at js::FetchName My configure flags are: (opt) CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --disable-debug --enable-optimize --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe Debug: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/e0741f7815ff user: Jason Orendorff date: Fri Apr 25 16:11:01 2014 -0500 summary: Bug 547140, part 2 - Remove flags argument from JS_GetPropertyDescriptor and friends. r=Waldo. Setting s-s and sec-critical as a start because this seems to access a weird memory address 0x109722000 in the opt shell. Jason, is bug 547140 likely related?

[Tracking Requested - why for this release]:

Reproduces readily. I think the right fix here is to make Object::sealOrFreeze use the new (JSPROP_IGNORE_VALUE | JSPROP_IGNORE_READONLY | JSPROP_IGNORE_ENUMERATE) stuff in bug 1037770. I'll wait until that lands.

Bug 1037770 is now fixed, so setting needinfo? from :jorendorff. :)

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 64c4ec2df3d4).

JSBugMon: Fix Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first good revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/9f2e00997d30 user: Jeff Walden date: Fri Aug 01 19:11:22 2014 -0700 summary: Bug 969478 - Make the arguments/caller properties of functions be entirely implemented by accessors living on Function.prototype. r=jorendorff This iteration took 351.774 seconds to run.

Waldo, is bug 969478 a probable fix?

Yes, Waldo's patch did fix the symptom here, by making a change to function.arguments specifically. Object.freeze's behavior here is not good, though, and efaust is working on fixing it.

It would fix the assertion when hit that way, yes. But bug 969478 is still semi-experimental, so I wouldn't rely on it as a fix at this point just yet. And in any event, that bug just changed away from broken old JSPropertyOp to a different mechanism. The broken old mechnaism is still going to be broken, if it's used anywhere. Which it is. js> var x = function() { return arguments; }(); __proto__ = x; Object.freeze(wrap(x)); length Assertion failure: shape->hasSlot(), at /home/jwalden/moz/aurora/js/src/vm/Interpreter-inl.h:195 Program received signal SIGSEGV, Segmentation fault. js::FetchName<false> (cx=0x1c69960, obj=(JSObject * const) 0x7fffee73e060 [object global] delegate, obj2=(JSObject * const) 0x7fffee750580 [object Arguments], name="length", shape=0x7fffee753538, vp=$jsval((JSObject *) 0x7fffee750580 [object Arguments])) at /home/jwalden/moz/aurora/js/src/vm/Interpreter-inl.h:195 195 JS_ASSERT(shape->hasSlot()); efaust and I are conversing over IRC about this As We Speak (he said, literally, redundantly).

var x = function() { return arguments; }(); __proto__ = x; Object.freeze(wrap(x)); length Assertion failure: shape->hasSlot(), at /Users/skywalker/trees/mozilla-central/js/src/vm/Interpreter-inl.h:195 Tested on m-c rev dc352a7bf234. (To update JSBugMon)

So, the problem is that when we do the Proxy::getOwnPropertyDescriptor in proxy_SetGenericAttributes, we lose the property op getter/setter that was in the shape. Since we don't want to expose the property ops in general from js::GetOwnPropertyDescriptor for native objects, and the fact that we are doing a get and a define is fundamentally kludge of the proxy code, we just "cheat" by forcing the define to just twiddle the attribute bits. This crash only affects properties with property ops accessors that are different from the class "defaults" (getProperty and setProperty hook). The last test that gary posted is an example of such a property that remains even after Waldo's patch. I am open to other avenues of fix, but this seems the most reasonable to me. I could maybe be convinced that we should push the JSPROP_IGNORE_VALUE out further, but this will certainly do.

Comment on attachment 8479484 [details] [diff] [review] Proposed fix Review of attachment 8479484 [details] [diff] [review]: ----------------------------------------------------------------- Clearing - we discussed on IRC and decided on a different approach.

4-line comment makes me sad, it should be 1-line but i don't have the right words

Comment on attachment 8480841 [details] [diff] [review] bug-1042567-freeze-v1.patch Review of attachment 8480841 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me. f? Waldo to make sure he's aware, as he's the defacto representative of all these property-op implemented data properties.

Comment on attachment 8480841 [details] [diff] [review] bug-1042567-freeze-v1.patch Review of attachment 8480841 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Eric Faust [:efaust] from comment #13) > he's the defacto representative of all these property-op implemented > data properties. I will not accept if nominated and will not serve if elected. I reject this false smear! :-P ::: js/src/jit-test/tests/basic/bug1042567.js @@ +1,4 @@ > +var x = function() { return arguments; }(); > +__proto__ = x; > +Object.freeze(wrap(x)); > +length; Kinda leery about landing a test in advance of fixing everywhere, we're off the rails in ways none of us fully understand (or should take the time to truly, fully understand), so caution is a virtue. I'd hold off on the landing til this has been in all releases for a couple few weeks at least.

Jason, any idea what the next steps are here?

This didn't land because the new assertions turned some things orange on the try server. I don't remember what, though, so I'm running it again. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a86bba0f3593

Hmm. Well, I tried a few things, and I don't think that particular assertion can't be salvaged in the short term. Try run with just the fix and the other (weaker) assertion looks good: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c198b9b5bd52 I'll land it today.

https://hg.mozilla.org/mozilla-central/rev/dbc722859c16 This didn't make the uplift to Aurora today. You'll need to land there on your own.

Comment on attachment 8480841 [details] [diff] [review] bug-1042567-freeze-v1.patch Approval Request Comment [Feature/regressing bug #]: bug 547140 [User impact if declined]: possibly exploitable crash in opt builds, assertions in debug builds that point to the problem [Describe test coverage new/current, TBPL]: no test coverage (will land tests in m-c after some time passes) [Risks and why]: May introduce an unknown bug or two, but that's better than leaving in a known and dangerous bug. (Why: touching this kind of code is always at least a moderate risk because nobody understands the full ramifications of the JS property attribute state space. The situation sucks and I'm actively working to simplify in other bugs.) :-\ [String/UUID change made/needed]: none

No sec-approval?

[Tracking Requested - why for this release]:

https://hg.mozilla.org/releases/mozilla-aurora/rev/da7bdb2b6ee3 This still needs a sec-approval nomination as well as beta/esr31 approval requests.

Comment on attachment 8480841 [details] [diff] [review] bug-1042567-freeze-v1.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? It contains a test which is somewhat non-sensical, but did create some problems for the engine. It's not clear if it was weaponizable, but it's a little scary. 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? Now beta and esr21. If not all supported branches, which bug introduced the flaw? Unknown. It's been a long time coming Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This patch should backport. How likely is this patch to cause regressions; how much testing does it need? Unfortunately, it was landed already, and has been riding the train for most of the last cycle, having just uplifted to aurora. This is mostly for sanity reasons and tracking purposes.

Comment on attachment 8480841 [details] [diff] [review] bug-1042567-freeze-v1.patch Ooops (as to landing). Do we need this ported to Beta then since it goes back forever? Can you nominate this for ESR31 as well?

JSBugMon: This bug has been automatically verified fixed. JSBugMon: This bug has been automatically verified fixed on Fx35

Eric, can you please nominate this for beta/esr31 approval?

Firefox 34 is marked as affected. Looks like a Beta uplift request is required as well.

Comment on attachment 8480841 [details] [diff] [review] bug-1042567-freeze-v1.patch [Approval Request Comment] Approval Request Comment [Feature/regressing bug #]: 547140 [User impact if declined]: Potential for internal state corruption in JS engine from relatively simple requests. Probably potentially exploitable. [Describe test coverage new/current, TBPL]: Landed on truck for the last cycle. Test also included in patch [Risks and why]: Patch is low risk. Adds some new asserts and plugs the hole. [String/UUID change made/needed]: N/A This had aurora support, but didn't make it before an uplift, so is just an extension of the previous request.

Comment on attachment 8480841 [details] [diff] [review] bug-1042567-freeze-v1.patch Beta+ ESR31+ (thanks RyanVM for the move from ESR24 -> ESR31 as ESR24 is already EOL)

https://hg.mozilla.org/releases/mozilla-beta/rev/043c91e3aaeb https://hg.mozilla.org/releases/mozilla-esr31/rev/77b6294a019e https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/43bee45176c4 https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/1bde54b2e7b0

JSBugMon: This bug has been automatically verified fixed on Fx34

Had to back this out from every branch covered by comment 32 for debug test failures with the assert shown in the log below. https://treeherder.mozilla.org/ui/logviewer.html#?job_id=114341&repo=mozilla-beta https://hg.mozilla.org/releases/mozilla-beta/rev/15bafc2978d8 https://hg.mozilla.org/releases/mozilla-esr31/rev/84f81600288d https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/ab9e1685d537 https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/599f2ea93566

OK, so it looks like this landed without 1065604, which that assertion relies upon. We can either remove the assertion or just also backport that bug. Waldo, you did the reviews here. Thoughts?

If a backport of bug 1065604 passes try trivially, it shouldn't hurt to backport that, I think. There's a chance older code is forgetting JSPROP_SHARED somewhere, tho -- if it turns out there's much/any of that, that the already-written patch affects, I would probably just remove the assertion. *twitch*

https://www.google.com/search?client=ubuntu&channel=fs&q=1065604&ie=utf-8&oe=utf-8 This seems to indicate that a BP of 1065604 is sufficient to clean things up, and it applied cleanly from the patch in that bug. I will nominate it for uplift then, I guess.

JSBugMon: This bug has been automatically verified fixed on Fx34

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/d38dd7de64b3 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/5c8e19357e57 https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/167f314a8c07