1916581 - Assertion failure: isLowered()

Status: RESOLVED FIXED

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

Description

[:ddme]

Attachment: poc.js

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/130.0.0.0 Safari/537.36

Steps to reproduce:

build with

ac_add_options --enable-project=js
ac_add_options --enable-debug
ac_add_options --disable-optimize

and run with --ion-warmup-threshold=100 --fuzzing-safe --no-threads

working on commit 5f641898e12dafdfb7d54a93f4f3164d2c1642b2

Actual results:

I notice that there are some related assertion failure in several years ago. Feel free to close it if this is not a security releated.

Assertion failure: isLowered(), at js/src/jit/MIR.h:850
bt

#0  0x0000555558dfd064 in js::jit::MDefinition::virtualRegister (this=0x7ffff67ea428) at /root/gecko-dev/js/src/jit/MIR.h:850
#1  0x0000555558e02ae7 in js::jit::LIRGeneratorShared::use (this=0x7fffffff87a0, mir=0x7ffff67ea428, policy=...) at /root/gecko-dev/js/src/jit/shared/Lowering-shared-inl.h:34
#2  0x0000555558e02c6e in js::jit::LIRGeneratorShared::useKeepalive (this=0x7fffffff87a0, mir=0x7ffff67ea428) at /root/gecko-dev/js/src/jit/shared/Lowering-shared-inl.h:563
#3  0x0000555558dfd522 in js::jit::LIRGeneratorShared::useKeepaliveOrConstant (this=0x7fffffff87a0, mir=0x7ffff67ea428) at /root/gecko-dev/js/src/jit/shared/Lowering-shared-inl.h:570
#4  0x0000555558deff26 in js::jit::LIRGeneratorShared::buildSnapshot (this=0x7fffffff87a0, rp=0x7ffff67ea4c0, kind=js::jit::BailoutKind::DuringVMCall) at /root/gecko-dev/js/src/jit/shared/Lowering-shared.cpp:269
#5  0x0000555558df0257 in js::jit::LIRGeneratorShared::assignSafepoint (this=0x7fffffff87a0, ins=0x7ffff6005860, mir=0x7ffff67eade0, kind=js::jit::BailoutKind::DuringVMCall) at /root/gecko-dev/js/src/jit/shared/Lowering-shared.cpp:300
#6  0x00005555591c26ef in js::jit::LIRGenerator::visitLoadTypedArrayElementHole (this=0x7fffffff87a0, ins=0x7ffff67eade0) at /root/gecko-dev/js/src/jit/Lowering.cpp:4767
#7  0x00005555591db0c0 in js::jit::LIRGenerator::visitInstructionDispatch (this=0x7fffffff87a0, ins=0x7ffff67eade0) at /root/gecko-dev/js/src/jit/Lowering.cpp:7590
#8  0x00005555591dede7 in js::jit::LIRGenerator::visitInstruction (this=0x7fffffff87a0, ins=0x7ffff67eade0) at /root/gecko-dev/js/src/jit/Lowering.cpp:7613
#9  0x00005555591df57b in js::jit::LIRGenerator::visitBlock (this=0x7fffffff87a0, block=0x7ffff67e8a58) at /root/gecko-dev/js/src/jit/Lowering.cpp:7683
#10 0x00005555591dfa0d in js::jit::LIRGenerator::generate (this=0x7fffffff87a0) at /root/gecko-dev/js/src/jit/Lowering.cpp:7757
#11 0x00005555590ef6f5 in js::jit::GenerateLIR (mir=0x7ffff67ce180) at /root/gecko-dev/js/src/jit/Ion.cpp:1546
#12 0x00005555590efc19 in js::jit::CompileBackEnd (mir=0x7ffff67ce180, snapshot=0x7ffff67cfc18) at /root/gecko-dev/js/src/jit/Ion.cpp:1635
#13 0x0000555559117289 in js::jit::IonCompile (cx=0x7ffff6936200, script=..., osrPc=0x7ffff69fa1f9 "\230D") at /root/gecko-dev/js/src/jit/Ion.cpp:1762
#14 0x00005555590f0b58 in js::jit::Compile (cx=0x7ffff6936200, script=..., osrFrame=0x7fffffff9fc8, osrPc=0x7ffff69fa1f9 "\230D") at /root/gecko-dev/js/src/jit/Ion.cpp:1916
#15 0x0000555559117aa7 in BaselineCanEnterAtBranch (cx=0x7ffff6936200, script=..., osrFrame=0x7fffffff9fc8, pc=0x7ffff69fa1f9 "\230D") at /root/gecko-dev/js/src/jit/Ion.cpp:2117
#16 0x00005555590f0ff5 in IonCompileScriptForBaseline (cx=0x7ffff6936200, frame=0x7fffffff9fc8, pc=0x7ffff69fa1f9 "\230D") at /root/gecko-dev/js/src/jit/Ion.cpp:2169
#17 0x00005555590f1392 in js::jit::IonCompileScriptForBaselineOSR (cx=0x7ffff6936200, frame=0x7fffffff9fc8, frameSize=0x70, pc=0x7ffff69fa1f9 "\230D", infoPtr=0x7fffffff9f60) at /root/gecko-dev/js/src/jit/Ion.cpp:2281

Comment 1

[Jan de Mooij [:jandem]]

Thanks! I can reproduce this.

Comment 2

[Jan de Mooij [:jandem]]

This is a problem with MInt64ToBigInt. When lowering a resume point, the conversion instruction hasn't been lowered yet because it's later in the MIR graph.

I think the issue is that we push an MInt64ToBigInt instruction on the stack in the transpiler, but then call resumeAfter on the actual instruction before it. For MIonToWasmCall we use resumeAfterUnchecked(postConversion) where postConversion can be the MInt64ToBigInt instruction. Maybe we need to do the same thing here?

I'm not sure yet why this doesn't trigger more often.

Comment 3

[André Bargull [:anba]]

Regression from https://phabricator.services.mozilla.com/D219656. Backing out that part should fix this issue.

Reduced test case:

function blackhole() {
  with ({});
}

const i64 = new BigInt64Array(1);

for (let i = 0; i < 100; i++) {
  let x = Atomics.load(i64, 0);
  let y = i64[1];
  blackhole(x, y);
}

Comment 4

[André Bargull [:anba]]

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

I'm not sure yet why this doesn't trigger more often.

I guess this situation is just too rare to happen more often: It needs an Atomics operations next to MLoadTypedArrayElementHole and then all instructions need to be in a good state so that instruction reordering can move MLoadTypedArrayElementHole before the MInt64ToBigInt emitted for the Atomics operation.

Comment 5

[Jan de Mooij [:jandem]]

That makes sense. It would be nice to handle these conversion steps better with resume points, but for now backing out D219656 seems simplest.

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1916581: Use consistent approach for BigInt conversion. r=jandem!

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1916581: Add a test case. r=jandem!

Comment 8

[BugBot [:suhaib / :marco]]

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

Comment 9

[Andrew McCreight [:mccr8]]

What are the security implications of this? Could it cause type confusion where one case has a pointer or something more benign?

Comment 10

[André Bargull [:anba]]

I couldn't get any further than here (after disabling more MOZ_ASSERT and MOZ_DIAGNOSTIC_ASSERT, we get there with payload->isUse() == true), which would make this a safe crash in release builds. But maybe I'm missing something.

Jan, do you know if there's another part of codegen which would make this not just a safe crash?

Comment 12

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #10)

I couldn't get any further than here (after disabling more MOZ_ASSERT and MOZ_DIAGNOSTIC_ASSERT, we get there with payload->isUse() == true), which would make this a safe crash in release builds. But maybe I'm missing something.

Jan, do you know if there's another part of codegen which would make this not just a safe crash?

MDefinition::virtualRegister() should always return 0 for this instruction (virtualRegister_ is in a union with the load-dependency field that's initialized to nullptr and shouldn't be set for this since it's not a load/store). Virtual register 0 is normally unused and is skipped in installAllocationsInLIR and that's probably why the payload allocation is still of type LUse in codegen.

I agree this looks like a safe MOZ_CRASH or MOZ_DIAGNOSTIC_ASSERT, but it's sketchy enough that we should probably treat this as sec-low or sec-moderate. I went with the latter.

Comment 13

[André Bargull [:anba]]

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

I agree this looks like a safe MOZ_CRASH or MOZ_DIAGNOSTIC_ASSERT, but it's sketchy enough that we should probably treat this as sec-low or sec-moderate. I went with the latter.

Thanks for taking a look at this!

Comment 14

[André Bargull [:anba]]

Attachment: Bug 1916581: Use consistent approach for BigInt conversion. r=jandem!

Original Revision: https://phabricator.services.mozilla.com/D221055

Comment 15

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Can crash through MOZ_CRASH or MOZ_DIAGNOSTIC_ASSERT
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low risk
• Explanation of risk level: Low risk, because its reverts changes from D219656, so we use the known good code again.
• String changes made/needed: N/A
• Is Android affected?: yes

Comment 16

[Dianna Smith [:diannaS]]

:anba anything else pending to land this?

Comment 17

[André Bargull [:anba]]

(In reply to Dianna Smith [:diannaS] from comment #16)

:anba anything else pending to land this?

No. Queued D221055 for landing. Will land the test case at a later point of time.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27b78a5c0603
Use consistent approach for BigInt conversion. r=jandem

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/27b78a5c0603

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/c8f3c6446d57

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3184eb76c817
Add a test case. r=jandem

Comment 22

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/3184eb76c817