Closed
Bug 1496331
Opened 2 years ago
Closed 2 years ago
Assertion failure: bi.kind() == BindingKind::FormalParameter, at js/src/frontend/EmitterScope.cpp:623
Categories
(Core :: JavaScript Engine, defect, P3)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox62 | --- | disabled |
firefox63 | --- | disabled |
firefox64 | --- | verified |
People
(Reporter: arai, Assigned: arai)
References
Details
(Keywords: assertion, sec-high, testcase)
Attachments
(2 files)
1.03 KB,
application/octet-stream
|
Details | |
2.93 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
tested on m-i 21b67d2084a6 Configure flags: --enable-warnings-as-errors --disable-optimize --enable-debug Runtime flag: -B param.binjs Result: Assertion failure: bi.kind() == BindingKind::FormalParameter, at js/src/frontend/EmitterScope.cpp:623
Assignee | ||
Comment 1•2 years ago
|
||
encoded from js/src/jit-test/tests/basic/testBug772328.js I'll add all testcase in bug 1495611, so no need to add it here
Updated•2 years ago
|
Group: core-security → javascript-core-security
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•2 years ago
|
||
as regular Parser does, we should check the current declaration kind before altering. https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/js/src/frontend/Parser.cpp#1516-1549 in this case, it was altering FormalParameter to BodyLevelFunction, which shouldn't happen.
Attachment #9016166 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•2 years ago
|
Attachment #9016166 -
Flags: review?(dteller)
Comment on attachment 9016166 [details] [diff] [review] Do not alter declaration kind from FormalParameter to BodyLevelFunction. Review of attachment 9016166 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BinSource.cpp @@ +241,5 @@ > > if (parseContext_ && syntax == FunctionSyntaxKind::Statement) { > auto ptr = parseContext_->varScope().lookupDeclaredName(atom); > MOZ_ASSERT(ptr); > + // FIXME: Should be merged with ParseContext::tryDeclareVarHelper. Can you file a bug? @@ +244,5 @@ > MOZ_ASSERT(ptr); > + // FIXME: Should be merged with ParseContext::tryDeclareVarHelper. > + DeclarationKind declaredKind = ptr->value()->kind(); > + if (DeclarationKindIsVar(declaredKind)) { > + MOZ_ASSERT(declaredKind != DeclarationKind::VarForAnnexBLexicalFunction); Mmmmh... Isn't that something that an ill-formed file could achieve? ::: js/src/frontend/Parser.h @@ +1699,5 @@ > JSFunction* > AllocNewFunction(JSContext* cx, HandleAtom atom, FunctionSyntaxKind kind, GeneratorKind generatorKind, FunctionAsyncKind asyncKind, > HandleObject proto, bool isSelfHosting = false, bool inFunctionBox = false); > > +bool Could you take the opportunity to document this method?
Attachment #9016166 -
Flags: review?(dteller) → review+
Assignee | ||
Updated•2 years ago
|
Attachment #9016166 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 4•2 years ago
|
||
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #3) > @@ +244,5 @@ > > MOZ_ASSERT(ptr); > > + // FIXME: Should be merged with ParseContext::tryDeclareVarHelper. > > + DeclarationKind declaredKind = ptr->value()->kind(); > > + if (DeclarationKindIsVar(declaredKind)) { > > + MOZ_ASSERT(declaredKind != DeclarationKind::VarForAnnexBLexicalFunction); > > Mmmmh... Isn't that something that an ill-formed file could achieve? the actual implementation for annex B will be handled in bug 1496334 I'm going to keep it an assertion for now.
Assignee | ||
Comment 5•2 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a298e5b921f7dde9e3e36f25fae60a950e94a20b Bug 1496331 - Do not alter declaration kind from FormalParameter to BodyLevelFunction. r=Yoric
https://hg.mozilla.org/mozilla-central/rev/a298e5b921f7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Group: javascript-core-security → core-security-release
Updated•2 years ago
|
status-firefox62:
--- → disabled
status-firefox63:
--- → disabled
status-firefox-esr60:
--- → unaffected
Updated•2 years ago
|
Flags: qe-verify+
Comment 7•2 years ago
|
||
I need some additional info regarding how to use the runtime flag (-B param.binjs) specified in comment 0, in order to reproduce/verify the current issue.
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 8•2 years ago
|
||
it's a bit complicated to verify this on Nightly (given it needs a web server that serves the attached binjs file with special MIME-Type). If you're fine with testing with the debug build of JS shell, it's simple, here's the STR: 1. download debug build of JS shell from treeherder 2. download the attached .binjs file as "a.binjs" 3. run the shell with `js -B a.binjs` command If you can prepare a private local web server that can have special MIME-Type, here's the STR with Nightly: 1. download debug build of Nightly 2. set dom.script_loader.binast_encoding.enabled pref to true in Nightly 3. download the attached .binjs file as "a.binjs" 4. create a file named "index.html" with the following content: <script src="a.binjs" async></script> 5. put a.binjs and index.html in the web server 6. associate .binjs extension with "application/javascript-binast" MIME-Type in the web server 7. open index.html from the web server with Nightly this fixes assertion failure which can result in wrong pointer access or something.
Flags: needinfo?(arai.unmht)
Comment 9•2 years ago
|
||
I tried both of your STR versions, but without managing to notice a difference between the affected build and the fixed version: - downloaded the debug build of the JS shell (target.jsshell.zip) from the corresponding treeherder location, extracted the shell, saved the attached .binjs file and ran the shell with the specified command. - installed the web server (tried with both Cheyenne and XAMP), set the "dom.script_loader.binast_encoding.enabled" pref to true in the corresponding Fx debug build, saved the attached .binjs file and moved it in the web server's upload folder (e.g. xampp\htdocs\) along with the index.html file, asociated .binjs extension with "application/javascript-binast" (e.g. set "application/javascript-binast .binjs" in the server's configuration file), then opened index.html using the debug build of Fx. Both affected build and the fixed one had the same behavior and also, no "Assertion failure: bi.kind() == BindingKind::FormalParameter" was noticed in terminal or Browser Console. Could you please confirm if the fix was properly implemented on 64 version?
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 10•2 years ago
|
||
confirmed on macOS failure on debug build of js shell from https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=c1ee6c65f85b0ea68006a3802db0937220d55da3&selectedJob=205636718 fixed on debug build of js shell from https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=4c11ab0cd98950983cfc957f579ace6c3e918a43&selectedJob=205637088
Status: RESOLVED → VERIFIED
Flags: needinfo?(arai.unmht)
Comment 11•2 years ago
|
||
Thank you, :arai! I will update the corresponding flags, per comment 10.
Flags: qe-verify+
Updated•1 year ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•