Closed Bug 1042567 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla36
Tracking Status
firefox33 --- wontfix
firefox34 + verified
firefox35 + verified
firefox36 + verified
firefox-esr31 34+ fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: gkw, Assigned: jorendorff)

References

Details

(5 keywords, Whiteboard: [jsbugmon:origRev=dc352a7bf234,testComment=9,update][adv-main34+][adv-esr31.3+][b2g-adv-main2.2-])

Crash Data

Attachments

(3 files)

Attached file 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?
Flags: needinfo?(jorendorff)
[Tracking Requested - why for this release]:
Flags: needinfo?(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.
Depends on: 1037770
Bug 1037770 is now fixed, so setting needinfo? from :jorendorff. :)
Flags: needinfo?(jorendorff)
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 64c4ec2df3d4).
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:bisectfix]
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:]
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?
Flags: needinfo?(jwalden+bmo)
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.
Flags: needinfo?(jorendorff)
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).
Flags: needinfo?(jwalden+bmo)
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)
Whiteboard: [jsbugmon:] → [jsbugmon:origRev=dc352a7bf234,testComment=9,update]
Attached patch Proposed fix — — Splinter Review
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.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8479484 - Flags: review?(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.
Attachment #8479484 - Flags: review?(jorendorff)
Attached patch bug-1042567-freeze-v1.patch — — Splinter Review
4-line comment makes me sad, it should be 1-line but i don't have the right words
Assignee: efaustbmo → jorendorff
Attachment #8480841 - Flags: review?(efaustbmo)
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.
Attachment #8480841 - Flags: review?(efaustbmo)
Attachment #8480841 - Flags: review+
Attachment #8480841 - Flags: feedback?(jwalden+bmo)
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.
Attachment #8480841 - Flags: feedback?(jwalden+bmo) → feedback+
Jason, any idea what the next steps are here?
Flags: needinfo?(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
Flags: needinfo?(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.
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.
Flags: needinfo?(jorendorff)
Target Milestone: --- → mozilla36
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Attachment #8480841 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jorendorff)
No sec-approval?
Group: javascript-core-security
[Tracking Requested - why for this release]:
Attachment #8480841 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8480841 - Flags: sec-approval?
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?
Attachment #8480841 - Flags: sec-approval? → sec-approval+
Status: RESOLVED → VERIFIED
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?
Flags: needinfo?(efaustbmo)
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.
Flags: needinfo?(jorendorff)
Flags: needinfo?(efaustbmo)
Attachment #8480841 - Flags: approval-mozilla-esr24?
Attachment #8480841 - Flags: approval-mozilla-beta?
Attachment #8480841 - Flags: approval-mozilla-esr24? → approval-mozilla-esr31?
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)
Attachment #8480841 - Flags: approval-mozilla-esr31?
Attachment #8480841 - Flags: approval-mozilla-esr31+
Attachment #8480841 - Flags: approval-mozilla-beta?
Attachment #8480841 - Flags: approval-mozilla-beta+
JSBugMon: This bug has been automatically verified fixed on Fx34
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?
Flags: needinfo?(jorendorff) → needinfo?(jwalden+bmo)
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*
Flags: needinfo?(jwalden+bmo)
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.
Depends on: 1065604
JSBugMon: This bug has been automatically verified fixed on Fx34
Whiteboard: [jsbugmon:origRev=dc352a7bf234,testComment=9,update] → [jsbugmon:origRev=dc352a7bf234,testComment=9,update][adv-main34+][adv-esr31.3+]
Whiteboard: [jsbugmon:origRev=dc352a7bf234,testComment=9,update][adv-main34+][adv-esr31.3+] → [jsbugmon:origRev=dc352a7bf234,testComment=9,update][adv-main34+][adv-esr31.3+][b2g-adv-main2.2-]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.