1711414 - Assertion failure: !args_->hasLiveDefUses(), at jit/ScalarReplacement.cpp:1526

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20210516-3210b5354d3e (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

function inlined() {
  return inlined.apply(arguments, arguments);
}
new inlined;

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x000055555763b989 in js::jit::ArgumentsReplacer::run() ()
#0  0x000055555763b989 in js::jit::ArgumentsReplacer::run() ()
#1  0x000055555763d7bc in js::jit::ScalarReplacement(js::jit::MIRGenerator*, js::jit::MIRGraph&) ()
#2  0x00005555578d6107 in js::jit::OptimizeMIR(js::jit::MIRGenerator*) ()
#3  0x00005555578de80c in js::jit::CompileBackEnd(js::jit::MIRGenerator*, js::jit::WarpSnapshot*) ()
#4  0x00005555578e00d7 in js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*) ()
#5  0x00005555578e0b48 in IonCompileScriptForBaseline(JSContext*, js::jit::BaselineFrame*, unsigned char*) ()
#6  0x000019f8b7dd18c5 in ?? ()
[...]
#28 0x0000000000000000 in ?? ()
rax	0x555555887a24	93824995588644
rbx	0x7ffff57e9978	140737312102776
rcx	0x555558038748	93825037207368
rdx	0x0	0
rsi	0x7ffff6abd770	140737331844976
rdi	0x7ffff6abc540	140737331840320
rbp	0x7ffffffaec50	140737488022608
rsp	0x7ffffffaebe0	140737488022496
r8	0x7ffff6abd770	140737331844976
r9	0x7ffff7fe3840	140737354020928
r10	0x0	0
r11	0x0	0
r12	0x7ffff57e97b8	140737312102328
r13	0x7ffff57e84f0	140737312097520
r14	0x7ffff57e9978	140737312102776
r15	0x7ffff57e8560	140737312097632
rip	0x55555763b989 <js::jit::ArgumentsReplacer::run()+1209>
=> 0x55555763b989 <_ZN2js3jit17ArgumentsReplacer3runEv+1209>:	movl   $0x5f6,0x0
   0x55555763b994 <_ZN2js3jit17ArgumentsReplacer3runEv+1220>:	callq  0x555556a833da <abort>

Marking s-s until investigated because this assert can be dangerous (but also triggers on OOM sometimes).

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jan de Mooij [:jandem]]

NI Iain because it looks like an arguments + apply edge case.

Comment 3

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20210516091748-3210b5354d3e.
The bug appears to have been introduced in the following build range:

Start: 7b19f4ed5182b81f3ec3f3dee1989d306c00f7e7 (20210323184817)
End: a2e2f181d82070e6bc3a9950a535f29da2fb1813 (20210323184849)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7b19f4ed5182b81f3ec3f3dee1989d306c00f7e7&tochange=a2e2f181d82070e6bc3a9950a535f29da2fb1813

Comment 4

[Iain Ireland [:iain]]

Took a quick look at this to distract myself. The problem is with foo.apply(arguments, ...). ArgumentsReplacer::escapes marks ApplyArgsObj as a replaceable consumer even if the arguments object is being used as this instead of (or in addition to) the second argument. As a result, we end up scalar-replacing ApplyArgsObj (which uses a concrete ArgumentsObject) with ApplyArgs (which uses the arguments right out of the frame), but still having a use of the CreateArgumentsObject afterwards, because the this argument is copied over. This triggers the assertion.

Fortunately, I think it's safe to get this wrong here. The end result is that we just don't optimize away the allocation. The arguments object we create is only passed into the applied function; everything else is scalar replaced. It's possible to write a testcase where we get the wrong result becaus we don't realize that the apply can modify the contents of arguments, so we scalar-replace arguments[0] to read it from the frame instead of the arguments object. For example:

function bar() { this[0] = "overwritten"; }

function foo() {
    bar.apply(arguments, arguments);
    return arguments[0];
}

with ({}) {}
for (var i = 0; i < 100; i++) {
    assertEq(foo("original"), "overwritten");
}

But that's just incorrect, not a security issue, and it's never going to happen in real code.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1711414: Don't scalar replace apply when arguments is this r=jandem!

I verified that the other "replaceable consumer" instructions are either unary or don't accept objects for their other arguments.

This testcase triggers the assertion in a pre-patch debug build, and gives the wrong result in a pre-patch no-debug build.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3076872278f2
Don't scalar replace apply when arguments is this r=jandem

Comment 7

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/3076872278f2

Comment 8

[Bugmon [:jkratzer for issues]]

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