Closed
Bug 508402
Opened 16 years ago
Closed 15 years ago
Segfault with JS_SetCallReturnValue2 and JIT
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jorendorff, Assigned: jorendorff)
Details
(Whiteboard: [sg:critical])
it[0] = "zero";
f = it.item;
for (var i = 0; i < 9; i++)
print(f.apply(it, [0]));
zero
zero
zero
Program received signal SIGSEGV, Segmentation fault.
#0 0x08124574 in js_DeflateString (cx=0x9ddaba8, chars=0x821bbe0,
nchars=165553128) at ../jsstr.cpp:3182
#1 0x080549ac in JS_EncodeString (cx=0x9ddaba8, str=0x9dde040)
at ../jsapi.cpp:5536
#2 0x08050f6b in Print (cx=0x9ddaba8, argc=1, vp=0xbff4c328)
at ../../shell/js.cpp:1012
#3 0xb7bddf83 in ?? ()
#4 0xbff4e988 in ?? ()
#5 0x08172c8f in js_MonitorLoopEdge (cx=0x9ddaba8,
inlineCallCount=@0xbff4f104) at ../jstracer.cpp:5957
#6 0x081c7ca2 in js_Interpret (cx=0x9ddaba8) at ../jsinterp.cpp:3923
#7 0x080b56a1 in js_Execute (cx=0x9ddaba8, chain=0x9dde000,
script=0x9de5628, down=0x0, flags=0, result=0x0) at ../jsinterp.cpp:1638
#8 0x080560ab in JS_ExecuteScript (cx=0x9ddaba8, obj=0x9dde000,
script=0x9de5628, rval=0x0) at ../jsapi.cpp:5025
#9 0x08051d10 in Process (cx=0x9ddaba8, obj=0x9dde000,
filename=0xbff4f662 "crasher.js", forceTTY=0) at ../../shell/js.cpp:408
#10 0x080528ac in ProcessArgs (cx=0x9ddaba8, obj=0x9dde000, argv=0xbff4f418,
argc=2) at ../../shell/js.cpp:816
#11 0x08052c73 in main (argc=2, argv=0xbff4f418, envp=0xbff4f424)
at ../../shell/js.cpp:4755
(gdb) f 1
#1 0x080549ac in JS_EncodeString (cx=0x9ddaba8, str=0x9dde040)
at ../jsapi.cpp:5536
5536 return js_DeflateString(cx, str->chars(), str->length());
(gdb) p/x *str
$4 = {mLength = 0x9de23e8, {mChars = 0x821bbe0, mBase = 0x821bbe0}}
The interpreter checks cx->rval2set after every call. It might be hard to avoid duplicating that on trace. Banning JS_SetCallReturnValue2 outright for JSFastNatives, and enforcing with an assertion, might work.
Security-sensitive because XPConnect calls JS_SetCallReturnValue2 (see http://hg.mozilla.org/tracemonkey/file/9fbf8217bd47/js/src/xpconnect/src/XPCDispObject.cpp#l396 -- greek to me, atm).
Comment 1•16 years ago
|
||
!@#$&*(%!
We should not support JS_SetCallReturnValue2 from JSFastNative implementations!
/be
Comment 2•16 years ago
|
||
(was cursing myself, no worries...)
Comment 3•16 years ago
|
||
Should we support it at all, in 2009? I remember the "we can do that too, no fear" thread about VBScript or JScript that led to its implementation, but it sure feels like a barnacle now...
Comment 4•16 years ago
|
||
It's a sunk cost, which if fast natives are protected, is not an issue (AFAIK) otherwise. Why waste time digging it up when embeddings use it? It did see usage, based on malign VB-isms in the DOM.
/be
Assignee | ||
Comment 5•16 years ago
|
||
> It's a sunk cost, which if fast natives are protected, is not an issue (AFAIK)
> otherwise.
We call JSNatives directly on trace. it.item is a JSNative. So banning JS_SetCallReturnValue2 for JSFastNatives would not by itself fix this crash.
(More generally, I claim there is no such thing as a sunk cost as far as SM features are concerned.)
To fix this bug while retaining the JS_SetCallReturnValue2 API as-is, I think we would have to do something like this:
1. In JS_SetCallReturnValue2, assert that the top stack frame is a JSNative. (Patch the interpreter to capture the tiny win from not worrying about this when calling a JSFastNative.)
2. Fix the JIT. Two proposals here.
2a. Instead of calling JSNatives directly on trace, have a builtin CallJSNative for this purpose. This will eliminate a lot of LIR (see below) each time we call a JSNative. Hard to say if this is a perf loss. Probably not.
2b. Emit even more LIR, as shown in the diff below (see how much LIR we already emit?):
ok = call #item(cx, argv, 1, vp, vp)
sti sp[0] = ok ;i don't know why we emit this
sti cx[496] = NULL ;null nativeVp and bailExit
sti cx[488] = NULL
- ld6 = ld state[72] ;interpState->builtinStatus
ld7 = ld add1[0] ;primary return value
sti sp[0] = ld7 ;save to stack ahead of deep-bail guard
+ ; check rval2set
+ ld6 = ld state[72] ;interpState->builtinStatus
+ rval2setbyte = ldb cx[offsetof(JSContext, rval2set)]
+ rval2set = eq rval2setbyte, 0
+ jf rval2set -> 1
+ call HandleRval2(cx, sp) ;modifies sp[0]
+ 1:
; update builtinStatus if ok==0, and guard that builtinStatus==0
and2 = and ok, 1 ;begin calculating new builtinStatus
xor1 = xor and2, 1
lsh1 = lsh xor1, 1
or1 = or ld6, lsh1
sti state[72] = or1 ;store new builtinStatus
eq3 = eq or1, NULL ;guard that builtinStatus==0
xf2: xf eq3 -> pc=0x9daa156 imacpc=(nil) sp+8 rp+0
; unbox
- js_UnboxDouble1 = fcall #js_UnboxDouble(ld7)
+ result = ldp sp[0] ; have to reload after conditional branch!
+ js_UnboxDouble1 = fcall #js_UnboxDouble(result)
Of course if we're willing to ditch/replace/evolve JS_SetCallReturnValue2 there may be other options.
Assignee | ||
Comment 6•16 years ago
|
||
Note in the above that LIR_ldb doesn't exist, and I'm not sure LIR_ldcb has the right semantics.
Comment 7•16 years ago
|
||
>
> Of course if we're willing to ditch/replace/evolve JS_SetCallReturnValue2 there
> may be other options.
Are we attached to it?
Comment 8•16 years ago
|
||
I still haven't heard a strong argument in favor of keeping JS_SetCallReturnValue2. Its clearly not a sunk cost. Its a lot of cost ahead, in implementation time, and a bit runtime cost. What exactly would break or work differently if we drop it?
Comment 9•16 years ago
|
||
http://archive.netbsd.se/?ml=mozilla-dev-tech-js-engine&a=2007-06&t=4511442 has a conversation, perhaps a bit ironic given this bug, about some uses for it, kinda. Those people might be better off doing source-to-source translation or something? (Or just sticking with JS 1.8.1, which will work just fine for them!)
http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/XPCDispObject.cpp#396 uses it for IDispatch, which I would not be afraid to dig up or replace with other syntax, I must confess.
Comment 10•16 years ago
|
||
The cost was sunk below sea-level and we didn't have sonar on ;-).
We can lose the whole feature. IIRC it was #if'ed via jsconfig.h (now jsversion.h) -- I hope that's still the case.
/be
Comment 11•16 years ago
|
||
Posted a low-impact proposal/warning about removing Reference return types at
http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_frm/thread/63c8a70d6e5795ed#
/be
Comment 12•16 years ago
|
||
Public bug about removing lvalue-return support: bug 509098. Use it for patching.
/be
Comment 13•16 years ago
|
||
(In reply to comment #5)
> (More generally, I claim there is no such thing as a sunk cost as far as SM
> features are concerned.)
There are alway sunk costs (e.g. all of the code onto which we bolted a tracing JIT without too much change). But see the sunk cost fallacy:
http://en.wikipedia.org/wiki/Sunk_costs#Loss_aversion_and_the_sunk_cost_fallacy
It's a real and ongoing problem for us, which may have been your point ;-).
/be
![]() |
||
Updated•15 years ago
|
Whiteboard: [sg:critical]
Comment 14•15 years ago
|
||
Jason, you removed JS_SetCallReturnValue2 in bug 509098. Is this bug only relevant on Firefox 3.5?
Assignee: general → jorendorff
Assignee | ||
Comment 15•15 years ago
|
||
That's correct.
Comment 16•15 years ago
|
||
I propose that this be marked FIXED. In FF 3.5, we can't call into XPConnect's Invoke while on trace, so we can't tickle the bug, and 3.6+ world has been rid of the ReturnValue2 burden.
Comment 17•15 years ago
|
||
concur. reopen if you disagree.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•