jsclosure crash on invalid context index

[davepacheco]

mdb_v8 crashed here:

> $C
080464b8 libc.so.1`_lwp_kill+0x15(1, 6, 8046510, feee2000, feee2000, 8046550)
080464d8 libc.so.1`raise+0x2b(6, 6, 8046510, fee29a40, d, 8093654)
08046528 libc.so.1`abort+0xcb(8046550, 8046550, 4e, fc519d20, fc519cec, 2a5)
08046758 0xfede1c54(fc519d20, fc519cec, 2a5, fc510607)
08046788 v8.so`v8context_elt+0x3b(110f59b0, 5, 8046c28, fc5108da, 8cf21bed, fc529f20)
080467a8 v8.so`v8context_var_value+0x4e(110f59b0, 1, 8046c0c, feac2000)
08046c28 v8.so`jsclosure_iter_var+0x86(110f5938, 8046c58, 110f59b0, 2)
08046c78 v8.so`v8scopeinfo_iter_vars+0x112(110f5938, 2, fc50f95e, 110f59b0)
08046ca8 v8.so`dcmd_jsclosure+0xae(bb1f78dd, b, 0, 0, 0, 0)
08046cd8 dcmd_invoke+0x40(8d46330, bb1f78dd, b, 0, 0, 0)
08046d38 mdb_call_idcmd+0x22b(8d46330, 80208e11, 0, 1, 0, b)
08046df8 mdb_call+0x325(80208ded, 0, 1, 0, 0, 26)
08046eb8 yyparse+0x3f7(feee2000, 8046f10, 8046ef8, 95aab48, fed72a40, 819dec8)
08046f78 mdb_run+0x26d(0, 8047db4, 0, 0, 0, 0)
08047c78 main+0x154c(8047c6c, feeef348, 8047ca4, 8063f97, 2, 8047cb0)
08047ca4 _start+0x83(2, 8047db0, 8047db4, 0, 8047dbf, 8047dcb)

On the original core file, using mdb_v8 v1.2.0, I can replicate this with just:

# mdb $MANTA_INPUT_FILE
Loading modules: [ libumem.so.1 libc.so.1 ld.so.1 ]
> ::load ./mdb_v8_ia32.so
mdb_v8 version: 1.2.0 (release, from f6c6dd3)
V8 version: 3.14.5.11
Autoconfigured V8 support from target
C++ symbol demangling enabled
> bb1f78dd::jsclosure
    "probe": 8a325c49: function runPostChain
Assertion failed: i < ctxp->v8ctx_nelts, file src/mdb_v8_function.c, line 718

*** mdb: received signal ABRT at:
    [1] libc.so.1`_lwp_kill+0x15()
    [2] libc.so.1`raise+0x2b()
    [3] libc.so.1`abort+0xcb()
    [4] libc.so.1`__assert+0x86()
    [5] mdb_v8_ia32.so`v8context_elt+0x43()
    [6] mdb_v8_ia32.so`v8context_var_value+0x4b()
    [7] mdb_v8_ia32.so`jsclosure_iter_var+0xa1()
    [8] mdb_v8_ia32.so`v8scopeinfo_iter_vars+0x111()
    [9] mdb_v8_ia32.so`dcmd_jsclosure+0xc7()
    [10] mdb`dcmd_invoke+0x40()
    [11] mdb`mdb_call_idcmd+0x128()
    [12] mdb`mdb_call+0x325()
    [13] mdb`yyparse+0x41b()
    [14] mdb`mdb_run+0x26d()
    [15] mdb`main+0x154c()
    [16] mdb`_start+0x83()

mdb: (c)ore dump, (q)uit, (r)ecover, or (s)top for debugger [cqrs]? 

Here's the underlying function:

> bb1f78dd::v8function
bb1f78dd: JSFunction: <anonymous> (as <anon>)
defined at /opt/smartdc/moray/node_modules/vasync/lib/vasync.js position 6298
context: bb1e1f51
shared scope_info: 8a3902ed
code: 90aa45e1
instructions: [90aa4620, 90aa468c)

jsclosure loads the v8function (bb1f78dd), gets its context (bb1e1f51), and gets the context's scopeinfo. To do that last step, we get the context's closure (bb1e0385), then load that as a function, and gets its scopeinfo:

> bb1e0385::v8function
bb1e0385: JSFunction: pipelineCallback
defined at /opt/smartdc/moray/lib/control.js position 2505
context: e363d261
shared scope_info: 8a393855
code: bbd1dec1
instructions: [bbd1df00, bbd1e0e4)

So for the original closure bb1f78dd, the context is bb1e1f51, and the scopeinfo is 8a393855. Sure enough, that scopeinfo has two context-local variables:

> 8a393855::v8scopeinfo
1 parameter
    parameter 0: 8cf1c7fd ("err")
0 stack local variables
2 context local variables
    context local variable 0: 992eede1 ("probe")
    context local variable 1: 8cf21bed ("done")
> 8a393855::v8array
2
2
0
4
8cf1c7fd
992eede1
8cf21bed
10
10
> 

but the closure we have for it only has one slot:

> bb1e1f51::v8context
closure function: bb1e0385 (JSFunction)
previous context: bb1e03a9 (JSArray)
extension: 0 (SMI: value = 0)
global object: 90209619 (JSGlobalObject)
    slot 0: 8a325c49 (JSFunction)
> 

I suspect that's because this is garbage, though it's hard to say for sure in this core file.

While iterating the variables, we call v8context_var_value to get the value of the ith variable, and we blow an assertion because there aren't that many slots in the context. What's weird is that we actually have a check for this, but I think it's checking the wrong thing. It's checking i, the user's argument, against the number of entries in the context array. However, the real index is idx, which is offset from i because of static fields at the front of the context array. I think the check just needs to be updated to check idx rather than i.

[davepacheco]

I've got a change to fix this (and release v1.2.1 at the same time) at https://cr.joyent.us/2628.

PS1 is make prepush clean on Node v0.10.46. I believe it is also zero risk, in that the only case that could be affected by the code change is one that would have previously crashed anyway.