Crash [@ js::gc::IsInsideNursery]

VERIFIED FIXED in Firefox 55

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
11 months ago
21 days ago

People

(Reporter: gkw, Assigned: luke)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla55
x86_64
Linux
crash, jsbugmon, sec-moderate, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox52 unaffected, firefox-esr52 unaffected, firefox53 wontfix, firefox54 wontfix, firefox55 verified)

Details

(Whiteboard: [jsbugmon:update][adv-main55+], crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

11 months ago
The following testcase crashes on mozilla-central revision b043233ec04f (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

// Adapted from randomly chosen test: js/src/jit-test/tests/debug/execution-observability-03.js
p = new Proxy(this, {
    "deleteProperty": function () {
        g = newGlobal();
        g.parent = this;
        g.eval("Debugger(parent).onEnterFrame = function() {};");
        return 1;
    }
});
delete p.f;
// Adapted from randomly chosen test: js/src/jit-test/tests/wasm/timeout/1.js
timeout(0.01);
var code = wasmTextToBinary('(module(func(export "f1")(loop$top br$top)))');
new WebAssembly.Instance(new WebAssembly.Module(code)).exports.f1();


Backtrace:

#0  js::gc::IsInsideNursery (cell=0xe4e4e4e400000000) at /home/ubuntu/shell-cache/js-dbg-64-dm-linux-b043233ec04f/objdir-js/dist/include/js/HeapAPI.h:328
#1  js::gc::Cell::isTenured (this=0xe4e4e4e400000000) at js/src/gc/Heap.h:251
#2  JSObject::readBarrier (obj=0xe4e4e4e400000000) at js/src/jsobj.h:644
#3  js::InternalBarrierMethods<js::WasmInstanceObject*>::readBarrier (v=0xe4e4e4e400000000) at js/src/gc/Barrier.h:266
#4  js::ReadBarrieredBase<js::WasmInstanceObject*>::read (this=0x7f0329759068) at js/src/gc/Barrier.h:553
/snip

For detailed crash information, see attachment.

Setting s-s because GC is on the stack, as a start.
(Reporter)

Comment 1

11 months ago
Created attachment 8854908 [details]
Detailed Crash Information
(Reporter)

Comment 2

11 months ago
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/766ead465209
user:        Yury Delendik
date:        Sat Jan 07 10:36:11 2017 -0600
summary:     Bug 1286948 - Extends AbstractFramePtr to reference wasm::DebugFrame. r=luke,shu

Yury, is bug 1286948 a likely regressor?
Blocks: 1286948
Flags: needinfo?(ydelendik)
status-firefox52: --- → unaffected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → unaffected
what does the 0xe4 memory poison value mean?

calling this "sec-moderate" because it looks like Debugger calls not available to the web are involved, but if the underlying bug can be hit from a normal context we should raise this to sec-high.
Keywords: sec-moderate
(In reply to Daniel Veditz [:dveditz] from comment #3)
> what does the 0xe4 memory poison value mean?

It's our junk fill [1]. Basically what you get when you malloc something but don't initialize it.

[1] http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/memory/mozjemalloc/jemalloc.c#5107-5112
(Assignee)

Comment 5

11 months ago
Ah hah, what's happening here is that the timeout() is asynchronously interrupting execution while wasm frames are on the stack.  Because the async interrupt doesn't set WasmActivation::exitFP, FrameIter doesn't find any frames and so the soon-to-be-popped frames are left in the debugger's frame map where they later explode.

I think the fix is for the interrupt callback to pass 'fp' to WasmHandleExecutionInterrupt() so that it can set exitFP.  This would actually strengthen our invariants about WasmActivation::exitFP and remove some async-interrupt special cases in other places.
(Assignee)

Comment 6

11 months ago
Created attachment 8857584 [details] [diff] [review]
fix-interrupt-fp

I'm pretty happy about this patch: it removes an entire case of "interrupted and lost the stack" with a pretty aggressive assert in FrameIterator.  It also means we always get full stacks in slow-script-interrupt situations.
Assignee: nobody → luke
Attachment #8857584 - Flags: review?(bbouvier)
(Assignee)

Comment 7

11 months ago
Created attachment 8857585 [details] [diff] [review]
make-test-faster

Unrelated, I saw this test timeout on non-JIT and eager runs.  Disabling in these cases allows the test to run in 7s without loss of coverage.
Attachment #8857585 - Flags: review?(bbouvier)
(Assignee)

Comment 8

11 months ago
Actually, the new tighter assertion shows that we now depend on the builtin thunks (b/c some of them can GC, and thus iterate the stack).  Great timing on that patch Benjamin!
Blocks: 1340219
(Assignee)

Comment 9

11 months ago
Created attachment 8857666 [details] [diff] [review]
fix-interrupt-fp

Testing on windows revealed that GetThreadContext() on Win64 was never returning fp; the CONTEXT_CONTROL flag had to be upgraded to CONTEXT_FULL to get fp (but only on Win64; on Win32, CONTEXT_CONtroL gives you fp...).  Yay for stronger assertions!
Attachment #8857584 - Attachment is obsolete: true
Attachment #8857584 - Flags: review?(bbouvier)
Attachment #8857666 - Flags: review?(bbouvier)
Comment on attachment 8857585 [details] [diff] [review]
make-test-faster

Review of attachment 8857585 [details] [diff] [review]:
-----------------------------------------------------------------

Ha, fun!
Attachment #8857585 - Flags: review?(bbouvier) → review+
Comment on attachment 8857666 [details] [diff] [review]
fix-interrupt-fp

Review of attachment 8857666 [details] [diff] [review]:
-----------------------------------------------------------------

So nice.

That makes me wonder about the remaining builtin calls that don't get thunked, but I think that for these we're good as well, because either they have their own exit prologue/epilogue (e.g. callimport, coerce_in_place, traps and debug trap, ), or they'll inherit the FP value of the caller (e.g. throw, OOB, unaligned accesses, and now handle interrupt with this patch). Good.

::: js/src/jit/arm/Simulator-arm.cpp
@@ +1555,5 @@
> +void
> +Simulator::handleWasmInterrupt()
> +{
> +    void* pc = reinterpret_cast<void*>(get_pc());
> +    uint8_t* fp = reinterpret_cast<uint8_t*>(get_register(r11));

s/r11/FramePointer/ ?

@@ +1580,5 @@
>      if (!act)
>          return false;
>  
>      void* pc = reinterpret_cast<void*>(get_pc());
> +    uint8_t* fp = reinterpret_cast<uint8_t*>(get_register(r11));

(same comment as above)

::: js/src/vm/Stack.cpp
@@ +1687,5 @@
> +    // interrupt-during-interrupt.
> +    MOZ_ASSERT(!interrupted());
> +    MOZ_ASSERT(compartment()->wasm.lookupCode(pc)->lookupRange(pc)->isFunction());
> +    MOZ_ASSERT(pc);
> +    MOZ_ASSERT(fp);

I'd move the last two assertions before the lookupCode assertion (since the lookupCode assertion will fail if !pc, but having the pc assertion trigger first is more explanatory).

@@ +1697,5 @@
> +void
> +WasmActivation::finishInterrupt()
> +{
> +    MOZ_ASSERT(resumePC_);
> +    MOZ_ASSERT(exitFP_);

Or just MOZ_ASSERT(interrupted())?

::: js/src/vm/Stack.h
@@ +1764,5 @@
> +    // simulator) and cleared by WasmHandleExecutionInterrupt or WasmHandleThrow
> +    // when the interrupt is handled.
> +    void startInterrupt(void* pc, uint8_t* fp);
> +    void finishInterrupt();
> +    bool interrupted() const { return !!resumePC_; }

Would it make sense that in the dtor of WasmActivation, we'd assert !interrupted()? (although the interrupt stub handler doesn't reset the state, so i'm not sure it holds as is)

::: js/src/wasm/WasmFrameIterator.cpp
@@ -235,4 @@
>      // Only non-imported functions can have debug frames.
>      return code_->metadata().debugEnabled &&
> -           fp_ &&
> -           !missingFrameMessage_ &&

I like these simplifications!

::: js/src/wasm/WasmSignalHandlers.cpp
@@ +843,1 @@
>          // to InterruptRunningCode's use of SuspendThread. When this happens,

pre-existing: InterruptRunningCode has changed name. There's a second occurrence above.
Attachment #8857666 - Flags: review?(bbouvier) → review+
Flags: needinfo?(ydelendik)
(Assignee)

Comment 12

11 months ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #11)
Thanks!

> s/r11/FramePointer/ ?

Unfortunately, the ARM simulator has its own set of enums for registers so FramePointer isn't compatible.

> Would it make sense that in the dtor of WasmActivation, we'd assert
> !interrupted()? (although the interrupt stub handler doesn't reset the
> state, so i'm not sure it holds as is)

Technically we already do (by asserting resumePC_ == null).  The interrupt state is always cleared on both the continue and stop paths (by WasmHandleExecutionInterrupt and WasmHandleThrow, resp.).  I can change to !interrupted(), though.
(Assignee)

Comment 13

11 months ago
Landed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f90707333e7f6daaaf364bc61890ba2b1849199d
  https://hg.mozilla.org/integration/mozilla-inbound/rev/20e85b5bd3429c3e7386b56fe6c6056d52219eb0

This created an intermittent win8 failure of asm.js/testBug1117255.js that I had missed in try and local runs.  Reducing the shell's watchdog timer interval from .1s to 1ms makes it reproduce easily, though.  With printf logging I can see a most bizarre sequence:
 1. the watchdog interrupts the thread at PC x and attempts to redirect to PC y
 2. an OOB fault at PC x fires (probably it had already fired internally before step 1, but hadn't bubbled up to userspace yet)
 3. the OOB handler handles successfully by redirecting PC and this *clobbers* the redirect in made in step 1
 4. now WasmActivation::interrupt() is left 'true' even though the interrupt handler will never be called (b/c of step 3)

Fortunately, just like the other weird case in the Windows handler, this is precisely detectable and has a simple fix (once you understand the madness):
  https://hg.mozilla.org/integration/mozilla-inbound/rev/b411de2683e99d545e82f55113a0f0dc2920808b
https://hg.mozilla.org/mozilla-central/rev/20e85b5bd342
https://hg.mozilla.org/mozilla-central/rev/f90707333e7f
https://hg.mozilla.org/mozilla-central/rev/b411de2683e9
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Looks like we'll want to uplift this to 54?
status-firefox53: affected → wontfix
Flags: needinfo?(luke)
(Assignee)

Comment 16

10 months ago
IIUC, the bug can only be activated by the debugger combined with the slow-script dialog.  I'm not sure what the policy is for rare devtools-only crashes, but I'd guess this didn't require uplift and letting the fix ride the trains would be fine.
Flags: needinfo?(luke)
Good enough for me.
status-firefox54: affected → wontfix

Updated

10 months ago
Group: javascript-core-security → core-security-release

Updated

10 months ago
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified

Comment 18

10 months ago
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main55+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.