1822966 - Gecko Assertion failure: Assertion failure: producer_ != nullptr

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[zx]

Run following js code

for (let v0 = 0; v0 < 100; v0++) {
    const v2 = [-1000000.0,1000000000.0,4.0,1e-15,992607.2897568105,-839563.687631161];
    v2.h = 2;
    for (let v6 = 0; v6 < 20; v6++) {
        for (let v7 = 0; v7 < 87; v7++) {
            function f9(a10, a11, a12) {
                arguments[255];
                return a11;
            }
            for (let v14 = 0; v14 < 100; v14++) {
                f9();
            }
        }
        for (let v16 = 0; v16 < 100; v16++) {
            function f18() {
                Math.imul();
                return v16;
            }
            f18.apply(2);
        }
        WebAssembly.Memory;
    }
    v2.a = v0;
    const t24 = [1,1,1,1,1];
    t24[1421000090] = 1;
}

Gecko fail

/*
Assertion failure: producer_ != nullptr, at /home/s/gecko-dev/js/src/jit/MIR.h:245
#01: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x274bc88]
#02: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x2ca7b65]
#03: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x2765c83]
#04: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x2717c0a]
#05: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x27769df]
#06: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x2718d3e]
#07: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x2718a11]
#08: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x271d16d]
#09: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x271db1b]
#10: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x271e1ef]
#11: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x271feb5]
#12: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x2b5ace5]
#13: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x2b6a466]
#14: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x2ba9d4a]
#15: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x1bb2fc6]
#16: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x1bb2bf2]
#17: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x1bd2600]
#18: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x1bd21e8]
#19: ???[/home/s/gecko-dev/obj-fuzzbuild/dist/bin/js +0x1be6dd4]
#20: ???[/lib/x86_64-linux-gnu/libc.so.6 +0x94b43]
#21: ???[/lib/x86_64-linux-gnu/libc.so.6 +0x126a00]
#22: ??? (???:???)

Backtrace

#0  js::jit::MUse::producer (this=<optimized out>) at /home/s/gecko-dev/js/src/jit/MIR.h:245
#1  js::jit::MAryInstruction<1ul>::getOperand (this=<optimized out>, index=0) at /home/s/gecko-dev/js/src/jit/MIR.h:1115
#2  0x00005555581fbb65 in js::jit::MDefinition::congruentIfOperandsEqual (this=0x7ffff546cf68, ins=0x7ffff546f760) at /home/s/gecko-dev/js/src/jit/MIR.cpp:474
#3  0x0000555557cb9c83 in js::jit::MConstantProto::congruentTo (this=0x7ffff546cf68, ins=0x7ffff546f760) at /home/s/gecko-dev/js/src/jit/MIR.h:9252
#4  0x0000555557c6bc0a in js::jit::ValueNumberer::VisibleValues::ValueHasher::match (k=0x7ffff546cf68, l=0x7ffff546f760) at /home/s/gecko-dev/js/src/jit/ValueNumbering.cpp:58
#5  0x0000555557cca9df in mozilla::detail::HashTable<js::jit::MDefinition* const, mozilla::HashSet<js::jit::MDefinition*, js::jit::ValueNumberer::VisibleValues::ValueHasher, js::jit::JitAllocPolicy>::SetHashPolicy, js::jit::JitAllocPolicy>::match (aEntry=@0x7ffff664fa88: 0x7ffff546cf68, aLookup=@0x7ffff65fe3b8: 0x7ffff546f760) at /home/s/gecko-dev/obj-fuzzbuild/dist/include/mozilla/HashTable.h:1737
#6  mozilla::detail::HashTable<js::jit::MDefinition* const, mozilla::HashSet<js::jit::MDefinition*, js::jit::ValueNumberer::VisibleValues::ValueHasher, js::jit::JitAllocPolicy>::SetHashPolicy, js::jit::JitAllocPolicy>::lookup<(mozilla::detail::HashTable<js::jit::MDefinition* const, mozilla::HashSet<js::jit::MDefinition*, js::jit::ValueNumberer::VisibleValues::ValueHasher, js::jit::JitAllocPolicy>::SetHashPolicy, js::jit::JitAllocPolicy>::LookupReason)0> (this=this@entry=0x7ffff65fe6e0, aLookup=@0x7ffff65fe3b8: 0x7ffff546f760, aKeyHash=3122858110) at /home/s/gecko-dev/obj-fuzzbuild/dist/include/mozilla/HashTable.h:1793
#7  0x0000555557c6cd3e in mozilla::detail::HashTable<js::jit::MDefinition* const, mozilla::HashSet<js::jit::MDefinition*, js::jit::ValueNumberer::VisibleValues::ValueHasher, js::jit::JitAllocPolicy>::SetHashPolicy, js::jit::JitAllocPolicy>::readonlyThreadsafeLookup (this=0x7ffff65fe6e0, aLookup=@0x7ffff65fe3b8: 0x7ffff546f760) at /home/s/gecko-dev/obj-fuzzbuild/dist/include/mozilla/HashTable.h:2083
#8  mozilla::detail::HashTable<js::jit::MDefinition* const, mozilla::HashSet<js::jit::MDefinition*, js::jit::ValueNumberer::VisibleValues::ValueHasher, js::jit::JitAllocPolicy>::SetHashPolicy, js::jit::JitAllocPolicy>::lookup (this=0x7ffff65fe6e0, aLookup=@0x7ffff65fe3b8: 0x7ffff546f760) at /home/s/gecko-dev/obj-fuzzbuild/dist/include/mozilla/HashTable.h:2088
#9  mozilla::HashSet<js::jit::MDefinition*, js::jit::ValueNumberer::VisibleValues::ValueHasher, js::jit::JitAllocPolicy>::lookup (this=0x7ffff65fe6e0, aLookup=@0x7ffff65fe3b8: 0x7ffff546f760) at /home/s/gecko-dev/obj-fuzzbuild/dist/include/mozilla/HashTable.h:533
#10 js::jit::ValueNumberer::VisibleValues::has (this=0x7ffff65fe6e0, def=0x7ffff546f760) at /home/s/gecko-dev/js/src/jit/ValueNumbering.cpp:113
#11 js::jit::ValueNumberer::discardDef (this=this@entry=0x7ffff65fe6d0, def=def@entry=0x7ffff546f760, allowEffectful=js::jit::ValueNumberer::AllowEffectful::No) at /home/s/gecko-dev/js/src/jit/ValueNumbering.cpp:320
#12 0x0000555557c6ca11 in js::jit::ValueNumberer::processDeadDefs (this=0x7ffff65fe6d0) at /home/s/gecko-dev/js/src/jit/ValueNumbering.cpp:380
#13 js::jit::ValueNumberer::discardDefsRecursively (this=this@entry=0x7ffff65fe6d0, def=def@entry=0x7ffff546f7f8, allowEffectful=<optimized out>) at /home/s/gecko-dev/js/src/jit/ValueNumbering.cpp:251
#14 0x0000555557c7116d in js::jit::ValueNumberer::visitUnreachableBlock (this=this@entry=0x7ffff65fe6d0, block=block@entry=0x7ffff546c8b8) at /home/s/gecko-dev/js/src/jit/ValueNumbering.cpp:974
#15 0x0000555557c71b1b in js::jit::ValueNumberer::visitDominatorTree (this=this@entry=0x7ffff65fe6d0, dominatorRoot=dominatorRoot@entry=0x7ffff545b3b0) at /home/s/gecko-dev/js/src/jit/ValueNumbering.cpp:1058
#16 0x0000555557c721ef in js::jit::ValueNumberer::visitGraph (this=this@entry=0x7ffff65fe6d0) at /home/s/gecko-dev/js/src/jit/ValueNumbering.cpp:1103
#17 0x0000555557c73eb5 in js::jit::ValueNumberer::run (this=<optimized out>, updateAliasAnalysis=<optimized out>) at /home/s/gecko-dev/js/src/jit/ValueNumbering.cpp:1271
#18 0x00005555580aece5 in js::jit::OptimizeMIR (mir=mir@entry=0x7ffff545a178) at /home/s/gecko-dev/js/src/jit/Ion.cpp:1225
#19 0x00005555580be466 in js::jit::CompileBackEnd (mir=0x7ffff545a178, snapshot=0x7ffff545b2c8) at /home/s/gecko-dev/js/src/jit/Ion.cpp:1533
#20 0x00005555580fdd4a in js::jit::IonCompileTask::runTask (this=0x7ffff545b340) at /home/s/gecko-dev/js/src/jit/IonCompileTask.cpp:52
#21 js::jit::IonCompileTask::runHelperThreadTask (this=0x7ffff545b340, locked=...) at /home/s/gecko-dev/js/src/jit/IonCompileTask.cpp:30
#22 0x0000555557106fc6 in js::GlobalHelperThreadState::runTaskLocked (this=this@entry=0x7ffff6609000, task=0x7ffff7c30a60 <_IO_stdfile_2_lock>, locked=...) at /home/s/gecko-dev/js/src/vm/HelperThreads.cpp:2767
#23 0x0000555557106bf2 in js::GlobalHelperThreadState::runOneTask (this=this@entry=0x7ffff6609000, lock=...) at /home/s/gecko-dev/js/src/vm/HelperThreads.cpp:2736
#24 0x0000555557126600 in js::HelperThread::threadLoop (this=this@entry=0x7ffff661c3a0, pool=pool@entry=0x7ffff6617680) at /home/s/gecko-dev/js/src/vm/InternalThreadPool.cpp:282
#25 0x00005555571261e8 in js::HelperThread::ThreadMain (pool=0x7ffff6617680, helper=0x7ffff661c3a0) at /home/s/gecko-dev/js/src/vm/InternalThreadPool.cpp:225
#26 0x000055555713add4 in js::detail::ThreadTrampoline<void (&)(js::InternalThreadPool*, js::HelperThread*), js::InternalThreadPool*&, js::HelperThread*>::callMain<0ul, 1ul> (this=0x7ffff6619270) at /home/s/gecko-dev/js/src/threading/Thread.h:220
#27 js::detail::ThreadTrampoline<void (&)(js::InternalThreadPool*, js::HelperThread*), js::InternalThreadPool*&, js::HelperThread*>::Start (aPack=0x7ffff6619270) at /home/s/gecko-dev/js/src/threading/Thread.h:209
#28 0x00007ffff7aa9b43 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
#29 0x00007ffff7b3ba00 in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
*/

Comment 1

[Jan de Mooij [:jandem]]

I see MConstantProto in the stack trace but it might be a red herring.

Comment 2

[Iain Ireland [:iain]]

I can't reproduce this locally. zx, can you confirm which revision you're testing on, your configuration options, and which shell flags you're using?

Comment 3

[zx]

It crash on HEAD but unstable, you may run it multible time, shell flag: /gecko-dev/obj-fuzzbuild/dist/bin/js --baseline-warmup-threshold=10 --ion-warmup-threshold=100 --ion-check-range-analysis --ion-extra-checks --fuzzing-safe --disable-oom-functions crash.js

Comment 4

[Iain Ireland [:iain]]

Thanks! I confirm that I can reproduce the bug with those options. I will take a look tomorrow.

Comment 5

[Iain Ireland [:iain]]

It took me forever, but I finally managed to get a version of this bug that fails without offthread compilation (which breaks iongraph). There is a very delicate race between OSR compilation and trial inlining. This version fails with only --no-threads:

function bar() { return Math; }

function foo() {
    for (var i = 0; i < 50; i++) {
	const arr = [1];
	const arr2 = [1];
	arr.h = 2;
	for (var j = 0; j < 10; j++) {
	    for (var n = 0; n < 3000; n++) {}

	    for (var k = 0; k < 101; k++) {
		bar();
	    }
	}

	arr.a = i;
	arr2.x = 1;
    }
}
foo()

The rough idea is that the arr and arr2 stuff creates MConstantProto nodes, and all the looping sets up a situation where we trial-inline the call to bar, but OSR-compile before we ever call the inlined version, causes us to generate an MBailout because we have no CacheIR to transpile. Then GVN notices that we will always enter the loop containing the call to bar, which makes various blocks suddenly unreachable because of the bailout, and then while we are deleting those blocks, something explodes in the MConstantProto nodes. Now that I have a more tractable testcase, I will figure out what explodes.

I suspect this will end up not being security-sensitive, but I'll investigate before making a firm conclusion.

Comment 6

[Iain Ireland [:iain]]

Okay, this is a tricky one.

The problem is that MConstantProto is doing some clever tricks, and they all collide with the implementation details of GVN in an awkward way. In this testcase, we generate an assortment of MConstantProto nodes. Each one has an MConstantObject operand, and a receiverObject field pointing to a definition that is guaranteed not to alias with the operand.

In particular, consider two MConstantProto nodes: A, asserting that arr.h = 1 doesn't mutate Object.prototype, and B, asserting the same thing about arr.a = i. A has a guardshape on the newarrayobject itself as its receiver. B's receiver is a guardshape on a phi (with one input ultimately coming from the OSR entry.)

We do GVN. A and B are not congruent with each other, because the receivers aren't the same. They both get inserted into the values_ hashtable in the ValueNumberer. Because of the way we've defined MConstantProto::valueHash, A and B collide, so B is put into the next slot in the hash chain.

As GVN proceeds, we determine that we will always enter the k loop. bar has been transpiled to a bailout, so this breaks the edges between the OSR entry (in the n loop) and other parts of the graph. Suddenly, the store (associated with B) is no longer reachable. We start to discard defs.

The last use of B is discarded. We reach this code and call values_.forget(B). We look B up in the hashtable. We first encounter A. When we initially added them to the hashtable, they didn't match. However, since the edge from the OSR entry was broken, B's phi was simplified away, and now getReceiverObject() returns the same definition for both nodes. We return A from the lookup.

A isn't B, so forget doesn't do anything, assuming that B was never in the hashtable.

We continue discarding B. As part of this process, we set its operands to nullptr.

Later, we look up another MConstantProto up in the values_ table. It's the one that says arr2.x = 1 doesn't mutate Object.prototype. For the same reason as before, it has a hash collision with A and B. It isn't congruent with A. To check whether it's congruent with B, we call getOperand(0) on B, which asserts because it shouldn't be called if the operand is a nullptr.

Doing almost anything "fixes" this bug, since it's so delicate. I'm not entirely sure what the actual best fix is. One possibility is to change valueHash. Another might be to change getReceiverObject to skip object guards when we create the node, instead of when we retrieve the receiver; I'm not sure whether this weakens our optimizations, though.

Doug, any thoughts?

Comment 7

[Steven DeTar [:sdetar]]

Iain, Doug, Do you have updates on this bug?

Comment 8

[Alex Thayer [:alexical] (she/her)]

Yeah. I think the proposed solution of just skipping object guards when we create the node sounds good to me. I'll put together a patch for that. I don't think this is a security problem though.

Another side issue that we discussed is that it feels a little awkward the way we are using the hash table. It feels very uncomfortable that hashtable entries can change from not matching to matching while in the hash table, and this is an issue outside of MConstantProto. We may want to look at that a bit more closely.

Comment 9

[Iain Ireland [:iain]]

Jan and I talked this morning about this, and I convinced myself that the fix is to skip object guards early, and then also include the receiver in the hash. IIRC, we only omitted the receiver from the hash because it would involve calling skipObjectGuards when hashing, which isn't an issue if we skip when we create the node. This bug occurs because three distinct MObjectProto nodes all get hashed to the same bucket in the hashtable. Including the receiver in the hash makes that much more unlikely.

Comment 10

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1822966 - Fix MConstantProto hashtable issues r?iain

Comment 11

[Iain Ireland [:iain]]

Thinking about this bug this morning, I convinced myself that it's not a security issue. The problem here is just that we have a dead definition in the values_ hashtable, which will never be congruent to anything because its operand has been nulled out. That can't go wrong, other than by triggering this assertion.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5e0bf1c32f0
Fix MConstantProto hashtable issues r=iain

Comment 13

[Noemi Erli[:noemi_erli]]

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