1772824 - Assertion failure: LookupPropertyPure(cx, obj, key, &pobj, &prop), at jit/VMFunctions.cpp:1605

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20220604-490469b53dbe (debug build, run with --fuzzing-safe --no-threads --baseline-warmup-threshold=0):

tryItOut = function(code) {
  try { evaluate(code); } catch(exc) {}
}
tryItOut(`
  o0='';
  a1=[];
  o1={};
  Object.defineProperty(a1,6, {
    get:(function() {
      eval();
      o0+=1;
    })
  })
  Array.prototype.join.apply(a1);
  n();
`)
tryItOut(`
  a=Array.prototype.concat.call(a1);
  Array.prototype.pop.call(a1);
`)
tryItOut(`
  Array.prototype.pop.apply(a1)
`)
tryItOut(`
  t=new Uint32Array(a1);
  Array.prototype.reverse.call(a1);
`)
tryItOut(`
  throw;
`)
tryItOut(`
  Array.prototype.pop.call(a1,o1.s);
`)
tryItOut(`
  const o0=0;
  a1.some(function() {});
`)
a1.shift(o1.a);

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555557799468 in js::jit::VerifyCacheEntry(JSContext*, js::NativeObject*, JS::PropertyKey, js::MegamorphicCache::Entry const&) ()
#0  0x0000555557799468 in js::jit::VerifyCacheEntry(JSContext*, js::NativeObject*, JS::PropertyKey, js::MegamorphicCache::Entry const&) ()
#1  0x00005555577a2be4 in js::jit::GetNativeDataPropertyPure(JSContext*, js::NativeObject*, JS::PropertyKey, JS::Value*) ()
#2  0x00003aa41cfa015f in ?? ()
#3  0x00007fffffffb408 in ?? ()
#4  0x00007fffffffb378 in ?? ()
#5  0x0000000000000000 in ?? ()
rax	0x5555558c41ae	93824995836334
rbx	0xfe	254
rcx	0x5555581e8838	93825038977080
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffb2c0	140737488335552
rsp	0x7fffffffb270	140737488335472
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f99840	140737353717824
r10	0x0	0
r11	0x0	0
r12	0x7ffff6018000	140737320681472
r13	0x2067f4401528	35630851560744
r14	0x243	579
r15	0x370f564123c0	60539011146688
rip	0x555557799468 <js::jit::VerifyCacheEntry(JSContext*, js::NativeObject*, JS::PropertyKey, js::MegamorphicCache::Entry const&)+632>
=> 0x555557799468 <_ZN2js3jitL16VerifyCacheEntryEP9JSContextPNS_12NativeObjectEN2JS11PropertyKeyERKNS_16MegamorphicCache5EntryE+632>:	movl   $0x645,0x0
   0x555557799473 <_ZN2js3jitL16VerifyCacheEntryEP9JSContextPNS_12NativeObjectEN2JS11PropertyKeyERKNS_16MegamorphicCache5EntryE+643>:	callq  0x555556be1628 <abort>

Marking s-s because this is a JIT-related assert and the test behaves very strangely when reducing.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20220606154314-533ad0ead234.
The bug appears to have been introduced in the following build range:

Start: b07f125d09fd9f3361398f1f3ac05fd898398a22 (20220211101702)
End: eb89031cbeda73e8987aec0713190d3fca0c6194 (20220211101730)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b07f125d09fd9f3361398f1f3ac05fd898398a22&tochange=eb89031cbeda73e8987aec0713190d3fca0c6194

Comment 4

[Iain Ireland [:iain]]

Eventually managed to reduce to this (run with --baseline-eager):

function foo() { eval(); boundName += 1; }

// Attach a megamorphic GetBoundName in foo.
boundName = 0;
for (var i = 0; i < 10; i++) {
  eval("var x" + i + " = 0;");
  foo();
}

// Redefine variable as const
evaluate(`
  const boundName = 0;
  for (var i = 0; i < 2; i++) {
    try { foo(); } catch {}
  }
`);

To trigger the bug, we first need to create a megamorphic GetBoundName. Then we arrange to pass a RuntimeLexicalErrorObject into that IC.
The megamorphic IC calls GetNativeDataPropertyPure. The error object does not have the property, ClassMayResolveId returns false, and it doesn't have a prototype, so we call initEntryForMissingProperty to add an entry to the megamorphic cache.

On the second iteration, we call GetNativeDataPropertyPure again. We hit in the cache. VerifyCacheEntry checks that LookupPropertyPure succeeds; however, the RuntimeLexicalErrorObject class has an OpsLookupProperty callback, so LookupPropertyPure returns false and we assert.

I'm not entirely sure where the bug is here. On the one hand, I couldn't find a way to trigger this bug without using the evaluate testing function; using Object.defineProperty to make boundName unwritable isn't enough. On the other hand, it doesn't seem right that the megamorphic cache will cache objects with an OpsLookupProperty callback. There's also a weird interaction between the split lookup behaviour of BindName/GetBoundName (for the compound assignment) and the delayed throw of RuntimeLexicalErrorObject.

I don't think this particular bug is exploitable, but I haven't looked at all the other object classes with OpsLookupProperty callbacks, so it's probably best to keep this closed for now.

Jan, any ideas?

Comment 5

[Christian Holler (:decoder)]

(In reply to Iain Ireland [:iain] from comment #4)

I couldn't find a way to trigger this bug without using the evaluate testing function

As far as I know, the semantics of having multiple script tags in the browser is roughly equivalent to having multiple evaluate calls. Not sure if it has the same properties here, but it is otherwise hard to simulate this in the JS shell.

Comment 6

[Jan de Mooij [:jandem]]

Thanks for the analysis!

I think we want to stop using the megamorphic cache for JSOp::GetBoundName. It's a weird and uncommon op that accesses environment objects. Normal native objects exposed to script shouldn't have a lookupProperty hook. We definitely should start asserting this somewhere though (bug 1393701...), I'll look into that this week.

Comment 7

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

Setting regressed_by field after analyzing regression range found by bugmon.

Comment 8

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

Set release status flags based on info from the regressing bug 1754837

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1772824 - Don't use the megamorphic cache for GetBoundName. r?iain!

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Don't use the megamorphic cache for GetBoundName. r=iain
https://hg.mozilla.org/integration/autoland/rev/a105c1f1ff2a99a6f7921d63bf415e842a3e26a7
https://hg.mozilla.org/mozilla-central/rev/a105c1f1ff2a

Comment 11

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20220624214101-340fab1c7c24.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR102 approval when you get a chance.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 9282538 [details]
Bug 1772824 - Don't use the megamorphic cache for GetBoundName. r?iain!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Disables an optimization that can be invalid in some edge cases. Uplifting to ESR 102 should be safe and low risk.
• User impact if declined: Correctness issues in some edge cases.
• Fix Landed on Version: 103
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Low risk because it just disables a particular optimization in an uncommon case.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9282538 [details]
Bug 1772824 - Don't use the megamorphic cache for GetBoundName. r?iain!

Approved for 102.1esr.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/da73c70e614d