1676631 - Assertion failure: !val.isMagic(), at jit/IonIC.cpp:174 or Crash [@ js::jit::IonCacheIRCompiler::compile] or Crash [@ js::jit::IonCacheIRCompiler::emitGuardFrameHasNoArgumentsObject]

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20201111-68867f327c62 (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --baseline-eager --ion-warmup-threshold=0):

function f54() {
  var a90 = arguments;
  for (var i63 = 0; i63 < 10; ++i63) {
    a90[unescape]
  }
}
f54();

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555557a184e4 in js::jit::IonGetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonGetPropertyIC*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) ()
#0  0x0000555557a184e4 in js::jit::IonGetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonGetPropertyIC*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) ()
#1  0x000034d5b3f36cb3 in ?? ()
[...]
#21 0x0000000000000000 in ?? ()
rax	0x5555558261fc	93824995189244
rbx	0xfff9800000000000	-1829587348619264
rcx	0x5555580c8878	93825037797496
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffb130	140737488335152
rsp	0x7fffffffae70	140737488334448
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f998c0	140737353717952
r10	0x58	88
r11	0x7ffff6dac7a0	140737334921120
r12	0x7ffff5688400	140737310655488
r13	0x7ffff6024000	140737320730624
r14	0x0	0
r15	0x7fffffffb640	140737488336448
rip	0x555557a184e4 <js::jit::IonGetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonGetPropertyIC*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>)+4036>
=> 0x555557a184e4 <_ZN2js3jit16IonGetPropertyIC6updateEP9JSContextN2JS6HandleIP8JSScriptEEPS1_NS5_INS4_5ValueEEESB_NS4_13MutableHandleISA_EE+4036>:	movl   $0xae,0x0
   0x555557a184ef <_ZN2js3jit16IonGetPropertyIC6updateEP9JSContextN2JS6HandleIP8JSScriptEEPS1_NS5_INS4_5ValueEEESB_NS4_13MutableHandleISA_EE+4047>:	callq  0x555556ac2316 <abort>

Marking s-s because this assert can indicate JIT security issues.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Crash [@ js::jit::IonCacheIRCompiler::compile(js::jit::IonICStub*)]
Build version: mozilla-central revision 20201111-68867f327c62
Build type: optimized (non-debug)
Runtime options: --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --ion-full-warmup-threshold=0

Testcase:

    setJitCompilerOption('ion.forceinlineCaches', 1);
    function f82() {
      var a97 = arguments;
      for (var i42 = 0; i42 < 1000; ++i42) {
        a97.callee;
      }
    }
    f82();

Backtrace:

    received signal SIGSEGV, Segmentation fault.
    0x00005555560281ce in js::jit::IonCacheIRCompiler::compile(js::jit::IonICStub*) ()
    #0  0x00005555560281ce in js::jit::IonCacheIRCompiler::compile(js::jit::IonICStub*) ()
    #1  0x000055555602a006 in js::jit::IonIC::attachCacheIRStub(JSContext*, js::jit::CacheIRWriter const&, js::jit::CacheKind, js::jit::IonScript*, bool*, js::jit::PropertyTypeCheckInfo const*) ()
    #2  0x000055555602cbff in js::jit::IonGetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonGetPropertyIC*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) ()
    #3  0x00002914978b91f8 in ?? ()
    [...]
    #21 0x0000000000000000 in ?? ()
    rax	0x55555663ef12	93825009970962
    rbx	0x7fffffffa950	140737488333136
    rcx	0x55555765d000	93825026871296
    rdx	0x32	50
    rsi	0x64	100
    rdi	0x7fffffff	2147483647
    rbp	0x7fffffffa840	140737488332864
    rsp	0x7fffffffa6a0	140737488332448
    r8	0x7fffffffb4f8	140737488336120
    r9	0x7ffff560255c	140737310106972
    r10	0x17	23
    r11	0x7fffffffa5a0	140737488332192
    r12	0x55555663e1e4	93825009967588
    r13	0x7fffffffb2e8	140737488335592
    r14	0xfffffffd	4294967293
    r15	0x7fffffffb810	140737488336912
    rip	0x5555560281ce <js::jit::IonCacheIRCompiler::compile(js::jit::IonICStub*)+36590>
    => 0x5555560281ce <_ZN2js3jit18IonCacheIRCompiler7compileEPNS0_9IonICStubE+36590>:	movl   $0x483,0x0
       0x5555560281d9 <_ZN2js3jit18IonCacheIRCompiler7compileEPNS0_9IonICStubE+36601>:	callq  0x5555556d4090 <abort>

Comment 3

[Christian Holler (:decoder)]

Attachment: Testcase for comment 2

Comment 4

[Jan de Mooij [:jandem]]

Maybe from the bailout changes.

Comment 5

[Daniel Veditz [:dveditz]]

I'll put that in as the regressor, but if it turns out to be something else we can replace it.

Comment 6

[Iain Ireland [:iain]]

Oh, that's a gross one.

It's from the bailout changes. Specifically, it's from the patch to disable OSR phi specialization if it's failed before. It turns out that we had a load-bearing bailout loop: OSR specialization was the only thing keeping the arguments optimization correct.

We compile and enter the warp script and store MagicOptimizedArguments in a90 (in a local slot). When we hit a90[unescape] and try to access a non-integer element of arguments, we call argumentsOptimizationFailed, invalidate the warp script, and set needsArgs. Then we try to recompile via OSR. We don't actually remove MagicOptimizedArguments from the local slot, so when we bail out it's still there in the baseline frame.

When we recompile, the a90 slot in the non-OSR entry block is the arguments object. If OSR phi specialization is enabled, then we will insert an optimistic unbox to convert the corresponding OSR value to Object. If it is disabled, then we will instead have a Value phi.

But the OSR value for a90 flowing in from the baseline frame is still MagicOptimizedArguments. In the phi specialization case, the unbox will always fail, and we will get stuck in a bailout loop. In the non-specialized case, as far as I can tell, we don't have anything preventing the magic value from flowing back into the rest of the script.

I'm not sure what the best fix is. In Ion, it looks like we abort in jsop_getelem if we can't prove that obj isn't MagicOptimizedArguments.

Comment 7

[Iain Ireland [:iain]]

Attachment: Bug 1676631: Tweak shouldSpecializeOSRPhis r=jandem

Comment 8

[Iain Ireland [:iain]]

I put up a dumb patch to re-enable the bailout loop in cases where needsArgsObj is set. It's not a good long-term fix, but we can land it to plug the hole if we need to.

Comment 9

[Iain Ireland [:iain]]

Attachment: Bug 1676631: Update local slots in SetFrameArgumentsObject r=jandem

Comment 10

[Iain Ireland [:iain]]

Attachment: Bug 1676631: Add test r=jandem

Depends on D96928

Comment 11

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

https://hg.mozilla.org/integration/autoland/rev/2538dcb17511cc7d2c0f3608a17aefcca41da5e4
https://hg.mozilla.org/mozilla-central/rev/2538dcb17511

Comment 12

[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 13

[Iain Ireland [:iain]]

Comment on attachment 9187516 [details]
Bug 1676631: Update local slots in SetFrameArgumentsObject r=jandem

Beta/Release Uplift Approval Request

• User impact if declined: Potentially exploitable type confusion
• 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): The bug being fixed landed in Nightly prior to the release date, but the fix didn't land until just after release. We should land the same fix in Beta.
• String changes made/needed:

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9187516 [details]
Bug 1676631: Update local slots in SetFrameArgumentsObject r=jandem

Approved for 84.0b3.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/66a7a385e2cb0d15b7a7c0964cdd4607c081742a

Comment 16

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

Add test r=jandem
https://hg.mozilla.org/integration/autoland/rev/ac908d9041f3713d614d4cdf942f23c08c6f921b
https://hg.mozilla.org/mozilla-central/rev/ac908d9041f3

Comment 17

[Bugmon [:jkratzer for issues]]

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