1845270 - Assertion failure: IsUninitializedLexical(lexicalEnv->getSlot(prop->slot())), at vm/Interpreter-inl.h:288

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[lukas.bernhard]

Steps to reproduce:

On git commit d6960b4e32d09bff32865694e32384eb9bca4af5 the attached sample asserts when invoked as obj-x86_64-pc-linux-gnu/dist/bin/js --baseline-warmup-threshold=10 --ion-warmup-threshold=100 --fuzzing-safe --disable-oom-functions crash.js

function f0(a1, a2) {
    const v5 = []; 
    v5.sameZoneAs = v5; 
    const t5 = this.newGlobal(v5).Debugger;
    const v9 = t5(v5);
    const v12 = v9.getNewestFrame().callee.environment;
    try { v12.setVariable(a2, a2); } catch (e) {}
    const v14 = v12.names(f0, f0, v9, v9);
    try { v14.toSorted(a1); } catch (e) {}
}
f0(f0);

class C18 extends WeakMap {
}

class C29 {
}

#0  js::InitGlobalLexicalOperation (cx=<optimized out>, lexicalEnv=0x3739f643f038, script=0x3739f6464150,
    pc=<optimized out>, value=...) at js/src/vm/Interpreter-inl.h:288
#1  0x000055555709ce6b in js::Interpret (cx=0x7ffff79f8a20 <_IO_stdfile_2_lock>, state=...)
    at js/src/vm/Interpreter.cpp:3775
#2  0x0000555557080bbb in MaybeEnterInterpreterTrampoline (cx=0x7ffff79f8a20 <_IO_stdfile_2_lock>, 
    cx@entry=0x7ffff602e100, state=...) at js/src/vm/Interpreter.cpp:400
#3  0x00005555570808aa in js::RunScript (cx=cx@entry=0x7ffff602e100, state=...)
    at js/src/vm/Interpreter.cpp:458
#4  0x00005555570856f2 in js::ExecuteKernel (cx=cx@entry=0x7ffff602e100, script=script@entry=..., 
    envChainArg=envChainArg@entry=..., evalInFrame=evalInFrame@entry=..., result=...)
    at js/src/vm/Interpreter.cpp:845
#5  0x0000555557085e5d in js::Execute (cx=cx@entry=0x7ffff602e100, script=script@entry=..., envChain=..., 
    rval=rval@entry=...) at js/src/vm/Interpreter.cpp:877
#6  0x000055555726330a in ExecuteScript (cx=cx@entry=0x7ffff602e100, envChain=..., script=..., 
    rval=rval@entry=...) at js/src/vm/CompilationAndEvaluation.cpp:493
#7  0x00005555572635e0 in JS_ExecuteScript (cx=cx@entry=0x7ffff602e100, scriptArg=scriptArg@entry=...)
    at js/src/vm/CompilationAndEvaluation.cpp:517
#8  0x0000555556fb1f96 in RunFile (cx=0x7ffff602e100, filename=<optimized out>, file=<optimized out>, 
    compileMethod=CompileUtf8::DontInflate, compileOnly=false, fullParse=<optimized out>)
    at js/src/shell/js.cpp:1102
#9  0x0000555556fb148d in Process (cx=cx@entry=0x7ffff602e100, filename=0x0, forceTTY=<optimized out>, 
    kind=kind@entry=FileScript) at js/src/shell/js.cpp:1682
#10 0x0000555556f6b993 in ProcessArgs (cx=0x7ffff602e100, op=0x7fffffffdcb8)
    at js/src/shell/js.cpp:10743
#11 Shell (cx=0x7ffff602e100, op=op@entry=0x7fffffffdcb8)
    at js/src/shell/js.cpp:10967
#12 0x0000555556f65761 in main (argc=<optimized out>, argv=0x7fffffffdf48)
    at js/src/shell/js.cpp:11399

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

So, this test case boils down pretty simply -- shocked we haven't seen the like before and fixed this previously.

// Add this as js/src/jit-test/tests/debug/Environment-setVariable-16.js
(function () {
    const otherDebugger = newGlobal({ sameZoneAs: this }).Debugger;
    const dbg = otherDebugger(this);

    const env = dbg.getNewestFrame().callee.environment;
    ran = false;
    try {
        env.setVariable("tdz_variable", 10);
        ran = true;
    } catch (e) { }
    assertEq(ran, false);
})();

// We crash when initializing this because it was set above
// while this was in TDZ.
let tdz_variable = 10;

I have a trivial fix patch written, which gets the old value of a property in DebuggerEnvironmnet::setVariable and throws if we're trying to overwrite an uninitialized lexical.

However, this doesn't play nicely with our support for optimized out issues: debug/Environment-setVariable-13.js throws on the GetProperty call; then the error is copied by ErrorCopier, but it loses its identity as a ReferenceError, and so the test cases fails as it checks for error identity.

On further investigation the actual place this seems like it ought to be handled is inside of DebugEnvironmentProxyHandler; as near as I can tell right about here, but I can't make heads or tails of how to correctly check the status of an existing binding to ensure it's not an unintialized lexical when we get to this point.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Arai, you might be more up to date on how we could check the binding here (or perhaps have a different opinion for how to fix this)

Comment 3

[Tooru Fujisawa [:arai]]

Yeah, it should be handled in DebugEnvironmentProxyHandler::handleUnaliasedAccess, in the same way as handling assignment for const.

https://searchfox.org/mozilla-central/rev/3c7b40d1d74c26a82486f38b5828c3f3a43e05da/js/src/vm/EnvironmentObject.cpp#1558-1561,1683-1686

if (action == SET && bi.kind() == BindingKind::Const) {
  ReportRuntimeLexicalError(cx, JSMSG_BAD_CONST_ASSIGN, id);
  return false;
}
...
if (action == SET && bi.kind() == BindingKind::Const) {
  ReportRuntimeLexicalError(cx, JSMSG_BAD_CONST_ASSIGN, id);
  return false;
}

We could add BaseAbstractBindingIter::isLexical method or something, and check the current value before setting if it's lexical.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

There's a few other test cases to do:

1. The TDZ variable isn't in the global scope (and thus the lexical environment object isn't extensible)
2. The TDZ variable is captured and therefore part of an environment chain.

These cases should probably all universally throw; right now however it's a mishmash. The first throws due to optimized out, the second succeeds despite the TDZ (but doesn't crash).

Unfortunately, I don't think there's a single codepath that catches all these cases.

I'm going to put this on pause, though I will upload a patch with the test cases I've written

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1845270 - Add test cases for uninitalized lexical variable setting r?arai

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

Ok -- I lied about putting this on pause; I have a partial patch

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1845270 - Block the debugger from writing to uninitialized lexicals r?arai

Depends on D184637

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6805ee188278
Block the debugger from writing to uninitialized lexicals r=arai

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

(erm. I feel like lando did a weird thing here. Anyhow. Triggering tests to land too)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/195161a06a7a
Add test cases for uninitalized lexical variable setting r=arai

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/6805ee188278
https://hg.mozilla.org/mozilla-central/rev/195161a06a7a