1330489 - Crash [@ mozilla::UniquePtr<JS::ubi::EdgeRange, JS::DeletePolicy<JS::ubi::EdgeRange> >::operator->] with wasm and Debugger

Reporter

Description

•

7 years ago

The following testcase crashes on mozilla-central revision 2963cf6be7f8 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

var lfGlobalTable = new WebAssembly.Table({
    element: 'anyfunc',
});
var lfModule = new WebAssembly.Module(wasmTextToBinary(`
    (module
        (import "global" "func" (result i32))
        (func (export "func_0") (result i32)
         call 0 ;; calls the import, which is func #0
        )
    )
`));
processModule(lfModule, `
g = newGlobal('#2: x = null; x ^= true; x === 1. Actual: ' + (SyntaxError));
g.parent = this;
g.eval("Debugger(parent).onExceptionUnwind = function () {};");
`);
lfModule = new WebAssembly.Module(wasmTextToBinary(`
    (module
        (import $imp "a" "b" (result i32))
        (memory 1 1)
        (table 2 2 anyfunc)
        (elem (i32.const 0) $imp $def)
        (func $def (result i32) (i32.load (i32.const 0)))
        (type $v2i (func (result i32)))
        (func $call (param i32) (result i32) (call_indirect $v2i (get_local 0)))
        (export "call" $call)
    )
`));
processModule(lfModule, `
jsTestDriverEnd();
`);
function processModule(module, jscode) {
    imports = {}
    for (let descriptor of WebAssembly.Module.imports(module)) {
        imports[descriptor.module] = {}
        imports[descriptor.module][descriptor.name] = new Function("x", "y", "z", jscode);
            instance = new WebAssembly.Instance(module, imports);
    }
    for (let descriptor of WebAssembly.Module.exports(module)) {
        switch (descriptor.kind) {
            case "function":
                print(instance.exports[descriptor.name]())
        }
    }
}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
mozilla::UniquePtr<JS::ubi::EdgeRange, JS::DeletePolicy<JS::ubi::EdgeRange> >::operator-> (this=0x1fff63d482fe8d1) at dist/include/mozilla/UniquePtr.h:320
#0  mozilla::UniquePtr<JS::ubi::EdgeRange, JS::DeletePolicy<JS::ubi::EdgeRange> >::operator-> (this=0x1fff63d482fe8d1) at debug/dist/include/mozilla/UniquePtr.h:320
#1  0x0000000000a52489 in js::wasm::Instance::debugEnabled (this=<optimized out>) at js/src/wasm/WasmInstance.h:129
#2  js::Debugger::ensureExecutionObservabilityOfFrame (cx=0x7ffff695f000, frame=...) at js/src/vm/Debugger.cpp:2692
#3  0x0000000000a93024 in js::Debugger::getScriptFrameWithIter (this=this@entry=0x7ffff693e800, cx=cx@entry=0x7ffff695f000, referent=..., maybeIter=maybeIter@entry=0x7fffffffbec0, result=..., result@entry=...) at js/src/vm/Debugger.cpp:774
#4  0x0000000000a932d4 in js::Debugger::getScriptFrameWithIter (this=this@entry=0x7ffff693e800, cx=cx@entry=0x7ffff695f000, referent=..., maybeIter=maybeIter@entry=0x7fffffffbec0, vp=..., vp@entry=...) at js/src/vm/Debugger.cpp:745
#5  0x0000000000a95720 in js::Debugger::getScriptFrame (vp=..., iter=..., cx=0x7ffff695f000, this=0x7ffff693e800) at js/src/vm/Debugger.h:1007
#6  js::Debugger::fireExceptionUnwind (this=this@entry=0x7ffff693e800, cx=0x7ffff695f000, vp=..., vp@entry=...) at js/src/vm/Debugger.cpp:1775
#7  0x0000000000a96106 in js::Debugger::<lambda(js::Debugger*)>::operator() (dbg=0x7ffff693e800, __closure=<synthetic pointer>) at js/src/vm/Debugger.cpp:1026
#8  js::Debugger::dispatchHook<js::Debugger::slowPathOnExceptionUnwind(JSContext*, js::AbstractFramePtr)::<lambda(js::Debugger*)>, js::Debugger::slowPathOnExceptionUnwind(JSContext*, js::AbstractFramePtr)::<lambda(js::Debugger*)> > (fireHook=..., cx=0x7ffff695f000, hookIsEnabled=...) at js/src/vm/Debugger.cpp:1893
#9  js::Debugger::slowPathOnExceptionUnwind (cx=cx@entry=0x7ffff695f000, frame=...) at js/src/vm/Debugger.cpp:1027
#10 0x0000000000de8670 in js::Debugger::onExceptionUnwind (frame=..., cx=0x7ffff695f000) at js/src/vm/Debugger-inl.h:66
#11 WasmHandleDebugThrow () at js/src/wasm/WasmTypes.cpp:152
#12 0x00007ffff7ff2c30 in ?? ()
[...]
#15 0x0000000000000000 in ?? ()
rax	0x1fff63d482fe8c1	144104456163682497
rbx	0x7fffffffbac0	140737488337600
rcx	0x0	0
rdx	0x4	4
rsi	0x7fffffffc564	140737488340324
rdi	0x1fff63d482fe8d1	144104456163682513
rbp	0x7fffffffbb20	140737488337696
rsp	0x7fffffffbab8	140737488337592
r8	0x7fffffffbae0	140737488337632
r9	0x0	0
r10	0x7ffff06c2160	140737227006304
r11	0x7ffff06c2101	140737227006209
r12	0x7ffff695f000	140737330409472
r13	0x7fffffffbba0	140737488337824
r14	0x7ffff693e9e0	140737330276832
r15	0x7fffffffbc80	140737488338048
rip	0x84e8f0 <mozilla::UniquePtr<JS::ubi::EdgeRange, JS::DeletePolicy<JS::ubi::EdgeRange> >::operator->() const>
=> 0x84e8f0 <mozilla::UniquePtr<JS::ubi::EdgeRange, JS::DeletePolicy<JS::ubi::EdgeRange> >::operator->() const>:	mov    (%rdi),%rax
   0x84e8f3 <mozilla::UniquePtr<JS::ubi::EdgeRange, JS::DeletePolicy<JS::ubi::EdgeRange> >::operator->() const+3>:	test   %rax,%rax

Benjamin Bouvier [:bbouvier] (inactive)

Updated

•

7 years ago

Blocks: 1286948

Yury Delendik (:yury)

Assignee

Comment 1

•

7 years ago

wasm::FrameIterator, during WasmHandleDebugThrow, assumes that the imported function called using indirect call (via table) has debug frame:

var g = newGlobal();
g.parent = this;
g.eval("Debugger(parent).onExceptionUnwind = function () {};");

let module = new WebAssembly.Module(wasmTextToBinary(`
    (module
        (import $imp "a" "b" (result i32))
        (memory 1 1)
        (table 2 2 anyfunc)
        (elem (i32.const 0) $imp $def)
        (func $def (result i32) (i32.load (i32.const 0)))
        (type $v2i (func (result i32)))
        (func $call (param i32) (result i32) (call_indirect $v2i (get_local 0)))
        (export "call" $call)
    )
`));

let instance = new WebAssembly.Instance(module, { a: {
  b: function () { throw "test"; }
}});
instance.exports.call(0);

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/104250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104250/

Yury Delendik (:yury)

Assignee

Comment 3

•

7 years ago

The same test exposes a problem that shows that we need to discard frame as we iterate and call onExceptionUnwind in the WasmHandleDebugThrow.

Assignee: nobody → ydelendik

Luke Wagner [:luke]

Comment 5

•

7 years ago

mozreview-review

Comment on attachment 8826297 [details]
Bug 1330489 - Ignore imports for wasm debug frames and discard frames in activation on throw.

https://reviewboard.mozilla.org/r/104250/#review105070

::: js/src/wasm/WasmFrameIterator.cpp:948
(Diff revision 1)
> +void
> +wasm::PatchActivationToFrame(WasmActivation* activation, const FrameIterator& iter)
> +{
> +    MOZ_ASSERT(!iter.done() && activation == iter.activation_);
> +    void *addressOfActivationFP = static_cast<uint8_t*>((void*)activation) + WasmActivation::offsetOfFP();
> +    *static_cast<void**>(addressOfActivationFP) = iter.fp_ + iter.callsite_->stackDepth();

Ooh, tricky\!  The basic fix here seems to be the right one.  I was thinking that really the point where we want to do this operation is in FrameIter::operator++() (where we're advancing fp\_ to the caller's fp\_).  Two ways I can imagine are (1) having an FrameIterator::Unwind::(True|False) enum that defaults to False but, when true, operator++ updates the WasmActivation.
(2) making an UnwindFrameIterator that derives FrameIterator and overrides FrameIterator::operator++.

Also, I think there's also a subtle corner case invariant involving the entry-stub's frame: normally, WasmActivation::fp is null while in the entry frame and the logic here would set fp\_ to the (non-wasm::Frame) fp of the entry stub.  Integrating with FrameIter::operator++ would fix this too.

Also, could you add a WasmActivation::unwindFP() method to WasmActivation instead of the offsetOfFP() trick?

ovidiu boca[:Ovidiu]

Updated

•

7 years ago

status-firefox50: --- → unaffected

status-firefox51: --- → unaffected

status-firefox52: --- → unaffected

Comment hidden (mozreview-request)

Comment on attachment 8826297 [details]
Bug 1330489 - Ignore imports for wasm debug frames and discard frames in activation on throw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104250/diff/1-2/

Yury Delendik (:yury)

Assignee

Comment 7

•

7 years ago

mozreview-review-reply

Comment on attachment 8826297 [details]
Bug 1330489 - Ignore imports for wasm debug frames and discard frames in activation on throw.

https://reviewboard.mozilla.org/r/104250/#review105070

> Ooh, tricky\!  The basic fix here seems to be the right one.  I was thinking that really the point where we want to do this operation is in FrameIter::operator++() (where we're advancing fp\_ to the caller's fp\_).  Two ways I can imagine are (1) having an FrameIterator::Unwind::(True|False) enum that defaults to False but, when true, operator++ updates the WasmActivation.
> (2) making an UnwindFrameIterator that derives FrameIterator and overrides FrameIterator::operator++.
> 
> Also, I think there's also a subtle corner case invariant involving the entry-stub's frame: normally, WasmActivation::fp is null while in the entry frame and the logic here would set fp\_ to the (non-wasm::Frame) fp of the entry stub.  Integrating with FrameIter::operator++ would fix this too.
> 
> Also, could you add a WasmActivation::unwindFP() method to WasmActivation instead of the offsetOfFP() trick?

Fixed via (1) method: added unwind_ property to the FrameIterator. Having non-const WasmActivation::unwindFP() drove activation const reference to pointer changes.

Luke Wagner [:luke]

Comment 8

•

7 years ago

mozreview-review

Comment on attachment 8826297 [details]
Bug 1330489 - Ignore imports for wasm debug frames and discard frames in activation on throw.

https://reviewboard.mozilla.org/r/104250/#review105192

Thanks!  Looks good with two changes:

::: js/src/wasm/WasmFrameIterator.h:67
(Diff revision 2)
>  
>      void settle();
>  
>    public:
>      explicit FrameIterator();
> -    explicit FrameIterator(const WasmActivation& activation);
> +    explicit FrameIterator(WasmActivation* activation, bool unwind = false);

Can you add an `enum class Unwind { True, False }`?  This is most definitely a case where you want to see `Unwind::True` at the use site.

::: js/src/wasm/WasmFrameIterator.cpp:120
(Diff revision 2)
>          DebugOnly<uint8_t*> oldfp = fp_;
>          fp_ += callsite_->stackDepth();
>          MOZ_ASSERT_IF(code_->profilingEnabled(), fp_ == CallerFPFromFP(oldfp));
> +        if (unwind_)
> +            activation_->unwindFP(fp_);
>          settle();

I think the unwindFP() should go after the settle() (so that when we reach the entry frame, at which point the frame iter becomes done(), WasmActivation::fp_ will become null (which it must be because the entry frame doesn't have a wasm::Frame and in general this is assumed by ProfilingFrameIter).  One way to test this would be to enabling profiling (`enableSPSProfiling(); enableSingleStepProfiling()`) and then execute some debug-mode code that throws.

Attachment #8826297 - Flags: review?(luke) → review+

Comment hidden (mozreview-request)

Comment on attachment 8826297 [details]
Bug 1330489 - Ignore imports for wasm debug frames and discard frames in activation on throw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104250/diff/2-3/

Yury Delendik (:yury)

Assignee

Comment 10

•

7 years ago

mozreview-review-reply

Comment on attachment 8826297 [details]
Bug 1330489 - Ignore imports for wasm debug frames and discard frames in activation on throw.

https://reviewboard.mozilla.org/r/104250/#review105192

> I think the unwindFP() should go after the settle() (so that when we reach the entry frame, at which point the frame iter becomes done(), WasmActivation::fp_ will become null (which it must be because the entry frame doesn't have a wasm::Frame and in general this is assumed by ProfilingFrameIter).  One way to test this would be to enabling profiling (`enableSPSProfiling(); enableSingleStepProfiling()`) and then execute some debug-mode code that throws.

Fixed by moving unwindFP() after settle() with additional check for done().

wasm/profiling.js test was modified to take changing stack state in account.

Comment hidden (mozreview-request)

Comment on attachment 8826297 [details]
Bug 1330489 - Ignore imports for wasm debug frames and discard frames in activation on throw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104250/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8826297 [details]
Bug 1330489 - Ignore imports for wasm debug frames and discard frames in activation on throw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104250/diff/4-5/

Luke Wagner [:luke]

Comment 13

•

7 years ago

mozreview-review

Comment on attachment 8826297 [details]
Bug 1330489 - Ignore imports for wasm debug frames and discard frames in activation on throw.

https://reviewboard.mozilla.org/r/104250/#review105304

::: js/src/wasm/WasmStubs.cpp:1148
(Diff revision 5)
> +    // We are about to pop all frames in this WasmActivation. Checking if fp is
> +    // set to null to maintain the invariant that fp is either null or pointing
> +    // to a valid frame.
> +    {
> +        Label ok;
> +        masm.branchPtr(Assembler::NonZero, Address(scratch, WasmActivation::offsetOfFP()), scratch, &ok);

I think you want `masm.branchPtr(Assembler::Equal, Address(scratch, offsetOfFP), Imm32(0), &ok)`.  I think what you're emitting here is a cmp which subtracts fp from the WasmActivation\* in scratch and then a branch to 'ok' if the result of the subtraction is non-zero :)

Also, nit: could you put a \n before #ifdef DEBUG and also remove the surrounding { } block (since the Label 'ok' shouldn't clash with anything else).

Comment hidden (mozreview-request)

Comment on attachment 8826297 [details]
Bug 1330489 - Ignore imports for wasm debug frames and discard frames in activation on throw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104250/diff/5-6/

Comment hidden (mozreview-request)

Comment on attachment 8826297 [details]
Bug 1330489 - Ignore imports for wasm debug frames and discard frames in activation on throw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104250/diff/6-7/

Luke Wagner [:luke]

Comment 16

•

7 years ago

mozreview-review

Comment on attachment 8826297 [details]
Bug 1330489 - Ignore imports for wasm debug frames and discard frames in activation on throw.

https://reviewboard.mozilla.org/r/104250/#review105360

::: js/src/wasm/WasmFrameIterator.cpp:81
(Diff revisions 6 - 7)
>      missingFrameMessage_(false)
>  {
>      if (fp_) {
>          settle();
> +        if (unwind == Unwind::True)
> +            activation_->unwindFP(fp_);

How about moving the unwind code into settle()?

Comment hidden (mozreview-request)

Comment on attachment 8826297 [details]
Bug 1330489 - Ignore imports for wasm debug frames and discard frames in activation on throw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104250/diff/7-8/

Yury Delendik (:yury)

Assignee

Updated

•

7 years ago

Keywords: checkin-needed

Pulsebot

Comment 18

•

7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5ac5cb6fc6ef
Ignore imports for wasm debug frames and discard frames in activation on throw. r=luke

Keywords: checkin-needed

Carsten Book [:Tomcat]

Comment 19

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/5ac5cb6fc6ef

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox53: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla53