1685925 - Crash [@ ??] or Assertion failure: Expecting length to fit in int32, at jit/VMFunctions.cpp:2789

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20210108-1c7597ab79cc (opt build, run with --fuzzing-safe --ion-offthread-compile=off --baseline-warmup-threshold=0):

var realToString = toString;
function tryItOut(code) {
  try {
    f = new Function(code);
  } catch (exc) {}
  try { f(); } catch(exc) {}
  delete this.toString;
  this.toString = realToString;
}
t0 = new Uint8ClampedArray();
  Object.defineProperty(this, 'g0', {
    get: function() {
      return o1.length;
    }
  });
o1={};
tryItOut(`g0`);
g0;
tryItOut("}");
tryItOut("}");
tryItOut("}")
tryItOut("}");
o1 = new Uint32Array();
tryItOut("}");
o1= Object.create(t0);
Array.p([g0]);

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000022f2b6aa609 in ?? ()
#0  0x0000022f2b6aa609 in ?? ()
#1  0xfffe01b4d1800900 in ?? ()
#2  0xfffe170f1be96400 in ?? ()
#3  0xfff9800000000000 in ?? ()
#4  0x0000000000000000 in ?? ()
rax	0x1b4d1800930	1876120570160
rbx	0x2f2f2f2fffff	51879701643263
rcx	0xffffaf2f2f2f2f2f	-88857786765521
rdx	0x7fffffffc360	140737488339808
rsi	0x0	0
rdi	0x7ffff6023000	140737320726528
rbp	0x7fffffffc118	140737488339224
rsp	0x7fffffffc0b8	140737488339128
r8	0x7ffff6023030	140737320726576
r9	0x7ffff6023000	140737320726528
r10	0x7fffffffb708	140737488336648
r11	0x1ffff	131071
r12	0x0	0
r13	0x0	0
r14	0x1043	4163
r15	0x7ffff6023000	140737320726528
rip	0x22f2b6aa609	2401615128073
=> 0x22f2b6aa609:	cmpl   $0x1,-0x17(%rbx)
   0x22f2b6aa60d:	jne    0x22f2b6aa63b

Marking s-s because the crash address has a 0x2f pattern and a debug build crashes with a dangerous-looking trap.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jan de Mooij [:jandem]]

This is bad.

Simpler test case below, fails with --baseline-eager.

function f(o) {
  return o.length;
}
let objects = [
  {},
  {length: 0},
  [],
  {x: 0, lenght: 0},
  {x: 0, y: 0, length: 0},
  {x: 0, y: 0, z: 0, length: 0},
  new Uint32Array(),
  Object.create(new Uint8Array()),
];
objects.forEach(f);

Comment 3

[Jan de Mooij [:jandem]]

It's hard for fuzzers to find this because it requires a megamorphic IC.

I'll add a JIT option to turn all ICs into megamorphic ones.

Comment 4

[Jan de Mooij [:jandem]]

We should be really careful with this, it's likely not too hard to write a test based on the fix.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1685925 - Only tryAttachTypedArrayLength for specialized stubs. r?iain!

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1685925 - Add a JitOption to force megamorphic ICs. r?iain!

Depends on D101344

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1685925 - Add tests. r?iain!

Depends on D101345

Comment 8

[Christian Holler (:decoder)]

(In reply to Jan de Mooij [:jandem] from comment #3)

It's hard for fuzzers to find this because it requires a megamorphic IC.

I'll add a JIT option to turn all ICs into megamorphic ones.

I suggest we do an audit with those options before we land them.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Can we file a separate sec-audit bug for D101345?

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 9196371 [details]
Bug 1685925 - Only tryAttachTypedArrayLength for specialized stubs. r?iain!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's likely easy to figure out the problem from the patch, for someone experienced.
• 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?: All
• If not all supported branches, which bug introduced the flaw?: Bug 1635878
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Should apply or be easy to backport.
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely, simple fix, doesn't need much testing.

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9196371 [details]
Bug 1685925 - Only tryAttachTypedArrayLength for specialized stubs. r?iain!

Approved to land and request uplift

Comment 12

[Julien Cristau [:jcristau]]

Please request uplift when you get a chance.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/ec930acbd073678eda29f60e7e6b0d89988a02ca

I still think that D101345 should get a new bug filed for it to make it easier to track for fuzzing.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec930acbd073

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 9196371 [details]
Bug 1685925 - Only tryAttachTypedArrayLength for specialized stubs. r?iain!

Beta/Release Uplift Approval Request

• User impact if declined: Security issues, crashes.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just disables a particular optimization in one more case.
• String changes made/needed: N/A

Comment 16

[Christian Holler (:decoder)]

So I tried to figure out why fuzzilli wouldn't easily detect this with the new megamorphic patch. I wrote this test, which resembles a bit better what fuzzilli does and it reproduces with --no-threads --cpu-count=1 --ion-offthread-compile=off --baseline-eager --fast-warmup --fuzzing-safe:

setJitCompilerOption("ic.force-megamorphic", 1);
function main() {
  function g() {}
  function f(o) {
    return o.length;
  }
  const v1 = new Int8Array(4);
  f(v1);
  const v2 = new Uint8Array();
  const v3 = Object.create(v2);
  f(v3);
}
main();
gc();

However, if I insert unescape(v3); (or any native call, like print) right before the call f(v3) then it fails to repro. If I call g(v3) before that, it still repros. Posting this here to keep the test in the bug for analysis.

Comment 17

[Phabricator Automation]

Comment on attachment 9196372 [details]
Bug 1685925 - Add a JitOption to force megamorphic ICs. r?iain!

Revision D101345 was moved to bug 1686890. Setting attachment 9196372 [details] to obsolete.

Comment 18

[Phabricator Automation]

Comment on attachment 9196373 [details]
Bug 1685925 - Add tests. r?iain!

Revision D101346 was moved to bug 1686890. Setting attachment 9196373 [details] to obsolete.

Comment 19

[Jan de Mooij [:jandem]]

As suggested by RyanVM, I filed bug 1686890 for landing the follow-up patches.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9196371 [details]
Bug 1685925 - Only tryAttachTypedArrayLength for specialized stubs. r?iain!

Approved for 85.0rc1 and 78.7esr.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a61d27eef45e
https://hg.mozilla.org/releases/mozilla-esr78/rev/0b2a3be4915b

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 23

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20210115035053-0f5e4a3c6f0a.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.