TM: Crash [@ js::TraceRecorder::traverseScopeChain] or "Assertion failure: isObject()," or TraceRecorder::traverseScopeChain doesn't handle call objects for strict mode eval frames

RESOLVED FIXED in mozilla2.0b11

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({assertion, testcase})

unspecified
mozilla2.0b11
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [sg:dos null-deref][softblocker][fixed-in-tracemonkey])

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
./js -j -m
var COUNT = 10; eval("'use strict'; for (let j = 0; j < COUNT; j++); x;");
Assertion failure: isObject(), at ../jsvalue.h:602
Aborted (core dumped)

The method performs a scope chain walk looking for Call objects, and it blindly calls getCallObjCalleeFunction() at one point -- but that's not valid on strict eval Call objects, and we quickly assert in debug builds, probably crash in opt builds.

This'll bite any strict mode eval that tries to use a variable not defined in the eval code in code the tracer tries to record, which seems to warrant fixing before we release strict eval to the masses in (probably) b9.
(Assignee)

Updated

7 years ago
blocking2.0: --- → ?
blocking2.0: ? → beta9+
Whiteboard: softblocker
(Assignee)

Comment 1

7 years ago
Recording a couple more failures here to make sure I fix everything here:

var COUNT = 10;
function t() { eval("'use strict'; for (let j = 0; j < COUNT; j++); x;"); }
t()
Assertion failure: fp->hasCallObj(), at ../jsinterpinlines.h:735


var COUNT = 10;
function t() { eval("'use strict'; for (let j = 0; j < COUNT; j++); x;"); }
try { t(); throw new Error('fail'); } catch (e) { assertEq(e instanceof ReferenceError, true); }
Assertion failure: fp->hasCallObj(), at ../jsinterpinlines.h:735
This was marked as a beta9 blocker after code freeze and before we spun builds, and didn't make it into the build. What gives?

Updated

7 years ago
Group: core-security

Comment 3

7 years ago
Eh. This is rare if ever used on the web but a test case that crashes us was filed openly, so if we ship this, that blows. I will check what kind of crash. This might be not sg:critical, just sg:dos.
Moving back to a nomination based on comment 3 - we can always get it in a dot release, or approve a safe patch.
blocking2.0: beta9+ → ?
(Assignee)

Comment 5

7 years ago
I'm pretty sure this bug is not a security concern.  It's a call object where we call getCallObjCalleeFunction() on it.  That retrieves a slot which, for ordinary callobj-having frames is non-null, and pulls a function private out of it.  For strict eval frames it's null, so pulling a function private from it should be a safe near-null crash.
blocking2.0: ? → betaN+
(Assignee)

Comment 6

7 years ago
Created attachment 503033 [details] [diff] [review]
Patch and test

This fixes the testcase.

More interesting, perhaps, is what's not changed -- all the other places that call getCallObjCallee(Function)?.  The key is that eval variables are defined with PropertyStub, so the getterOp and setterOp are NULL.  This causes all the callers in methodjit, and in the tracer (save these two), to never be hit, because they depend on a shape having a particular non-stub op -- either to be hit (the methodjit bits), or to cause their callers to never be called in that situation (the tracer bits).  But I could probably use a double-check here.  :-)
Attachment #503033 - Flags: review?(lw)

Comment 7

7 years ago
Comment on attachment 503033 [details] [diff] [review]
Patch and test

I'm really the wrong one for this; probably jorendorff or brendan.

My only comment is that it would be nice to factor out a "call and (for eval or heavy)" predicate, called from both places, and lay down a thick layer of comment icing as to why we care about some call objects and not others (I had trouble understanding this from the current comment near the top of traverseScopeChain).
Attachment #503033 - Flags: review?(lw)
(Assignee)

Comment 8

7 years ago
Comment on attachment 503033 [details] [diff] [review]
Patch and test

To be honest, I'm not even entirely sure I understand the reason for the distinction.  Heavyweight means "requires Call object" (ignoring strict eval), so it would have seemed to me unnecessary.  But the change did seem in the spirit of the current code, and I was loathe to make less careful changes now without a better understanding of the code.
Attachment #503033 - Flags: review?(brendan)
See bug 622781. We need to fix that bug first and rebase here.

/be
Depends on: 622781
Group: core-security
Whiteboard: softblocker → [sg:dos null-deref] softblocker
Comment on attachment 503033 [details] [diff] [review]
Patch and test

Luke's suggestion about an inline helper to common that chunky condition is good, but if we can get rid of this code (or simplify it both in size and in how often it runs) by fixing bug 622781 first, we should.

/be
Attachment #503033 - Flags: review?(brendan) → review-
(Assignee)

Comment 11

7 years ago
Comment on attachment 503033 [details] [diff] [review]
Patch and test

It would seem to me better to keep this "chunky", actually.  That will make it clearer that further changes are needed, in a particular way, when we refactor scope chain handling to make it no longer object-based.  (If bug 622781 beats it, all the better, but I'd rather not hold my breath when we can fix this mistake now.)  Can you explain what that condition actually does?

But if I'm to fix this by adding an inline helper, despite my thinking it potentially less illuminating in the future, could you at least explain its point to me?
Attachment #503033 - Flags: review- → review?(brendan)
See Also: → bug 626767
Summary: TraceRecorder::traverseScopeChain doesn't handle call objects for strict mode eval frames → TM: Crash [@ js::TraceRecorder::traverseScopeChain] or "Assertion failure: isObject()," or TraceRecorder::traverseScopeChain doesn't handle call objects for strict mode eval frames
Comment on attachment 503033 [details] [diff] [review]
Patch and test

Waldo: no deep explanation is needed. An inline helper is just commoning repeated code -- DRY.

Were you asking about the isHeavyweight condition? I'm not sure why that is there, probably in case a lightweight function has a Call object created for it due to debugger mischief. That "should not happen", though. Maybe dmandelin remembers more.

There's no guarantee this open-coding will help or hinder any revision of scopes to use non-objects for activation scopes. The code is full of Call object uses. If an inline helper is added, it will test &js_CallClass and then you'll have to find its callers. But clearly we can go too far with this argument, and open-code too much.

For now this seems fine.

/be
Attachment #503033 - Flags: review?(dmandelin)
Attachment #503033 - Flags: review?(brendan)
Attachment #503033 - Flags: review+
(Assignee)

Comment 13

7 years ago
http://hg.mozilla.org/tracemonkey/rev/bc2ae2ec69c4
Whiteboard: [sg:dos null-deref] softblocker → [sg:dos null-deref][softblocker][fixed-in-tracemonkey]
(Assignee)

Comment 14

7 years ago
Backed out in a blaze of orange.  Not for problems with this patch, but for problems with a patch that landed at the same time as it, then its followups, then complete orangetastrophe which might either have been those patches' fault or a different push not by me at all.  I'll figure it out tomorrow afternoon hopefully.
Whiteboard: [sg:dos null-deref][softblocker][fixed-in-tracemonkey] → [sg:dos null-deref][softblocker]
(Assignee)

Comment 15

7 years ago
http://hg.mozilla.org/tracemonkey/rev/46b9f2a8343b
Whiteboard: [sg:dos null-deref][softblocker] → [sg:dos null-deref][softblocker][fixed-in-tracemonkey]
Target Milestone: mozilla2.0b9 → mozilla2.0b11
Attachment #503033 - Flags: review?(dmandelin)
(In reply to comment #12)
> Were you asking about the isHeavyweight condition? I'm not sure why that is
> there, probably in case a lightweight function has a Call object created for it
> due to debugger mischief. That "should not happen", though. Maybe dmandelin
> remembers more.

I don't remember exactly. I remember there was a fear of debugger mischief at the time. But I don't remember whether that condition was required to pass test failures or if it was theoretical.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.