Closed Bug 1812979 Opened 2 years ago Closed 1 year ago

Assertion failure: whyMagic() == why, at js/Value.h:863

Categories

(Core :: JavaScript Engine: JIT, defect)

defect

Tracking

()

RESOLVED FIXED
111 Branch
Tracking Status
firefox111 --- fixed

People

(Reporter: lukas.bernhard, Assigned: iain)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Steps to reproduce:

The attached sample crashes on commit 3a13c5ad9c1dbd065e484a399af75eb98c235def with Assertion failure: whyMagic() == why.
I'm precautiously marking this as s-s.

commandline:

obj-x86_64-pc-linux-gnu/dist/bin/js --baseline-warmup-threshold=10 --fuzzing-safe --ion-warmup-threshold=100 crash.js
function f1(a2) { 
    if (a2 == 99) {
        const v6 = [];  
        v6.sameZoneAs = Float32Array;
        this.enableTrackAllocations(v6);
        const v10 = this.newGlobal(v6).Debugger;
        v10(Float32Array).getNewestFrame().older;
    }
}
for (let v14 = 0; v14 < 100; v14++) {
    for (const v16 in "aaaaaaaaaaaaaaaaaa") { } 
    f1(v14);
}
#0  0x000055555752ab68 in JS::Value::isMagic (this=0x7fffffffab68, why=JS_ION_ERROR)
    at obj-x86_64-pc-linux-gnu/dist/include/js/Value.h:863
#1  0x00005555577671ed in js::WrappedPtrOperations<JS::Value, JS::Rooted<JS::Value>, void>::isMagic (this=0x7fffffffab58, why=JS_ION_ERROR)
    at obj-x86_64-pc-linux-gnu/dist/include/js/Value.h:1332
#2  0x000055555881603d in EnterBaseline (cx=0x7ffff742f100, data=...)
    at js/src/jit/BaselineJIT.cpp:160
#3  0x0000555558815640 in js::jit::EnterBaselineInterpreterAtBranch (cx=0x7ffff742f100, 
    fp=0x7ffff4cfc020, pc=0x7ffff74fdaf5 "\224\003")
    at js/src/jit/BaselineJIT.cpp:198
#4  0x00005555576d645f in Interpret (cx=0x7ffff742f100, state=...)
    at js/src/vm/Interpreter.cpp:2219
#5  0x00005555576d49d0 in js::RunScript (cx=0x7ffff742f100, state=...)
    at js/src/vm/Interpreter.cpp:431
#6  0x00005555576f39cc in js::ExecuteKernel (cx=0x7ffff742f100, script=..., envChainArg=..., 
    evalInFrame=..., result=...)
    at js/src/vm/Interpreter.cpp:812
#7  0x00005555576f4275 in js::Execute (cx=0x7ffff742f100, script=..., envChain=..., rval=...)
    at js/src/vm/Interpreter.cpp:844
#8  0x000055555792b266 in ExecuteScript (cx=0x7ffff742f100, envChain=..., script=..., 
    rval=...) at js/src/vm/CompilationAndEvaluation.cpp:473
#9  0x000055555792b3bd in JS_ExecuteScript (cx=0x7ffff742f100, scriptArg=...)
    at js/src/vm/CompilationAndEvaluation.cpp:497
Group: firefox-core-security → core-security
Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core
Group: core-security → javascript-core-security

NI Iain because of that for-in, but I don't know if it's related to work in that area...

Flags: needinfo?(iireland)

This is debugger-only, and unrelated to for-in. The problem is that rematerialized frames don't handle the noRval case properly. Here's a reduced version of the testcase (run with --fast-warmup --no-threads):

function foo(n) {
  with ({}) {}
  if (n == 9) {
    enableTrackAllocations();
    var g = newGlobal({newCompartment: true});
    var dbg = g.Debugger(this);
    dbg.getNewestFrame().older;
  }
}
for (var i = 0; i < 10; i++) {
  for (var j = 0; j < 20; j++) {}
  foo(i);
}

We Ion-compile the outermost script. It's an OSR compilation for the inner loop, and the entry point of the script can't reach the OSR entry (because we don't have any CacheIR for the SetGName for i = 0). Phi elimination notices that the Phi for the return value is never used (because this is a top-level script) and converts it to MagicOptimizedOut.

On the last iteration of the outer loop, we call enableTrackAllocations, which discards all jitcode, invalidating the top-level IonScript. Then we use the debugger to create a rematerialized frame for that IonScript. The return value of the rematerialized frame is set to MagicOptimizedOut.

When we return, we bail out because the frame is invalidated. In CopyFromRematerializedFrame, we set the baseline frame's return value to MagicOptimizedOut, based on the rematerialized frame's return value. Calling setReturnValue on the baseline frame sets the HasRval flag.

Then we hit the JSOp::RetRval at the end of the script. We're in blinterp, because we just bailed out, so we rely on HasRval to decide whether we should return UndefinedValue or the return value stored in the frame.

Finally, we assert in EnterBaseline because the only returnable magic value should be JS_ION_ERROR.

We can fix this by not setting the return value in CopyFromRematerializedFrame when the script has no rval, similar to what we already do during bailouts.

This isn't security-sensitive: it depends on the debugger, and at worst it involves us "returning" a magic value from a top-level script, which doesn't do anything.

Group: javascript-core-security
Flags: needinfo?(iireland)

The only other place where we get the return value from a rematerialized frame is in OnLeaveIonFrame. The top level frame obviously can't be a generator. I tried and failed to write a testcase that would force a return from a top-level Ion frame, but we always seem to bail out to baseline first. I'm not convinced it's possible; if it is, then eventually the fuzzer will figure out a testcase.

Assignee: nobody → iireland
Status: NEW → ASSIGNED
Pushed by iireland@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0990112db9d3
Don't copy return value into top-level baseline frames r=jandem
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 111 Branch
Duplicate of this bug: 1813565
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: