687102 - JS Shell-only crash with line2pc/pc2line [@ js_getOpcode]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test crashes on mozilla-central revision f3f5d8a8a473 (options -m -n -a):

function caller(obj) {}
var pc = line2pc(caller, pc2line(caller, 0XeBebb ) + 2);


As the test uses line2pc/pc2line, it should be shell-only. Backtrace:

==35745== Invalid read of size 1
==35745==    at 0x8209E3A: js_GetOpcode(JSContext*, JSScript*, unsigned char*) (jsscript.h:966)
==35745==    by 0x820E75F: js_PCToLineNumber(JSContext*, JSScript*, unsigned char*) (jsscript.cpp:1503)
==35745==    by 0x80C94FA: JS_PCToLineNumber (jsdbgapi.cpp:400)
==35745==    by 0x804F948: PCToLine(JSContext*, unsigned int, jsval_layout*) (js.cpp:1780)
==35745==    by 0x816439F: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, js::Value*), js::CallArgs const&) (jscntxtinlines.h:300)
==35745==    by 0x813C5D4: js::InvokeKernel(JSContext*, js::CallArgs const&, js::MaybeConstruct) (jsinterp.cpp:660)
==35745==    by 0x81520C2: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:4040)
==35745==    by 0x8312062: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, js::Value*, bool) (MethodJIT.cpp:914)
==35745==    by 0x83121BF: CheckStackAndEnterMethodJIT(JSContext*, js::StackFrame*, void*, bool) (MethodJIT.cpp:945)
==35745==    by 0x83122A8: js::mjit::JaegerShot(JSContext*, bool) (MethodJIT.cpp:962)
==35745==    by 0x813C354: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:611)
==35745==    by 0x813CDC2: js::ExecuteKernel(JSContext*, JSScript*, JSObject&, js::Value const&, js::ExecuteType, js::StackFrame*, js::Value*) (jsinterp.cpp:814)
==35745==  Address 0x8e299a3 is not stack'd, malloc'd or (recently) free'd

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Check PC arguments

I think the minimal test case would be

  function f() {}
  pc2line(f, 1);

The shell was not bounds-checking the 'pc' argument, resulting in an invalid memory read if it is larger than the actual script. Bad shell.

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 560644 [details] [diff] [review]
Check PC arguments

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

::: js/src/shell/js.cpp
@@ +1611,1 @@
>              int32 *ip)

Alignment. Also, can we just change ip into a uint32 now that we're validating it? There's not a lot of callers, they don't do anything useful with negative values, and they mostly cast to uint32. :-)

Comment 3

[Christian Holler (:decoder)]

Is this patch still good? If so, could it be landed to m-i?

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 560644 [details] [diff] [review]
Check PC arguments

Sorry, I attempted to push this a while back and got some test failures. When I looked into it, I found that the patch is wrong. It makes a false generalization across functions that require different arguments.

It's still not a difficult thing to fix; I just haven't gone back and done it. The clean way would be figure out a generalization that works for the trap command as well as pc2line/line2pc, probably as something that is called by GetTrapArgs and Get(SomethingElse)Args. But it could be done quicker through a cut & paste.

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Check PC arguments

Ok, the problem was just that line2pc takes different arguments from trap, untrap, and pc2line, and I had merged it in with the others. Or rather, it was already using GetTrapArgs, which previously worked because GetTrapArgs was grabbing a script and an integer, which could be interpreted as either a PC or a line number, and now it can't.

The previous code for line2pc was a little weird too. It allowed the line number to be omitted, in which case it would default to the first line of the script -- but only if the script was given. So the interface was sort of

  line2pc(fun[,line] | line)

(either a function followed optionally by a line number, or just a line number) whereas it was documented as

  line2pc([fun, ] line)

which is similar but not the same -- the docs allow fun,line and line, but the code allowed fun and fun,line and line.

This updated patch makes the code match the documentation, which admittedly is not as powerful as the previous version. I can easily implement the previous signature, if you are ok with my updated usage description above or can come up with a better one.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 572931 [details] [diff] [review]
Check PC arguments

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

Whatever works. :-)

::: js/src/shell/js.cpp
@@ +1615,5 @@
>      jsval v;
>      uintN intarg;
>      JSScript *script;
>  
> +    script = JS_GetFrameScript(cx, JS_GetScriptedCaller(cx, NULL));

Nit: while we're in here, can we push the decls down to the definition points?

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/0e7a9ed58b96

Comment 8

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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