516897 - 64bit-only crash [@ GetGCThingFlags] [@ JS_CallTracer] with setter, watch, gc

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Using a 64-bit js shell build on 32-bit Leopard, this crashes.  (opt or debug, with or without -j.)

this.__defineSetter__("x", gc);
this.watch("x",function(){return});
x = 3;

It usually dereferences 0x0fe0 or 0x000c, but I don't trust GC, so I'm filing as security-sensitive.  Oh, and sometimes it divides by 0 instead.

As usual, I'm curious why this is 64bit-only.  I used dvander's build instructions from bug 489146 comment 27.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

This happens on 32-bit too - it just doesn't crash. 

(gdb) f
#0  js_TraceStackFrame (trc=0xbfffe8c4, fp=0xbfffec3c) at ../jsgc.cpp:2856
2856	            TRACE_JSVALS(trc, nslots, fp->slots, "slot");
(gdb) p nslots
$3 = 268436698

The GC is reading totally invalid memory because js_watch_set decided to set sp = NULL.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Whoops, and meant to also show the computation that goes wrong:

2845	        if (fp->slots) {
(gdb) 
2850	            if (fp->regs) {
(gdb) 
2851	                nslots = (uintN) (fp->regs->sp - fp->slots);

Comment 3

[Jeff Walden [:Waldo]]

(Typed most of this before comment 1, posting for general edumacation in case it helps)

2850         /* fp->slots is null for watch pseudo-frames, see js_watch_set. */
2851         if (fp->slots) {
2852             /*
2853              * Don't mark what has not been pushed yet, or what has been
2854              * popped already.
2855              */
2856             if (fp->regs) {
2857                 nslots = (uintN) (fp->regs->sp - fp->slots);
2858                 JS_ASSERT(nslots >= fp->script->nfixed);
2859             } else {
2860                 nslots = fp->script->nfixed;
2861             }
2862             TRACE_JSVALS(trc, nslots, fp->slots, "slot");
2863         }

fp->regs->sp is NULL, fp->slots is valid.  The subtraction on 32-bit produces some monster number for nslots, of course greater than fp->script->nfixed.  Why doesn't 32-bit crash?

2822 #define TRACE_JSVALS(trc, len, vec, name)                      \
2823     JS_BEGIN_MACRO                                             \
2824     jsval _v, *_vp, *_end;                                     \
2825                                                                \
2826         for (_vp = vec, _end = _vp + len; _vp < _end; _vp++) { \
2827             _v = *_vp;                                         \
2828             if (JSVAL_IS_TRACEABLE(_v)) {                      \
2829                 JS_SET_TRACING_INDEX(trc, name, _vp - (vec));  \
2830                 JS_CallTracer(trc, JSVAL_TO_TRACEABLE(_v),     \
2831                               JSVAL_TRACE_KIND(_v));           \
2832             }                                                  \
2833         }                                                      \
2834     JS_END_MACRO

(Column-79-aligned continuation characters are killing me here, I had to realign the entire thing!)

|_vp = fp->slots, _end = fp->slots + nslots| but |nslots == (fp->regs->sp - fp->slots)| on 32-bit, so |_end = fp->slots + fp->regs->sp - fp->slots| and therefore |_end = fp->slots|, so the loop halts immediately.  Of course, on 64-bit |0 - pointer| produces bignum, and the truncation to 32-bit kills the alignment of the stars that happens on 32-bit.

Comment 4

[Jeff Walden [:Waldo]]

The js_watch_set fake-up code:

634                     memset(&frame, 0, sizeof(frame));
635                     frame.script = script;
636                     frame.regs = NULL;
637                     frame.fun = fun;
638                     frame.argv = argv + 2;
639                     frame.down = js_GetTopStackFrame(cx);
640                     frame.scopeChain = OBJ_GET_PARENT(cx, closure);
641                     if (script && script->nslots)
642                         frame.slots = argv + slotsStart;
643                     if (script) {
644                         JS_ASSERT(script->length >= JSOP_STOP_LENGTH);
645                         regs.pc = script->code + script->length
646                                   - JSOP_STOP_LENGTH;
647                         regs.sp = NULL;
648                         frame.regs = &regs;
649                         if (fun &&
650                             JSFUN_HEAVYWEIGHT_TEST(fun->flags) &&
651                             !js_GetCallObject(cx, &frame)) {
652                             if (argv != smallv)
653                                 cx->free(argv);
654                             DBG_LOCK(rt);
655                             DropWatchPointAndUnlock(cx, wp, JSWP_HELD);
656                             return JS_FALSE;
657                         }
658                     }

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: So, fix?

The pseudo-frame is all about making the rest of the engine happy. This seems like the easiest way to do that.

That being said, there may be light at the end of the tunnel in bug 504021 which gives caps the ability to clamp a given context's principal. We'd need an API in the engine calling out to caps, but it would let us get rid of this pseudo frame which has been nothing but trouble since day one.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 400961 [details] [diff] [review]
So, fix?

Sure, why not?

Comment 7

[Jesse Ruderman]

../jsdbgapi.cpp: In function ‘JSBool js_watch_set(JSContext*, JSObject*, jsval, jsval*)’:
../jsdbgapi.cpp:647: error: ‘fp’ was not declared in this scope
make[1]: *** [jsdbgapi.o] Error 1
make: *** [default] Error 2

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/2f592bea272b with the obvious (and tested on my other computer fix to comment 7).

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

bug 517041 is why not. bug 516911 is the real solution.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/2f592bea272b

Comment 11

[Daniel Veditz [:dveditz]]

Is this something we need to worry about on older branches, or was it introduced recently? Some of the regressions from this were marked blocking1.9.2+, does it go back further than that?

Comment 12

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/aec8df4a92eb

Comment 13

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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