533705 - Possible Stack Corruption starting at ntdll!DbgBreakPoint +0x0000000000000000 called from mozjs!js_AddProperty+0x000000000000006b

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Carsten Book [:Tomcat]]

Reproduced with a recent Firefox 3.7 Trunk Debug build

STR:
Load : 
-> http://www.mapquest.com/maps?1c=Kannapolis&1s=NC&1a=153+Fairmont+Cir&1z=28083-7819&1y=US&1l=35.486246&1g=-80.602619&1v=ADDRESS&2c=Concord&2s=NC&2a=77+Union+St+S&2z=28025-5013&2y=US&2l=35.40922&2g=-80.579819&2v=ADDRESS
-> Crash on load after a few seconds

Exploitability Classification: UNKNOWN
Recommended Bug Title: Possible Stack Corruption starting at ntdll!DbgBreakPoint
+0x0000000000000000 called from mozjs!js_AddProperty+0x000000000000006b (Hash=0x
23154549.0x2e043a68)

The stack trace contains one or more locations for which no symbol or module cou
ld be found. This may be a sign of stack corruption.
ChildEBP RetAddr
WARNING: Stack unwind information not available. Following frames may be wrong.
0012e3e8 00668d0b ntdll!DbgBreakPoint
0012e414 0639ecaa mozjs!js_AddProperty+0x6b
0012e484 005ffb6c 0x639ecaa
0012e494 005ff57c mozjs!ExecuteTrace+0x3c
0012e540 00602483 mozjs!ExecuteTree+0x20c
0012e598 00530e83 mozjs!js_MonitorLoopEdge+0x2f3
0012ebcc 0052dce4 mozjs!js_Interpret+0x1b53
0012ec5c 004d5099 mozjs!js_Execute+0x424
0012ec84 0284afe8 mozjs!JS_EvaluateUCScriptForPrincipals+0xe9
0012ed40 026a92c7 gklayout!nsJSContext::EvaluateString+0x328
0012ee38 026a8c9f gklayout!nsScriptLoader::EvaluateScript+0x377
0012eefc 026a806a gklayout!nsScriptLoader::ProcessRequest+0x10f
0012f3fc 02bb82b9 gklayout!nsScriptLoader::ProcessScriptElement+0xc3a
0012f430 02bd0374 gklayout!nsScriptElement::MaybeProcessScript+0x149
0012f4e8 02bd008f gklayout!nsHTMLScriptElement::MaybeProcessScript+0x24
0012f4f4 02767cff gklayout!nsHTMLScriptElement::DoneAddingChildren+0x1f
0012f518 02762bbd gklayout!HTMLContentSink::ProcessSCRIPTEndTag+0xcf
0012f54c 02765ff0 gklayout!SinkContext::CloseContainer+0x31d
0012f564 01e5fb7a gklayout!HTMLContentSink::CloseContainer+0xa0
0012f594 01e5cc00 htmlpars!CNavDTD::CloseContainer+0x18a
quit:

Comment 1

[Carsten Book [:Tomcat]]

(5b0.7bc): Break instruction exception - code 80000003 (!!! second chance !!!)
eax=0000006e ebx=0ad3e700 ecx=461e4ead edx=10313d38 esi=0ad3f1c0 edi=051783e0
eip=7c90120e esp=0012e3e4 ebp=0012e3e8 iopl=0         nv up ei pl nz na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000206

Comment 2

[David Mandelin [:dmandelin]]

Attachment: Reduced shell test case

Here is a reduced test case. Brendan spotted the cause of the bug over my shoulder: we have two functions that have the same shape, but different numbers of reserved slots. Thus, the freeslot in the object is different. 

I tried to write a patch to fix this by adding the reserved slot count to JSEmptyScope and sharing scopes only if the reserved slot count is the same. It didn't work because I was calling obj->getClass()->reserveSlots in createEmptyScope for that check, but some reserveSlots functions (e.g., args_reserveSlots) require a fully initialized object, which we don't have yet at that point.

Comment 3

[David Mandelin [:dmandelin]]

Assigning to jorendorff because he's the expert on this stuff.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I just hit this in regular browsing. In bug 533945 Tomcat hit it on Facebook.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually I hit the assertion "slot == scope->freeslot" failure, which Brendan duped to this.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

FWIW this is my stack:

#3  0x003b6790 in JS_Assert (s=0x4530f0 "slot == scope->freeslot", file=0x46636c "/Users/roc/mozilla-checkin/js/src/jsbuiltins.cpp", ln=238) at /Users/roc/mozilla-checkin/js/src/jsutil.cpp:70
#4  0x0044542e in js_AddProperty (cx=0x22536e00, obj=0xe53d2a0, sprop=0x222ab410) at /Users/roc/mozilla-checkin/js/src/jsbuiltins.cpp:238
#5  0x0bb68953 in ?? ()
#6  0x003d6f25 in ExecuteTrace (cx=0x22536e00, f=0x32377504, state=@0xbfffc7f0) at /Users/roc/mozilla-checkin/js/src/jstracer.cpp:6469
#7  0x003e74a3 in ExecuteTree (cx=0x22536e00, f=0x32377504, inlineCallCount=@0xbfffcc20, innermostNestedGuardp=0xbfffc8c8) at /Users/roc/mozilla-checkin/js/src/jstracer.cpp:6567
#8  0x0040561a in js_MonitorLoopEdge (cx=0x22536e00, inlineCallCount=@0xbfffcc20, reason=Record_Branch) at /Users/roc/mozilla-checkin/js/src/jstracer.cpp:7057
#9  0x0030e655 in js_Interpret (cx=0x22536e00) at jsops.cpp:360
#10 0x00335b1c in js_Execute (cx=0x22536e00, chain=0xe568580, script=0x266a5400, down=0x0, flags=0, result=0x0) at jsinterp.cpp:1619
#11 0x002aa1ed in JS_EvaluateUCScriptForPrincipals (cx=0x22536e00, obj=0xe568580, principals=0x3358c7b4, chars=0xa9ff008, length=52323, filename=0x2dce10c8 "http://static.thebigchair.com.au/mycareerjobbox/script/fd.mt.scroller.js", lineno=1, rval=0x0) at /Users/roc/mozilla-checkin/js/src/jsapi.cpp:5057
#12 0x02ceeb07 in nsJSContext::EvaluateString (this=0xff0bf40, aScript=@0x272a2ec4, aScopeObject=0xe568580, aPrincipal=0x3358c7b0, aURL=0x2dce10c8 "http://static.thebigchair.com.au/mycareerjobbox/script/fd.mt.scroller.js", aLineNo=1, aVersion=0, aRetValue=0x0, aIsUndefined=0xbfffd1e4) at /Users/roc/mozilla-checkin/dom/base/nsJSEnvironment.cpp:1713

Comment 7

[Brendan Eich [:brendan]]

This seems like a bad bug. Should it block 1.9.2 or did other factors keep it from looming large there?

Patch coming up, my hg blame is all over this -- Jason, I hope it's ok to put up a patch. I'm on vacation and will be afk a lot, so if you could review and test and land, I would be grateful.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix

The slotchanges property cache event was completely bogus, it papered over this bug. I got rid of it. There's a minimal fix to OOM_EXIT from js_AddProperty on a slot change but I think the interpreter needs fixing too, instead of trying to slam in a new entry vword (sprop2).

Separate fix: must lock proto-scope across canProvideEmptyScope/getEmptyScope.

/be

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 417876 [details] [diff] [review]
proposed fix

r=me with some comments, which I'll provide. Thanks for the patch.

Comment 10

[Jason Orendorff [:jorendorff]]

Actually I checked in less delta to jsops.cpp than the patch has.

http://hg.mozilla.org/tracemonkey/rev/74ad683e3ae2

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> Actually I checked in less delta to jsops.cpp than the patch has.
> 
> http://hg.mozilla.org/tracemonkey/rev/74ad683e3ae2

This leaves entry->vword and sprop differing, which does not break anything (yet) AFAICS in TR::record_SetPropHit, TR::setProp, etc. -- but it is a bit scary, or at least worrying. Comment-worthy, at least?

/be

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/74ad683e3ae2

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> http://hg.mozilla.org/mozilla-central/rev/74ad683e3ae2

What about comment 11?

/be

Comment 14

[Robert Sayre]

(In reply to comment #13)
> (In reply to comment #12)
> > http://hg.mozilla.org/mozilla-central/rev/74ad683e3ae2
> 
> What about comment 11?
> 
> /be

I think jorendorff took off for vacation right before you left comment 11. Is it ok to just file a follow up on him to write a comment?

Comment 15

[Brendan Eich [:brendan]]

It's not a comment but agreement between the data structures maintained by the interpreter and used by the recorder and on trace. IIRC Jason agreed on IRC, the effect would be the original patch landing. But I'll let Jason check me on that, followup bug is fine.

/be

Comment 16

[Robert Sayre]

This doesn't bite on the branch, because the no-middle-deletes patch didn't land there.

Comment 17

[Jason Orendorff [:jorendorff]]

Was there a followup bug for this? I agree landing the original patch is the right thing but I don't know if it ever happened.

Comment 18

[Brendan Eich [:brendan]]

The cset that hit tm is now in m-c:

http://hg.mozilla.org/mozilla-central/rev/74ad683e3ae2

Need a followup per comment 15. Jason, will you file and patch? Thanks,

/be

Comment 19

[Daniel Veditz [:dveditz]]

If I'm interpreting comment 16 right, the branches are unaffected so we can now unhide this bug?

Comment 21

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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