Segfault with JS_SetCallReturnValue2 and JIT

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

(Assignee)

Description

9 years ago
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).
!@#$&*(%!

We should not support JS_SetCallReturnValue2 from JSFastNative implementations!

/be
(was cursing myself, no worries...)
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...
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

9 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

9 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

9 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

9 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?
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.
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
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
Public bug about removing lvalue-return support: bug 509098. Use it for patching.

/be
(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
Whiteboard: [sg:critical]

Comment 14

9 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

9 years ago
That's correct.
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

9 years ago
concur. reopen if you disagree.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.