Last Comment Bug 687102 - JS Shell-only crash with line2pc/pc2line [@ js_getOpcode]
: JS Shell-only crash with line2pc/pc2line [@ js_getOpcode]
Status: RESOLVED FIXED
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- major (vote)
: mozilla11
Assigned To: Steve Fink [:sfink] [:s:]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2011-09-16 10:26 PDT by Christian Holler (:decoder)
Modified: 2013-01-19 14:17 PST (History)
7 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Check PC arguments (3.67 KB, patch)
2011-09-16 16:00 PDT, Steve Fink [:sfink] [:s:]
sphink: review-
Details | Diff | Splinter Review
Check PC arguments (4.62 KB, patch)
2011-11-08 12:03 PST, Steve Fink [:sfink] [:s:]
cdleary: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2011-09-16 10:26:43 PDT
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:] 2011-09-16 16:00:43 PDT
Created attachment 560644 [details] [diff] [review]
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) 2011-09-26 15:30:28 PDT
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) 2011-10-28 16:21:43 PDT
Is this patch still good? If so, could it be landed to m-i?
Comment 4 Steve Fink [:sfink] [:s:] 2011-11-01 12:19:49 PDT
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:] 2011-11-08 12:03:23 PST
Created attachment 572931 [details] [diff] [review]
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) 2011-11-08 12:39:30 PST
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] 2011-11-09 05:31:03 PST
https://hg.mozilla.org/mozilla-central/rev/0e7a9ed58b96
Comment 8 Christian Holler (:decoder) 2013-01-19 14:17:25 PST
Automatically extracted testcase for this bug was committed:

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

Note You need to log in before you can comment on or make changes to this bug.