Closed Bug 1240414 Opened 4 years ago Closed 4 years ago

Crash [@ callee] or Assertion failure: isNonEvalFunctionFrame(), at vm/Stack.h:676 with ES6 Classes

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox45 --- unaffected
firefox46 + verified
firefox47 + verified
firefox-esr38 --- unaffected

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(6 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(1 file)

The following testcase crashes on mozilla-central revision 5644818538de (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug, run with --fuzzing-safe --no-threads):

new class extends class {} {
  constructor() { eval("this") }
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
callee (this=<synthetic pointer>) at js/src/vm/Stack-inl.h:682
#0  callee (this=<synthetic pointer>) at js/src/vm/Stack-inl.h:682
#1  js::ThrowUninitializedThis (cx=cx@entry=0x7ffff6907800, frame=...) at js/src/vm/Interpreter.cpp:4875
#2  0x0000000000835f59 in Interpret (cx=cx@entry=0x7ffff6907800, state=...) at js/src/vm/Interpreter.cpp:2497
[...]
#17 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6987
rax	0x7fffffffffff	140737488355327
rbx	0x7ffff6907800	140737330051072
rcx	0x7ffff5258158	140737306263896
rdx	0xe5e5e5e5e5e5e5e5	-1880844493789993499
rsi	0x7ffff5258158	140737306263896
rdi	0x7ffff6907800	140737330051072
rbp	0x7ffff6907818	140737330051096
rsp	0x7fffffffb840	140737488336960
r8	0x3b	59
r9	0x7ffff7e607c0	140737352435648
r10	0x7ffff52ff5bc	140737306949052
r11	0x1b	27
r12	0x7fffffffc150	140737488339280
r13	0x7fffffffc190	140737488339344
r14	0x17e3be0	25050080
r15	0x4000000	67108864
rip	0x8277ae <js::ThrowUninitializedThis(JSContext*, js::AbstractFramePtr)+206>
=> 0x8277ae <js::ThrowUninitializedThis(JSContext*, js::AbstractFramePtr)+206>:	and    -0x10(%rdx),%rax
   0x8277b2 <js::ThrowUninitializedThis(JSContext*, js::AbstractFramePtr)+210>:	jmpq   0x82770a <js::ThrowUninitializedThis(JSContext*, js::AbstractFramePtr)+42>


Marking s-s due to use-after-free, ES6 Classes are nightly-only though.
(In reply to Christian Holler (:decoder) from comment #0)
> Marking s-s due to use-after-free, ES6 Classes are nightly-only though.

ES6 classes are no longer nightly-only since Firefox 45, see https://groups.google.com/d/msg/mozilla.dev.platform/nz0eLJIAWDw/qV6N9TNuAwAJ

This one is my fault though, regression from bug 1234845.
Blocks: 1234845
Flags: needinfo?(jdemooij)
Attached patch PatchSplinter Review
A couple ThrowUninitializedThis changes:

* Since bug 1234845, we can no longer call frame->callee() when the frame is an eval frame. Fortunately, we can use script->getCallerFunction() in this case.

* This patch also makes the error message a bit clearer when uninitialized-this is used inside an arrow function. I think we used to report that as an anonymous class but now we say "... in arrow function in class constructor".

Maybe we could recover the class name from the scope chain in this case, but that's fairly complicated and not necessary for now.

* Finally, the error message tests I added failed with --baseline-eager: GetScopeName needs to check for .this, similar to the check we have in FetchName.

Unfortunately the interpreter calls NameOperation/FetchName and Baseline calls GetScopeName. Oh well.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8709881 - Flags: review?(efaustbmo)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160114062530" and the hash "ac467630a2d6a2c41d3148bbf41bbfbbd9dd92e4".
The "bad" changeset has the timestamp "20160114080631" and the hash "8310deb3b848ab94ee35e6a2e1b25b98157e381e".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ac467630a2d6a2c41d3148bbf41bbfbbd9dd92e4&tochange=8310deb3b848ab94ee35e6a2e1b25b98157e381e
Comment on attachment 8709881 [details] [diff] [review]
Patch

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

This looks good to me.

::: js/src/vm/Interpreter.cpp
@@ +4907,5 @@
> +        return false;
> +    }
> +
> +    MOZ_ASSERT(fun->isArrow());
> +    JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_UNINITIALIZED_THIS_ARROW);

Not that it really matters, but do we really /need/ a new error message here? I guess we benefit from the extra specificity if the arrow function references |this| and escapes the constructor's call context?
Attachment #8709881 - Flags: review?(efaustbmo) → review+
(In reply to Eric Faust [:efaust] from comment #4)
> Not that it really matters, but do we really /need/ a new error message
> here? I guess we benefit from the extra specificity if the arrow function
> references |this| and escapes the constructor's call context?

The problem is that the existing JSMSG_UNINITIALIZED_THIS error expects the name of the class:

  "|this| used uninitialized in {0} class constructor"

What are we going to use here if we're inside an arrow function and don't have the class name? We could try to recover it from the scope chain, but that seems more trouble than it's worth.

If we use an empty string, we end up with 2 spaces between "in" and "class". So, to solve this, it seemed simplest to add a second error message that does not require an explicit name. The added context ("arrow function in class constructor") was a nice bonus.
Comment on attachment 8709881 [details] [diff] [review]
Patch

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?
Not sure, I don't think it's easy.

* Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

* Which older supported branches are affected by this flaw?
46+ (Nightly and Aurora)

* If not all supported branches, which bug introduced the flaw?
Bug 1234845.

* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply or be trivial to backport.

* How likely is this patch to cause regressions; how much testing does it need?
Unlikely. It only affects an error case with ES6 classes, a new feature.
Attachment #8709881 - Flags: sec-approval?
Comment on attachment 8709881 [details] [diff] [review]
Patch

sec-approval+ for trunk. Please nominate a patch for branch as well so we can fix it in both affected places.
Attachment #8709881 - Flags: sec-approval? → sec-approval+
Comment on attachment 8709881 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1234845.
[User impact if declined]: Crashes, security bugs.
[Describe test coverage new/current, TreeHerder]: Fixes the reported test.
[Risks and why]: Low risk. Only affects an uncommon error case for ES6 classes.
[String/UUID change made/needed]: None.
Attachment #8709881 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/74ff645145b8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Tracking for 46 since this is a sec-high issue and a recent regression.
Comment on attachment 8709881 [details] [diff] [review]
Patch

Crash fix, verified on m-c. Please uplift to aurora.
Attachment #8709881 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: javascript-core-security → core-security-release
has problems uplifting:

grafting 328738:74ff645145b8 "Bug 1240414 - Fix ThrowUninitializedThis to do the right thing for eval and arrow function frames. r=efaust"
merging js/src/js.msg
merging js/src/vm/Interpreter.cpp
merging js/src/vm/Xdr.h
warning: conflicts while merging js/src/vm/Xdr.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
Flags: needinfo?(jdemooij)
Group: core-security-release
JSBugMon: This bug has been automatically verified fixed on Fx46
You need to log in before you can comment on or make changes to this bug.