Assertion failure: CheckLexicalNameConflict(cx, lexicalEnv, varObj, name), at js/src/vm/Interpreter-inl.h:299

RESOLVED FIXED in Firefox 52

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: decoder, Assigned: shu)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla52
x86_64
Linux
assertion, jsbugmon, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox49 wontfix, firefox50 wontfix, firefox51+ wontfix, firefox52+ fixed)

Details

(Whiteboard: [jsbugmon:])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
The following testcase crashes on mozilla-central revision dc89484d4b45 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2 --ion-offthread-compile=off --ion-eager --baseline-eager --ion-extra-checks -f test/driver-dcd.js < test/execution.log):

See attachment.


Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x000000000084d108 in js::DefLexicalOperation (attrs=7, name=..., varObj=..., lexicalEnv=..., cx=0x7fa827d5f000) at js/src/vm/Interpreter-inl.h:299
#1  js::jit::DefLexical (cx=0x7fa827d5f000, dn=..., attrs=7, envChain=...) at js/src/jit/VMFunctions.cpp:214
#2  0x00007fa82926e1a4 in ?? ()
[...]
#14 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffd4ca95630	140725889619504
rcx	0x7fa828054a2d	140360202668589
rdx	0x0	0
rsi	0x7fa828323770	140360205612912
rdi	0x7fa828322540	140360205608256
rbp	0x7ffd4ca956e0	140725889619680
rsp	0x7ffd4ca95610	140725889619472
r8	0x7fa828323770	140360205612912
r9	0x7fa829414740	140360223377216
r10	0x0	0
r11	0x0	0
r12	0x7ffd4ca95650	140725889619536
r13	0x1da60a0	31088800
r14	0x7ffd4ca95640	140725889619520
r15	0x1da6140	31088960
rip	0x84d108 <js::jit::DefLexical(JSContext*, JS::Handle<js::PropertyName*>, unsigned int, JS::Handle<JSObject*>)+1032>
=> 0x84d108 <js::jit::DefLexical(JSContext*, JS::Handle<js::PropertyName*>, unsigned int, JS::Handle<JSObject*>)+1032>:	movl   $0x0,0x0
   0x84d113 <js::jit::DefLexical(JSContext*, JS::Handle<js::PropertyName*>, unsigned int, JS::Handle<JSObject*>)+1043>:	ud2    


This is an assertion that I keep seeing but that is extremely difficult to isolate and provide a testcase for. The attached zip file contains a minimized log and the required driver to run it. I was not able to merge them into a single file. The testcase is intermittent but reproduces cleanly most of the time. I suggest debugging this as long as it works and reproduces so nicely. I have quite a few other crashes with the same assert that never even reproduced initially.

Marking s-s due to intermittent behavior and because :shu once mentioned that this assertion is potentially dangerous.
(Reporter)

Comment 1

2 years ago
Created attachment 8802463 [details]
Testcase
(Reporter)

Comment 2

2 years ago
Needinfo from :shu because this is probably related to the parser/frontend.
Flags: needinfo?(shu)

Updated

2 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]

Comment 3

2 years ago
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Updated

2 years ago
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
(Assignee)

Comment 4

2 years ago
I ran the test a bunch of times and it never crashed with the build and shell flags. You might need to give me access to a machine where this reproduces for this.
Flags: needinfo?(shu)
(Assignee)

Comment 5

2 years ago
Created attachment 8804951 [details] [diff] [review]
Fix global redeclaration check for prologue bailouts from Ion.

I couldn't really reproduce this on my machine, but I think the bug is that we
can bailout of Ion on global frames before the redecl check. This ends up
resuming in the main body instead of the prologue because of some other
constraints, and so we never do the redecl check at all.

The new flag on BaselineFrame is kinda gross. Do you know of a cleaner way to
check whether we bailed out during the prologue of a global frame, Jan?

decoder, could you also check if this fixes the crash?
Attachment #8804951 - Flags: review?(jdemooij)
Attachment #8804951 - Flags: feedback?(choller)
Keywords: sec-high
Assignee: nobody → shu
Blocks: 1263355
status-firefox49: --- → unaffected
status-firefox50: --- → unaffected
status-firefox51: --- → affected
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
Flags: in-testsuite?
Per IRC discussion with Shu, this is probably a longer standing issue that was exposed by the rewrite. ni? him to figure that out.
status-firefox49: unaffected → ?
status-firefox50: unaffected → ?
status-firefox-esr45: --- → ?
Flags: needinfo?(shu)
Tracking 52+ since this is sec-hig - makes sense to track it.
tracking-firefox52: ? → +
Hm I'm not familiar with MGlobalNameConflictsCheck, but I think I understand the problem.

(1) Instead of adding a flag to BaselineFrame, we can store a flag in BaselineBailoutInfo maybe? That is passed to FinishBailoutToBaseline (just make sure you read it before the js_free(bailoutInfo)..). That way it only affects the bailout code.

(2) The bailoutInfo already stores the |jsbytecode* resumePC|. Is that + frame->script() enough info to figure out if we have to do this work?

(3) Can we make this a separate bytecode op? I've wanted to do that for a while for all other stuff we do in the prologue (CallObject allocation, interrupt check, etc).
(In reply to Jan de Mooij [:jandem] from comment #8)
> (3) Can we make this a separate bytecode op? I've wanted to do that for a
> while for all other stuff we do in the prologue (CallObject allocation,
> interrupt check, etc).

This may be annoying for the debugger though, but that might be easier to handle than the code we have in the JITs now, not sure.

The bailout is probably caused by the overrecursion check, at least that would explain why it's hard to repro. Maybe we should add a testing function to trigger random failures there (something like, after emitting x bailouts, emit one that bails unconditionally).

For this bug, your patch + (1) or (2) above is probably safest.
Comment on attachment 8804951 [details] [diff] [review]
Fix global redeclaration check for prologue bailouts from Ion.

Clearing review per comments 8/9.
Attachment #8804951 - Flags: review?(jdemooij)
(Assignee)

Comment 11

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #8)
> Hm I'm not familiar with MGlobalNameConflictsCheck, but I think I understand
> the problem.
> 
> (1) Instead of adding a flag to BaselineFrame, we can store a flag in
> BaselineBailoutInfo maybe? That is passed to FinishBailoutToBaseline (just
> make sure you read it before the js_free(bailoutInfo)..). That way it only
> affects the bailout code.

I'll do this.

> 
> (2) The bailoutInfo already stores the |jsbytecode* resumePC|. Is that +
> frame->script() enough info to figure out if we have to do this work?
> 

No, this is the same problem as the one that has come up many times before: a pc with offset 0 pre- or mid-prologue is indistinguishable from post-prologue. Which leads us to (3)...

> (3) Can we make this a separate bytecode op? I've wanted to do that for a
> while for all other stuff we do in the prologue (CallObject allocation,
> interrupt check, etc).

Yes, this would be nice. But not doing this rearchitecting for this patch. It would make not just this case but other bailout cases (and possibly weird debug mode OSR code as well) simpler.
Flags: needinfo?(shu)
(Assignee)

Comment 12

2 years ago
Created attachment 8808311 [details] [diff] [review]
Fix global redeclaration check for prologue bailouts from Ion.

Reworked the patch per comment 8.

decoder, since I still can't repro this reliably, could you check again if the
new patch still makes the crashes go away?
Attachment #8808311 - Flags: review?(jdemooij)
Attachment #8808311 - Flags: feedback?(choller)
(Assignee)

Updated

2 years ago
Attachment #8804951 - Attachment is obsolete: true
Attachment #8804951 - Flags: feedback?(choller)
Comment on attachment 8808311 [details] [diff] [review]
Fix global redeclaration check for prologue bailouts from Ion.

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

Thanks!
Attachment #8808311 - Flags: review?(jdemooij) → review+
Tracking for 51, once this has sec approval please request uplift.
tracking-firefox51: ? → +
https://hg.mozilla.org/mozilla-central/rev/5174d6ae8e9c

seems this landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/5174d6ae8e9c without sec-approval
status-firefox52: affected → fixed
Flags: needinfo?(abillings)
Target Milestone: --- → mozilla52

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Why did you land this, Shu? You need to mark the sec-approval? flag on the patch and answer the sec-approval template questions so we know if you just 0day'd our users.

Comment 6 says that this issue may have been "exposed by the rewrite." What version of Firefox did that land in? I need to know if Firefox 50 is exposed or not since it is marked "?" in the status flags above.
Flags: needinfo?(abillings) → needinfo?(shu)
(Assignee)

Comment 17

2 years ago
(In reply to Al Billings [:abillings] from comment #16)
> Why did you land this, Shu? You need to mark the sec-approval? flag on the
> patch and answer the sec-approval template questions so we know if you just
> 0day'd our users.
> 

My apologies. It did not occur to me this bug was still marked s-s. It shouldn't be. The bug is that certain JavaScript programs under rare circumstances would exhibit non-spec-compliant behavior. Specifically, global scope 'let' or 'const' declarations would be allowed to be redeclared. There's no security issue here. Christian marked this s-s based on the signature of the crash itself, before an analysis was made.

> Comment 6 says that this issue may have been "exposed by the rewrite." What
> version of Firefox did that land in? I need to know if Firefox 50 is exposed
> or not since it is marked "?" in the status flags above.

It is, but per above, this bug is not s-s and was conservatively marked s-s in the beginning.
Flags: needinfo?(shu)
Ah, Ok. Should we open this up? Dan?
Flags: needinfo?(dveditz)
status-firefox49: ? → wontfix
status-firefox50: ? → wontfix
status-firefox-esr45: ? → ---
(Reporter)

Comment 19

2 years ago
Comment on attachment 8808311 [details] [diff] [review]
Fix global redeclaration check for prologue bailouts from Ion.

Fix confirmed.
Attachment #8808311 - Flags: feedback?(choller) → feedback+
Group: javascript-core-security → core-security-release
Group: core-security-release
Flags: needinfo?(dveditz)
Keywords: sec-high
Hi :shu,
Is this worth uplifting to Beta51?
Flags: needinfo?(shu)
(Assignee)

Comment 21

2 years ago
(In reply to Gerry Chang [:gchang] from comment #20)
> Hi :shu,
> Is this worth uplifting to Beta51?

Given the extreme rarity of this triggering, I think no, just let the fix ride the trains as normal.
Flags: needinfo?(shu)

Updated

2 years ago
status-firefox51: affected → wontfix
You need to log in before you can comment on or make changes to this bug.