1877357 - Assertion failure: v.isUndefined(), at vm/StringType.cpp:2467

Status: RESOLVED FIXED

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

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

function f(x) {
  let y = x | 0;
  z = 1;
  Function((x | 0 ? 1 : 9999999999) ? (z ? y : 9999999999) : 1);
}
f(1);
f(1);
f(1);
f(1);
f(1);
f(1);
f(1);
f(1);
f(9999999999);
f(1);
f();
f(9007199254740993);

Thread 1 "js-dbg-64-linux" received signal SIGSEGV, Segmentation fault.
js::ToStringSlow<(js::AllowGC)1> (cx=0x7ffff772e100, arg=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/StringType.cpp:2467
2467	    MOZ_ASSERT(v.isUndefined());
(gdb) bt
#0  js::ToStringSlow<(js::AllowGC)1> (cx=0x7ffff772e100, arg=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/StringType.cpp:2467
#1  0x00005555573f8286 in js::ToString<(js::AllowGC)1> (cx=0x7ffff7e31700 <_IO_stdfile_2_lock>, cx@entry=0x7ffff772e100, v=...)
    at /home/skygentoo/trees/mozilla-central/js/src/vm/StringType.h:1892
#2  0x00005555573ef2fb in CreateDynamicFunction (cx=0x7ffff772e100, args=..., generatorKind=js::GeneratorKind::NotGenerator, asyncKind=js::FunctionAsyncKind::SyncFunction)
    at /home/skygentoo/trees/mozilla-central/js/src/vm/JSFunction.cpp:1383
#3  0x00005555573ee929 in js::Function (cx=0x7ffff772e100, argc=<optimized out>, vp=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/vm/JSFunction.cpp:1483
#4  0x000032f6f0cce3b3 in ?? ()
#5  0x00007ffff7795975 in ?? ()
#6  0x00007fffffffc0f8 in ?? ()
#7  0x00007fffffffc1c0 in ?? ()
#8  0x0000000000000000 in ?? ()
(gdb)

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/7ad5c59c746b
user:        Nicolas B. Pierron
date:        Thu Jan 25 16:45:20 2024 +0000
summary:     Bug 1874456 part 2 - Fix EliminateDeadResumePointOperands logic. r=iain

Run with --fuzzing-safe --no-threads --ion-eager, compile with AR=ar sh ../configure --enable-debug --enable-debug-symbols --with-ccache --enable-nspr-build --enable-ctypes --enable-gczeal --enable-rust-simd --disable-tests, tested on m-c rev 2eb7051ff4ed.

Nicolas, is bug 1874456 a likely regressor? Setting s-s to be safe.

Comment 1

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

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

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) from comment #0)

Nicolas, is bug 1874456 a likely regressor? Setting s-s to be safe.

Yes, this sounds likely.
Thank you Gary :)

Comment 3

[Nicolas B. Pierron [:nbp]]

The problem comes from an issue between the ApplyTypes phase and the RemoveDeadResumePointOperands phase.

The ApplyType phase insert a type coercion MIR instruction, which remains as the only live use of one of the BitOr value, leaving all others as resume point uses. Thus RemoveDeadResumePointOperands will replace the captured value by an optimized-out constant.

If we happen to bail sooner than the use of the coerced type, then we are going to attempt to interpret the optimized-out value, and fail.

I do not yet know what would be the best approach to solve this problem.

Comment 4

[Daniel Veditz [:dveditz]]

The code right after the assert we return the value 'undefined' and don't reference the unknown v, so it doesn't look immediately harmful here. But I don't know what happens with the v-of-unexpected-type before or after we get here or how it might be mishandled.

Is this benign, or a sign of impending doom?

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1877357 - Insert coercion instruction in the precedessor.

Previously, when resolving Phi's type, we would insert the coercion instruction
at the end of the block in which the instruction is.

This causes 3 problems:

• With RemoveDeadResumePointOperands, which looks at the live time of
  instructions based on where they are consumed, which is fooled by not following
  the coerced type.

• By increasing the register pressure by keeping multiple value alive while
  only one is needed.

• By potentially adding fallible instruction before it could be guarded by
  precoditions, thus potentially be the source of repeated bailouts.

This patch changes TypeAnalyzer::adjustPhiInputs to insert instructions at the
end of the matching block preceeding the Phi instruction. Doing so should avoid
all 3 issues previously seen.

Comment 7

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

Copying crash signatures from duplicate bugs.

Comment 9

[Nicolas B. Pierron [:nbp]]

Bug 1874456 is about to be backed out, as another bug, which is not solved by the current patch has been found in on the web.
I will open this bug as soon as the other one is backed out.

If you happen to have other failures which are blaming Bug 1874456, such as differential testing issue, I would be happy to try them all.

Comment 10

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

Copying crash signatures from duplicate bugs.

Comment 12

[Andrew McCreight [:mccr8]]

So, is this a security issue? The first thing in comment 5 sounds like it could result in some kind of JIT type confusion, while the other two only sound like performance problems. Thanks.

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Andrew McCreight [:mccr8] from comment #12)

So, is this a security issue? The first thing in comment 5 sounds like it could result in some kind of JIT type confusion, while the other two only sound like performance problems. Thanks.

Not anymore, since the offending patch has been backed out for causing other failure not yet reported by fuzzers.

Now, the worst that could happen is that we would bailout early because one of the branches requires it. I would not expect any of our coercion function to introduce any side-effect.

Comment 14

[Frederik Braun [:freddy]]

Hey, as the release owner for 124, I would like to clarify: If the offending patch has been backed out, can we set 124 as unaffected? If not, we need to get the patch reviewed and landed soon

Comment 15

[Dianna Smith [:diannaS]]

marking 124 as unaffected since the regressor was backed by bug 1876978

Comment 16

[Nicolas B. Pierron [:nbp]]

(In reply to Frederik Braun [:freddy] from comment #14)

Hey, as the release owner for 124, I would like to clarify: If the offending patch has been backed out, can we set 124 as unaffected? If not, we need to get the patch reviewed and landed soon

Yes, I will fix this issue before landing Bug 1874456 again.

Comment 17

[Nicolas B. Pierron [:nbp]]

Iain, can we land this patch, as this still blocks Bug 1874456 and it has a tiny potential for helping with performance and correctness.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e191d0048b49
Insert coercion instruction in the precedessor. r=iain

Comment 19

[Natalia Csoregi [:nataliaCs]]

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

Comment 20

[Daniel Veditz [:dveditz]]

Setting severity based on duplicate bug, and interpreting comment 13 ("not anymore") as confirming the problem existed before the backout. The fact that it was backed out anyway for other regressions doesn't guarantee the re-landing would have been free of this security part, and the patches did include the testcase from the duplicate security bug.

Awarding a split bounty because of the duplicate filed within the window.