Closed Bug 1496333 Opened 1 year ago Closed 1 year ago

Assertion failure: positionalParams.get().length() == paramsCount, at js/src/frontend/BinSource.cpp:483

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, Whiteboard: [post-critsmash-triage])

Attachments

(2 files, 1 obsolete file)

Attached file rest.binjs
tested on m-i 21b67d2084a6

Configure flags: --enable-warnings-as-errors --disable-optimize --enable-debug

Runtime flag: -B rest.binjs

Result:
Assertion failure: positionalParams.get().length() == paramsCount, at js/src/frontend/BinSource.cpp:483
encoded from js/src/jit-test/tests/basic/spread-call-rest.js

I'll add all testcase in bug 1495611, so no need to add it here
I'll take a look from this
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Group: core-security → javascript-core-security
Getting the parameter list length sounds like it could go bad quickly, guessing sec-high.
so, turns out that the length of ListOfAssertedMaybePositionalParameterName has nothing to do with non-rest parameter's length,
for the following reasons:
  * ListOfAssertedMaybePositionalParameterName can contain the name of destructuring parameters,
    so the length of ListOfAssertedMaybePositionalParameterName can be greater than the length of parameters (even more than 65535)
  * there can be empty destructuring parameters, so the length of parameters can be shorter than ListOfAssertedMaybePositionalParameterName length

it means we need to resize positionalParams on demand, up to 65535 (which is just implementation detail tho).
Attachment #9015487 - Flags: review?(efaustbmo)
Comment on attachment 9015487 [details] [diff] [review]
Do not use AssertedParameterScope.paramNames.length for allocation.

apparently the patch is broken.
I'll post again
Attachment #9015487 - Attachment is obsolete: true
Attachment #9015487 - Flags: review?(efaustbmo)
Comment on attachment 9015770 [details] [diff] [review]
Do not use AssertedParameterScope.paramNames.length for allocation.

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

Makes sense to me. Nice of you to wire up destructuring ahead of time, even though we can't parse them right now.
Attachment #9015770 - Flags: review?(efaustbmo) → review+
Depends on: 1497446
https://hg.mozilla.org/integration/mozilla-inbound/rev/3293a3393b4a68e942b7f8fd5f2dd77b337b8676
Bug 1496333 - Do not use AssertedParameterScope.paramNames.length for allocation. r=efaust
https://hg.mozilla.org/mozilla-central/rev/3293a3393b4a
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Hello,
In order to verify this issue, could you please provide some STR?
It would be greatly appreciated, thank you!
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 out-of-range memory access
Flags: needinfo?(arai.unmht)
Hello,
I tried to reproduce this issue using the steps from comment 11, but to no avail. Can you please confirm the fix repairs the issue?
Thank you!
Flags: qe-verify+ → needinfo?(arai.unmht)
confirmed the fix with debug build of jsshell on macOS, retrieved from treeherder
  before: m-c c1ee6c65f85b : reproducible
  after:  m-c 4c11ab0cd989 : not reproducible
Flags: needinfo?(arai.unmht)
I am closing this issue based on comment 13.
Thank you!
Status: RESOLVED → VERIFIED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.