1423937 - Crash [@ js::FrameIter::callObj] with getBacktrace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 4b94da21a9e6 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2):

function print(x) {
  for (var i = 0; i < 40; ++i) {
      var stack = getBacktrace({args: true});
      (function( ... target     )  { g = x;});
}
  setInterruptCallback(function() {
    print("Worker Interrupt!");
  });
    interruptIf(true);
  print("MainThread: " + i);
}
print("MainThread Done!");


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000000000c3e726 in js::FrameIter::callObj (this=this@entry=0x7ffffffdccc0, cx=<optimized out>) at js/src/vm/Stack.cpp:1290
#0  0x0000000000c3e726 in js::FrameIter::callObj (this=this@entry=0x7ffffffdccc0, cx=<optimized out>) at js/src/vm/Stack.cpp:1290
#1  0x00000000009f87df in FormatFrame (showThisProps=false, showLocals=false, showArgs=true, num=2, inBuf=<unknown type in js, CU 0x39ce882, DIE 0x3bb1ad5>, iter=..., cx=0x7ffff5f16000) at js/src/jsfriendapi.cpp:890
#2  JS::FormatStackDump(JSContext*, mozilla::UniquePtr<char [], JS::FreePolicy>&&, bool, bool, bool) (cx=0x7ffff5f16000, inBuf=inBuf@entry=<unknown type in /mnt/LangFuzz/work/builds/debug64/dist/bin/js, CU 0x39ce882, DIE 0x3cb3bd8>, showArgs=true, showLocals=false, showThisProps=false) at js/src/jsfriendapi.cpp:1079
#3  0x0000000000897662 in GetBacktrace (cx=0x7ffff5f16000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/TestingFunctions.cpp:3296
#4  0x0000199a83660b1d in ?? ()
[...]
#8  0x0000000000000000 in ?? ()
rax	0x1f8ede0	33091040
rbx	0x1fe00a0	33423520
rcx	0x7ffff5088280	140737304363648
rdx	0x1f8ede0	33091040
rsi	0x1fdfe00	33422848
rdi	0x7ffff508b060	140737304375392
rbp	0x7ffffffdc900	140737488210176
rsp	0x7ffffffdc8d0	140737488210128
r8	0x7ffff4f104c5	140737302824133
r9	0x7ffff4f10698	140737302824600
r10	0x7ffff4f106e8	140737302824680
r11	0x206	518
r12	0x0	0
r13	0x1fdff20	33423136
r14	0x1fe0060	33423456
r15	0x1fdff60	33423200
rip	0xc3e726 <js::FrameIter::callObj(JSContext*) const+86>
=> 0xc3e726 <js::FrameIter::callObj(JSContext*) const+86>:	mov    (%r12),%rax
   0xc3e72a <js::FrameIter::callObj(JSContext*) const+90>:	mov    (%rax),%rax

Comment 1

[Jason Orendorff [:jorendorff]]

Kannan, could you take this one?

Comment 2

[Kannan Vijayan [:djvj]]

The core issue here is a frame type mismatch.  The frame walker is trying to unpack an ion frame, but is looking at a baseline frame.  Not sure why that's happening yet, but that should fix the core issue.

Comment 3

[Jim Mathies [:jimm]]

Hey Steven, this is currently a P1 and isn't active. Can we prioritize?

Comment 4

[Steven DeTar [:sdetar]]

Kannan, any updates on this bug?

Comment 5

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "bad" changeset has the timestamp "20160512023618" and the hash "4053751140c48939e4f294373aad5c74703aad12".
The "good" changeset has the timestamp "20160512023806" and the hash "6ed0dea5f446d1d305285d89a7a5b26adfb1f202".

Likely fix window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4053751140c48939e4f294373aad5c74703aad12&tochange=6ed0dea5f446d1d305285d89a7a5b26adfb1f202

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The bisection result in comment 5 is likely to not be accurate.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I could reproduce this with m-c rev 9294f67b3f3b with --fuzzing-safe --cpu-count=2:

Thread 1 "js-dbg-64-dm-li" received signal SIGSEGV, Segmentation fault.
0x0000000000c8e7b6 in js::FrameIter::callObj (this=this@entry=0x7ffffffccec0, cx=<optimized out>)
    at /home/winworklin/trees/mozilla-central/js/src/vm/Stack.cpp:1304
1304	    JSObject* pobj = environmentChain(cx);
(gdb) bt
#0  0x0000000000c8e7b6 in js::FrameIter::callObj (this=this@entry=0x7ffffffccec0, cx=<optimized out>)
    at /home/winworklin/trees/mozilla-central/js/src/vm/Stack.cpp:1304
#1  0x0000000000a420f2 in FormatFrame (showThisProps=false, showLocals=false, showArgs=true, num=2, 
    inBuf=<unknown type in /home/winworklin/shell-cache/js-dbg-64-dm-linux-9294f67b3f3b/js-dbg-64-dm-linux-9294f67b3f3b, CU 0x4246dd1, DIE 0x43e3b09>, 
    iter=..., cx=<optimized out>) at /home/winworklin/trees/mozilla-central/js/src/jsfriendapi.cpp:895
#2  JS::FormatStackDump(JSContext*, mozilla::UniquePtr<char [], JS::FreePolicy>&&, bool, bool, bool) (cx=<optimized out>, 
    inBuf=inBuf@entry=<unknown type in /home/winworklin/shell-cache/js-dbg-64-dm-linux-9294f67b3f3b/js-dbg-64-dm-linux-9294f67b3f3b, CU 0x4246dd1, DIE 0x43e3b09>, showArgs=true, showLocals=false, showThisProps=false) at /home/winworklin/trees/mozilla-central/js/src/jsfriendapi.cpp:1087
#3  0x00000000008b0cf2 in GetBacktrace (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>)
    at /home/winworklin/trees/mozilla-central/js/src/builtin/TestingFunctions.cpp:3382
#4  0x00002a0af5ef4c4a in ?? ()
#5  0x0000000000000000 in ?? ()
(gdb)

This is currently tracked by jsbugmon and is still active. Having an inaccurate bisection result doesn't mean it doesn't currently reproduce.

Please don't resolve this WFM unless it has been verified to not reproduce.

Comment 8

[Kannan Vijayan [:djvj]]

I'll take a look at this soon.  I'm close to kind of a high-pressure milestone for BinAST and need to wrap up that work.  Leaving needinfo on.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 11

[Christian Holler (:decoder)]

Still reproduces, reopening...

Comment 12

[Kannan Vijayan [:djvj]]

CC-ing Iain because he is looking for fuzzbugs to work on.

Comment 13

[Iain Ireland [:iain]]

Yep, I'm looking at this one. I've successfully reproduced the failure in my own build, and I'm poking at it in rr.

Comment 14

[Iain Ireland [:iain]]

I've worked through what is happening, but I haven't figured out which part is wrong yet. My reduced testcase:

setInterruptCallback(function() {
    foo("A");
});
function foo(x) {
    for (var i = 0; i < 1000; i++) {
	var stack = getBacktrace({args: true});
    }
    interruptIf(true);
    foo("B");
    (function()  { g = x;});
}
foo("C");

The situation is:
  We register an interrupt callback that calls |foo|.
  We ion-compile foo.
  Because foo calls itself recursively, we inline a copy of foo inside itself.
  With an ion frame of foo-with-inlined-foo on the stack, we trigger an interrupt.
  There is an interruptcheck inside the inlined recursive call. It fires.
  The interrupt is handled. The handler calls foo.
  foo calls GetBacktrace.
  At this point, the stack looks roughly like this (most recent frame on top):
  #1  foo("A") -- ion
  #2  interrupt handler -- baseline
  #3  <c++ interrupt handling code>
  #4  foo("B") -- ion (inlined into one frame)
  #5  foo("A")  /
  #6  interrupt handler -- baseline
  #7  <c++ interrupt handling code)
  #8  foo("B") -- baseline?
  #9  foo("C") -- baseline?
  #10 main
  GetBacktrace successfully dumps the stack frame for the most recent call to foo and the interrupt handler.
  When it tries to print out the interrupted call to foo, it tries to access the call obj.
  (We need a callobj because of the unused inner function that closes over x.)
  However, we don't find a callobj: all we find is the lexical frame of foo, contained in the global environment.
  We walk off the end of the environment chain, and dereference a nullptr.

My best guess is that something is wrong with the snapshots or recovery instructions. The first step of finding the call object is building the environment chain. In frame #1, we hit RNewCallObject::recover and allocate a new call object. That's the same object that we end up using in FormatFrame. In frame #3, we hit a snapshot with mode CST_UNDEFINED, and resort to a fallback.

Comment 15

[Iain Ireland [:iain]]

I'm pretty sure I've worked out what is going on.

I was wrong earlier: the interruptcheck we care about is not in the inlined recursive call. It's from the checkoverrecursed at the very beginning of the function. This check happens *before* the call object is created. When we "bail out" to that point to collect backtrace information, the snapshot correctly says that the call object is undefined. I don't think there's anything wrong with the snapshots; the problem seems to be with the scope chain recovery code, which is assuming that every stack frame it walks through will always have a call object.

(I assume that we have good reasons for wanting the checkoverrecursed to happen before we start building the call object.)

Nicolas, it looks like you wrote this code (bug 1073033). Does my analysis make sense? Thoughts on the best solution?

Comment 16

[Nicolas B. Pierron [:nbp]]

After some discussion over IRC, the problem is that we are trying to get a callObj() from a frame which executed the interruption callback before creating the CallObject or even registering it in the corresponding snapshot slot.  This is due to CheckOverRecurse which is at the entry of the function, which also calls the interruption callback.

The call to callObj() happens in FormatFrame, to request arguments which are aliased in the CallObject. This would make sense, if we had create the CallObject and that one of the inner function mutated it.

Iain suggested adding a maybeCallObj() function, to discard this first way to lookup for arguments. I think this is likely to work as expected, as the code from FormatFrame should be made by reading the content of the actual frame arguments.

Comment 17

[Iain Ireland [:iain]]

Upon inspection, it turns out that this was the only caller of FrameIter::callObj, so instead of implementing maybeCallObj I just changed the existing implementation to return a null pointer on failure.

I tried using unaliasedActual in FormatFrame to read the content of the actual frame arguments, but it asserts because we don't have a usable abstract frameptr at that point. Instead, I'm just setting arg to MagicValue(JS_OPTIMIZED_OUT), which prints "[unavailable]":

0 foo(x = "A") ["/home/iain/src/foo.js":9]
1 anonymous() ["/home/iain/src/foo.js":3]
2 foo(x = [unavailable]) ["/home/iain/src/foo.js":6]
3 foo(x = "A") ["/home/iain/src/foo.js":13]
4 anonymous() ["/home/iain/src/foo.js":3]
5 foo(x = "B") ["/home/iain/src/foo.js":6]
6 foo(x = "C") ["/home/iain/src/foo.js":13]
7 <TOP LEVEL> ["/home/iain/src/foo.js":16]

I think that's good enough, especially since this code is not user-facing.

Comment 18

[Iain Ireland [:iain]]

Attachment: Bug 1423937: Rewrite FrameIter::callObj to be fallible r?tcampbell

Comment 19

[Iain Ireland [:iain]]

Attachment: Bug 1423937: Add FrameIter::hasInitialEnvironment to guard FrameIter::callObj r?tcampbell

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5caf51de3bc8
Add FrameIter::hasInitialEnvironment to guard FrameIter::callObj r=tcampbell

Comment 21

[Narcis Beleuzu [:NarcisB]]

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