Closed Bug 687102 Opened 13 years ago Closed 13 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: decoder, Assigned: sfink)


(Keywords: crash, testcase)


(1 file, 1 obsolete file)

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
Attached patch Check PC arguments (obsolete) — Splinter Review
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.
Attachment #560644 - Flags: review?(cdleary)
Whiteboard: js-triage-needed
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. :-)
Attachment #560644 - Flags: review?(cdleary) → review+
Is this patch still good? If so, could it be landed to m-i?
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.
Attachment #560644 - Flags: review+ → review-
Keywords: checkin-needed
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.
Assignee: general → sphink
Attachment #560644 - Attachment is obsolete: true
Attachment #572931 - Flags: review?(cdleary)
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?
Attachment #572931 - Flags: review?(cdleary) → review+
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Automatically extracted testcase for this bug was committed:
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.