273963 - function arg/var vs. "static property" collision hard case runs off obj->slots

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Due to jsinterp.c:SetFunctionSlot calling js_ChangeScopePropertyAttrs to go from
slot-less to slot-full.  js_ChangeScopePropertyAttrs botches by following
scope->object back to the prototype when calling js_AllocSlot.  Fix is to call
js_GetMutableScope in SetFunctionSlot before changing attributes.

Could corrupt the heap => crash bug.

/be

Comment 1

[Brendan Eich [:brendan]]

Er, all we need is to call js_ChangeNativePropertyAttrs instead of
js_ChangeScopePropertyAttrs.  js_ChangeNativePropertyAttrs does the
js_GetMutableScope for us, as you'd expect for a jsobj.c:js_<verb>Native<noun>.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

Comment 3

[Dylan Salisbury]

After applying the patch, we quickly hit an assert in the JS_UNLOCK_SCOPE call
shortly after the changed function call.  Now that the object may have a new
scope, do we need to unlock the object's new scope, or do no unlocking here if
the scope has changed?  Or is it more complicated than that -- does the rest of
the for loop still make sense if the object has a new scope?  Here is a partial
stack trace showing the assert.  This core dumped much much more quickly than
previous tests so we think there is no memory corruption before this point but
we haven't proven that yet:

#10 0xdde5c113 in abort () from /usr/lib/libc.so.1
#11 0xde19e464 in JS_Assert (s=0xde1ab86e "scope->u.count > 0", 
    file=0xde1ab6c0 "jslock.c", ln=1085) at jsutil.c:155
#12 0xde15f60f in js_UnlockScope (cx=0x85fbc58, scope=0x10833958)
    at jslock.c:1085
#13 0xde14b178 in SetFunctionSlot (cx=0x85fbc58, obj=0x10404388, 
    setter=0xde14b19c <js_SetArgument>, id=1, v=25) at jsinterp.c:543
#14 0xde14b1c1 in js_SetArgument (cx=0x85fbc58, obj=0x10404388, id=1, 
    vp=0xd96dee74) at jsinterp.c:556
#15 0xde16d055 in js_SetProperty (cx=0x85fbc58, obj=0x10404388, id=222414664, 
    vp=0xd96dee74) at jsobj.c:2757
#16 0xde158393 in js_Interpret (cx=0x85fbc58, result=0xd96df6b0)
    at jsinterp.c:2803
#17 0xde14cc35 in js_Execute (cx=0x85fbc58, chain=0xfd6e4f0, script=0xfde0ad8, 
    down=0x0, special=0, result=0xd96df6b0) at jsinterp.c:1155
#18 0xde11cf76 in JS_ExecuteScript (cx=0x85fbc58, obj=0xfd6e4f0, 
    script=0xfde0ad8, rval=0xd96df6b0) at jsapi.c:3425

Comment 4

[Brendan Eich [:brendan]]

Attachment: oops

Sorry, was going too fast there.  This will work.

/be

Comment 5

[John Bandhauer]

Comment on attachment 168364 [details] [diff] [review]
oops

Looks right to me. We'll let you know how our testing goes.

Comment 6

[Brendan Eich [:brendan]]

Fixed, let me know if this doesn't relieve the dmalloc symptom.

/be