1042567 - Crash [@ js::FetchName] or Assertion failure: shape->hasSlot(), at vm/Interpreter-inl.h

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: debug and opt stacks

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?

Comment 1

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

[Tracking Requested - why for this release]:

Comment 2

[Jason Orendorff [:jorendorff]]

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.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 4

[Christian Holler (:decoder)]

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

Comment 5

[Christian Holler (:decoder)]

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.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Waldo, is bug 969478 a probable fix?

Comment 7

[Jason Orendorff [:jorendorff]]

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.

Comment 8

[Jeff Walden [:Waldo]]

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).

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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)

Comment 10

[Eric Faust [:efaust]]

Attachment: Proposed fix

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 11

[Jason Orendorff [:jorendorff]]

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.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: bug-1042567-freeze-v1.patch

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

Comment 13

[Eric Faust [:efaust]]

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 14

[Jeff Walden [:Waldo]]

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.

Comment 15

[Jason Orendorff [:jorendorff]]

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

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jason, any idea what the next steps are here?

Comment 17

[Jason Orendorff [:jorendorff]]

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

Comment 18

[Jason Orendorff [:jorendorff]]

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.

Comment 19

[Jason Orendorff [:jorendorff]]

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

Comment 20

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

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 21

[Jason Orendorff [:jorendorff]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

No sec-approval?

Comment 23

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/da7bdb2b6ee3

This still needs a sec-approval nomination as well as beta/esr31 approval requests.

Comment 25

[Eric Faust [:efaust]]

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 26

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

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?

Comment 27

[Christian Holler (:decoder)]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 30

[Eric Faust [:efaust]]

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 31

[Lawrence Mandel [:lmandel] (use needinfo)]

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)

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx34

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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

Comment 35

[Eric Faust [:efaust]]

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?

Comment 36

[Jeff Walden [:Waldo]]

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*

Comment 37

[Eric Faust [:efaust]]

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.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/4bc71e296ce3

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d38dd7de64b3

Comment 40

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx34

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/5c8e19357e57