1885774 - js::gc::HeaderWord::getAtomic (this=0x4000000000000) at gc/Cell.h:100

Status: RESOLVED FIXED

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

Description

[lukas.bernhard]

Steps to reproduce:

On git commit 6d5114b3ba4e5c3414a19419ca1d0170ca149b13 the attached sample crashes the js-shell while dereferencing 0x4000000000000 when invoked as obj-x86_64-pc-linux-gnu/dist/bin/js --fast-warmup --fuzzing-safe crash.js
The crash is flakey, if the crash does not manifesting the shell logs: TypeError: illegal operation attempted on a revoked proxy

The issue might not actually be s-s, see https://bugzilla.mozilla.org/show_bug.cgi?id=1851416#c9

const v22 = new Proxy({}, {});
const v24 = Proxy.revocable({}, {});
v24.revoke();
for (const v28 of [v22, v24.proxy]) {
    function f31() {
        for (let i33 = 0; i33 < 50; i33++) {
            v28.length;
        }
        for (let i43 = 0; i43 < 50; i43++) {
        }
    }
    function f54() {
        for (let i56 = 0; i56 < 50; i56++) {
            const o62 = {
            };
            function f63() {
            }
            f63.bind(o62).length;
        }
        for (let i68 = 0; i68 < 50; i68++) {
            function f75() {
            }
            f75.bind({}).length;
        }
    }
    f31();
    f54();
}

#0  js::gc::HeaderWord::getAtomic (this=0x4000000000000) at js/src/gc/Cell.h:100
#1  0x00005555578fb655 in js::gc::HeaderWord::flags (this=0x4000000000000) at js/src/gc/Cell.h:116
#2  0x00005555578fb595 in js::gc::Cell::flags (this=0x4000000000000) at js/src/gc/Cell.h:154
#3  0x00005555578fb502 in js::gc::CellWithTenuredGCPointer<js::gc::Cell, js::Shape>::headerPtr (this=0x4000000000000)
    at js/src/gc/Cell.h:797
#4  0x00005555578fb4a5 in JSObject::shape (this=0x4000000000000) at js/src/vm/JSObject.h:93
#5  0x00005555578fb2d5 in JSObject::compartment (this=0x4000000000000) at js/src/vm/JSObject.h:145
#6  0x0000555558b652f2 in js::jit::AssertValidObjectPtr (cx=0x7ffff7639100, obj=0x4000000000000)
    at js/src/jit/VMFunctions.cpp:1374
#7  0x00002b050c01b09f in ?? ()
#8  0x0ff44def78192f00 in ?? ()
#9  0x00007fffffff9df8 in ?? ()
#10 0x0ff44def78192f00 in ?? ()
#11 0xaaaaaaaaaaaaaaaa in ?? ()

Bisecting the issue points to bug 1824051.

Comment 1

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

:alexical, since you are the author of the regressor, bug 1824051, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Jan de Mooij [:jandem]]

Iain will take a look at this one.

Comment 3

[Iain Ireland [:iain]]

I confirm that this is crashing cleanly while dereferencing a fixed invalid pointer. We load, unbox, and dereference the wrapper target of a revoked proxy, which has been set to NullValue. Unboxing Null will always give 0x4000000000000, which has a high bit set and will never be a valid address, so we will always crash safely.

We transpile this CacheIR for v28.length:

  WarpCacheIR (offset 45, JSOp::GetProp)
    stubCode: 0x26398da67088
    stubInfo: 0x7fdd322e91c0
    stubData: 0x7fdd32294300
    IR:
      GuardToObject                 inputId 0
      GuardIsProxy                  objId 0
      GuardHasProxyHandler          objId 0, handlerOffset 0
      LoadScriptedProxyHandler      resultId 1, objId 0
      GuardToObject                 inputId 1
      LoadWrapperTarget             objId 0, resultId 2
      GuardIsNativeObject           objId 2
      GuardShape                    objId 1, shapeOffset 8
      LoadProtoObject               resultId 3, protoObjOffset 16, receiverObjId 1
      GuardShape                    objId 3, shapeOffset 24
      MegamorphicLoadSlotResult     objId 2, nameOffset 32
      ReturnFromIC                  

We load the scripted proxy handler, guard that it is an object, and then load the wrapper target. We added the GuardToObject in bug 1851416 to handle revoked proxies. It mostly works, but in this case LICM messes us up. During Ion optimization, we hoist the LoadWrapperTarget outside the loop, but we leave the LoadScriptedProxyHandler and the Unbox inside the loop. When we hit this code with a revoked proxy, we try loading the target before we've guarded that the proxy hasn't been revoked. In debug builds, we catch this using the checks inserted here. In a release build, it crashes in the GuardIsNativeObject, which we also hoist.

This consistently crashes with a known safe value, so I'm going to open it up.

I confirm that the bisection found the right regressor.

Tomorrow I'll try writing a simpler testcase and think about the best fix.

Comment 4

[Iain Ireland [:iain]]

Here's a clean testcase:

// |jit-test| --no-threads; --fast-warmup

var {proxy, revoke} = Proxy.revocable({x:1}, {});

function foo(o) {
  var res = 0;
  for (var i = 0; i < 10; i++) {
    res += o.x;
  }
  return res;
}

for (var i = 0; i < 1000; i++) {
  assertEq(foo(proxy), 10);
}

revoke();
try {
  foo(proxy);
} catch {}

I think the best fix is probably to have LoadScriptedProxyHandler check the type internally and return an object instead of a value. That prevents the operations from being split up (and should generate slightly tighter code as well).

Comment 5

[Iain Ireland [:iain]]

Never mind, that solution doesn't work: the LoadScriptedProxyHandler wasn't being hoisted anyway, so checking the type internally doesn't solve the problem.

Instead it's probably best to just use a fallible unbox inside LoadWrapperTarget.

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1885774: Make loadWrapperTarget fallible r=jandem

Comment 7

[Iain Ireland [:iain]]

Attachment: Bug 1885774: Check for revoked proxies inside LoadScriptedProxyHandler r=jandem

This is an optional patch. I wrote it before I realized that it didn't fix the bug. I think it might still be a tiny improvement, because we can unbox directly from memory instead of loading the value into a register first.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c0812706a34
Make loadWrapperTarget fallible r=jandem
https://hg.mozilla.org/integration/autoland/rev/90c630b807ff
Check for revoked proxies inside LoadScriptedProxyHandler r=jandem

Comment 9

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/7c0812706a34
https://hg.mozilla.org/mozilla-central/rev/90c630b807ff

Comment 10

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

The patch landed in nightly and beta is affected.
:iain, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox125 to wontfix.

For more information, please visit BugBot documentation.

Comment 11

[Iain Ireland [:iain]]

This is not exploitable. It can ride the trains.