1670378 - Crash [@ JS_TransplantObject] with OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20201008-e88890094825 (opt build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

evalInWorker(`
  Object.defineProperty(this, "u", {
        x: 1, y: 2, z: 3, a: 4,
        b: 1, c: 2, d: 3, e: 4,
      log: function(s) { return s.toString(); },
  });
let lfInitMod = parseModule("export default function x() { return 0; }");
try { evaluate("/*"); } catch(exc) {}
const thisGlobal = this;
const otherGlobalSameCompartment = newGlobal({sameCompartmentAs: thisGlobal});
const otherGlobalNewCompartment = newGlobal({newCompartment: true});
const globals = [thisGlobal, otherGlobalSameCompartment, otherGlobalNewCompartment];
function testProperties(global, count) {
  let {object: source, transplant} = transplantableObject();
  transplant(global);
}
for (let global of globals) {
  for (i2 = 0; i2 < 100000; i2++) {
    count = new WeakSet();
    testProperties(global, count);
  }
}
`)

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555560d6d07 in JS_TransplantObject(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) ()
#1  0x0000555556077d33 in TransplantObject(JSContext*, unsigned int, JS::Value*) ()
#2  0x0000555555ce8a35 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) ()
#3  0x0000555555f2bd0a in js::jit::InvokeFunction(JSContext*, JS::Handle<JSObject*>, bool, bool, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) ()
#4  0x0000116d2e469d0d in ?? ()
[...]
#13 0x0000000000000000 in ?? ()
rax	0x555556678cc0	93825010207936
rbx	0x7ffff6089018	140737321144344
rcx	0x555557732f38	93825027747640
rdx	0x7ffff55fd948	140737310087496
rsi	0x7ffff60ea000	140737321541632
rdi	0x0	0
rbp	0x7ffff55fd9b0	140737310087600
rsp	0x7ffff55fd8f0	140737310087408
r8	0x0	0
r9	0x5555566d8490	93825010599056
r10	0x7ffff5702000	140737311154176
r11	0x246	582
r12	0x7ffff6089000	140737321144320
r13	0x7ffff561bd80	140737310211456
r14	0x7ffff5aea000	140737315250176
r15	0x7ffff55fd958	140737310087512
rip	0x5555560d6d07 <JS_TransplantObject(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>)+1815>
=> 0x5555560d6d07 <_Z19JS_TransplantObjectP9JSContextN2JS6HandleIP8JSObjectEES5_+1815>:	movl   $0x307,0x0
   0x5555560d6d12 <_Z19JS_TransplantObjectP9JSContextN2JS6HandleIP8JSObjectEES5_+1826>:	callq  0x55555567e600 <abort>

I think this is hitting a MOZ_CRASH but the testcase is highly fragile and only reproduces in opt builds. Changing it slightly will result in an unhandlable oom crash instead. I was not able to find a better test for this so far. Marking s-s until investigated.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20201009214545-cad2c1678593.
Failed to bisect testcase (Start build crashes!):

Start: ac63c8962183502a4b0ec32222efc67d3841d157 (20191107230801)
End: e88890094825db821835b2f173e1e16eae3f51af (20201008094950)
BuildFlags: BuildFlags(asan=False, tsan=False, debug=False, fuzzing=False, coverage=False, valgrind=False)

Comment 3

[Andrew McCreight [:mccr8]]

Do you know which MOZ_CRASH is it hitting?

Comment 4

[Christian Holler (:decoder)]

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

Do you know which MOZ_CRASH is it hitting?

No, because I cannot reproduce this in a debug build and the opt build doesn't have the required symbols. I could try doing a local build with --disable-debug and symbols, if it is important.

Comment 5

[Jason Orendorff [:jorendorff]]

For me, it's hitting this one. But I don't think it matters.

Any kind of failure during JS_TransplantObject is unrecoverable. We have a comment in jsapi.cpp:

/*
 * [SMDOC] Brain transplants.
 *
 * Not for beginners or the squeamish.
[54 lines omitted]
 *
 * We don't have a good way to recover from failure in this function, so
 * we intentionally crash instead.
 */

I'm guessing the test case is fragile because it's causing one particular allocation to fail "by chance", and oomTest could be used to trigger this more reliably.

Christian, this isn't a bug for us. Is it a fuzzblocker? How do you want us to fix it?

• We can add a message to these MOZ_CRASH calls, so that the fuzzer can recognize it and treat such crashes as acceptable.

• Or, we can move transplantableObject to the unsafe function list.

Comment 6

[Christian Holler (:decoder)]

(In reply to Jason Orendorff [:jorendorff] from comment #5)

Christian, this isn't a bug for us. Is it a fuzzblocker? How do you want us to fix it?

Yes, this is a crasher and needs to be fixed.

• We can add a message to these MOZ_CRASH calls, so that the fuzzer can recognize it and treat such crashes as acceptable.

If these are OOMs, they should include the appropriate message like in https://searchfox.org/mozilla-central/rev/e0eb861a187f0bb6d994228f2e0e49b2c9ee455e/js/src/vm/JSContext.cpp#1178

Including this tag in the MOZ_CRASH message is not sufficient because that message is not emitted for opt builds but calling AutoEnterOOMUnsafeRegion::crash should do the trick.

• Or, we can move transplantableObject to the unsafe function list.

I'd rather not do that, because I remember that we found other (legit) bugs with this before.

Comment 8

[Tom Ritter [:tjr] (OOTO until April)]

Attachment: autobisect output.txt

I ran this through autobisect to learn how to use it, and got the attached.

[2020-11-06 10:55:05] Reduced build range to:
[2020-11-06 10:55:05] > Start: 256602905ce82b1866a1d2f37b409fa768fb7ae6 (20200513145103)
[2020-11-06 10:55:05] > End: d1114574b777bb00b85f73733a8bd6a78d2ad87e (20200513215059)
[2020-11-06 10:55:05] > https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=256602905ce82b1866a1d2f37b409fa768fb7ae6&tochange=d1114574b777bb00b85f73733a8bd6a78d2ad87e
[2020-11-06 10:55:05] Bisection completed in: 0:08:32

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1670378: Add AutoEnterOOMUnsafeRegion to JS_TransplantObject. r=decoder!

Transplanting objects is inherently oom-unsafe, so add AutoEnterOOMUnsafeRegion
to JS_TransplantObject().

Now that all callers to JSObject::swap are in an AutoEnterOOMUnsafeRegion,
we can also remove the AutoEnterOOMUnsafeRegion within JSObject::swap and
instead pass it by reference. That should make it even more clear that this
operation can't recover on OOM.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca78db9e1af5
Add AutoEnterOOMUnsafeRegion to JS_TransplantObject. r=tcampbell

Comment 11

[Natalia Csoregi [:nataliaCs]]

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

Comment 12

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Bug marked as FIXED but still reproduces on mozilla-central 20201211213049-1130661c79c2.

Comment 13

[André Bargull [:anba]]

(In reply to Jason Kratzer [:jkratzer] from comment #12)

Bugmon Analysis:
Bug marked as FIXED but still reproduces on mozilla-central 20201211213049-1130661c79c2.

The crash is inevitable, but it should now be annotated with "[unhandlable oom]". Is the annotation missing?

Comment 14

[Jason Kratzer [:jkratzer]]

Andre, Bugmon doesn't implement any special handling for that annotation. I'll work with :decoder to implement it. In the meantime, I'll remove the bugmon keyword and mark this as fixed.