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)

defect

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)

Attached file param.binjs
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
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
Group: core-security → javascript-core-security
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
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)
Depends on: 1497446
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+
Attachment #9016166 - Flags: review?(efaustbmo)
(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.
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
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)
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)
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)
Thank you, :arai!
I will update the corresponding flags, per comment 10.
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.