1192401 - (CVE-2015-4507) Crash due to Assertion failure: getSlotRef(EVAL).isUndefined(), at js/src/vm/GlobalObject.h:147

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Spandan Veggalam]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150629114049

Steps to reproduce:

mozilla-central revision 892594bdad30 (build with: --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --enable-debug)

const dbg = new Debugger();
const g = evalcx("lazy");
dbg.addDebuggee(g);
dbg.memory.trackingAllocationSites = true;
g.eval("this.alloc = {}");


Actual results:

Assertion failure: getSlotRef(EVAL).isUndefined(), at js/src/vm/GlobalObject.h:147



Expected results:

undefined or some other relevant message

Comment 1

[Jan de Mooij [:jandem]]

Thanks for the report! CC'ing the debugger gurus.

Nick, I'll NI you because of the trackingAllocationSites bits, please forward if that's unrelated.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: full backtrace

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

bt 10

> #0  js::GlobalObject::setOriginalEval (this=0x10657c060, evalobj=0x106588380) at GlobalObject.h:148
> #1  0x000000010011e954 in FinishObjectClassInit (cx=0x103221000, ctor={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfab58}, proto={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfab98}) at Object.cpp:1097
> #2  0x0000000100301695 in js::GlobalObject::resolveConstructor (cx=0x103221000, global={<js::HandleBase<js::GlobalObject *>> = {<No data fields>}, ptr = 0x7fff5fbfad10}, key=JSProto_Object) at GlobalObject.cpp:206
> #3  0x0000000100300df8 in js::GlobalObject::ensureConstructor (cx=0x103221000, global={<js::HandleBase<js::GlobalObject *>> = {<No data fields>}, ptr = 0x7fff5fbfad10}, key=JSProto_Object) at GlobalObject.cpp:99
> #4  0x0000000100a1ba1d in JS_ResolveStandardClass (cx=0x103221000, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb158}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbfb738}, resolved=0x7fff5fbfae9b) at jsapi.cpp:1314
> #5  0x000000010001b56e in sandbox_resolve (cx=0x103221000, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb158}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbfb738}, resolvedp=0x7fff5fbfae9b) at js.cpp:2381
> #6  0x000000010033e8ac in js::CallResolveOp (cx=0x103221000, obj={<js::HandleBase<js::NativeObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb158}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbfb738}, propp={<js::MutableHandleBase<js::Shape *>> = {<No data fields>}, ptr = 0x7fff5fbfb130}, recursedp=0x7fff5fbfaf8f) at NativeObject-inl.h:388
> #7  0x00000001003368ef in js::LookupOwnPropertyInline<(js::AllowGC)1> (cx=0x103221000, obj={<js::HandleBase<js::NativeObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb158}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbfb738}, propp={<js::MutableHandleBase<js::Shape *>> = {<No data fields>}, ptr = 0x7fff5fbfb130}, donep=0x7fff5fbfb117) at NativeObject-inl.h:477
> #8  0x000000010033a669 in NativeGetPropertyInline (cx=0x103221000, obj={<js::HandleBase<js::NativeObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb308}, receiver={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb418}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbfb738}, nameLookup=NotNameLookup, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x1067100b0}) at NativeObject.cpp:1910
> #9  0x000000010033a57a in js::NativeGetProperty (cx=0x103221000, obj={<js::HandleBase<js::NativeObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb308}, receiver={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb418}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbfb738}, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x1067100b0}) at NativeObject.cpp:1954
> (More stack frames follow...)

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I'm not really sure what is going on here as I don't have a good grasp of how lazy standard classes work. Somewhat obvious, given the test case and backtrace, is that there are issues with tracking allocations and then instantiating lazy standard classes.

Digging in some more...

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: backtrace at preceeding setOriginalEval call

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Do not capture SavedFrame stacks before Object.prototype has been initialized

Comment 8

[Andrew McCreight [:mccr8]]

Marking as sec-moderate because it requires the debugger running some very specific steps.

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8646503 [details] [diff] [review]
Do not capture SavedFrame stacks before Object.prototype has been initialized

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

I suppose this code ensures that SavedFrame.prototype is instantiated?

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Shu-yu Guo [:shu] from comment #9)
> I suppose this code ensures that SavedFrame.prototype is instantiated?

Yes: https://dxr.mozilla.org/mozilla-central/rev/d4f3a8a75577e4af2914a4e899ca2e724f9715c4/js/src/vm/SavedStacks.cpp#1189

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae8717fafc50

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f05420e460d

How far back does this go?

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Looking at the source in each of aurora/beta/release/esr38, I think all branches are affected.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8646503 [details] [diff] [review]
Do not capture SavedFrame stacks before Object.prototype has been initialized

This existing patch should still apply on aurora.

Approval Request Comment
[Feature/regressing bug #]: js::SavedStacks / Debugger API allocation site tracking
[User impact if declined]: possible crashes when using the Debugger API
[Describe test coverage new/current, TreeHerder]: Fairly extensive test coverage, plus new testcase that crashes without the rest of the patch
[Risks and why]: Very little. Only pref'd off developer facing features use this part of the Debugger API at the moment.
[String/UUID change made/needed]: None

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: beta.patch

Patch for beta. See comment 14.

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Do we want to uplift this to release/esr? How do we decide what should or should not be uplifted that far?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Beta is probably far enough. We typically don't take sec-moderates on esr38 without a really good reason to do so. And for release it'd basically have to be chemspill-worthy to take. Thanks for the quick reply and rebase!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f05420e460d

Comment 19

[Spandan Veggalam]

Thanks for your consideration and fixation.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8648121 [details] [diff] [review]
beta.patch

Fix a crash, taking it.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2857cf15aba3
https://hg.mozilla.org/releases/mozilla-beta/rev/2d9e2f22c813

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Win8 PGO runs on beta appear to be hitting frequent jittest timeouts since this was uplifted there:
https://treeherder.mozilla.org/logviewer.html#?job_id=467828&repo=mozilla-beta

https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&filter-searchStr=Win%20x64%20JIT&fromchange=0aa0af046d24&tochange=6fbcbf05900f

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Kind of baffled why that is reported as failing.

Unless I am completely misunderstanding, I don't think it is timing out as it says "timed-out: false". It seems to complain about "InternalError: too much recursion" and the exit 3 is what I remember being used for uncaught exceptions. But failing because of too much recursion is *also* strange because the top of the test in question has "|jit-test| allow-oom; allow-overrecursed" and I would think "allow-overrecursed" would allow this test to pass in its current state.

Is there a practical difference between "|jit-test| allow-overrecursed" and "|jit-test| error: InternalError: too much recursion"? I see some tests using one and some using the other.

On top of all that, I'm also baffled as to how this change would affect that test case.

Anyways, I'm not sure what the immediate action should be. Should we back this out from Beta?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Maybe shu can weigh in on that?

Comment 25

[Jan de Mooij [:jandem]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #23)
> But failing because of too much recursion is *also* strange
> because the top of the test in question has "|jit-test| allow-oom;
> allow-overrecursed" and I would think "allow-overrecursed" would allow this
> test to pass in its current state.

I added that allow-overrecursed last week so it's not on beta:

https://hg.mozilla.org/integration/mozilla-inbound/rev/45abee091485

Maybe I should have filed a bug for that, but it was a followup to unbreak the tree and I didn't expect this to start happening on other branches out of the blue...

> Is there a practical difference between "|jit-test| allow-overrecursed" and
> "|jit-test| error: InternalError: too much recursion"? I see some tests
> using one and some using the other.

I think the former means overrecursion is allowed but the test will pass if we don't overrecurse. In the latter case, we require an overrecursion error.