Closed Bug 533705 Opened 11 years ago Closed 11 years ago

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


(Core :: JavaScript Engine, defect, P1)




Tracking Status
blocking2.0 --- alpha1+
status1.9.2 --- unaffected
status1.9.1 --- unaffected


(Reporter: cbook, Assigned: jorendorff)




(Keywords: crash, testcase, Whiteboard: [crashkill-automation][fixed-in-tracemonkey][sg:critical])


(2 files)

Reproduced with a recent Firefox 3.7 Trunk Debug build

Load : 
-> 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

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
(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
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.
Assigning to jorendorff because he's the expert on this stuff.
Assignee: general → jorendorff
I just hit this in regular browsing. In bug 533945 Tomcat hit it on Facebook.
Flags: blocking1.9.2?
Actually I hit the assertion "slot == scope->freeslot" failure, which Brendan duped to this.
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 "", 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 "", aLineNo=1, aVersion=0, aRetValue=0x0, aIsUndefined=0xbfffd1e4) at /Users/roc/mozilla-checkin/dom/base/nsJSEnvironment.cpp:1713
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.

OS: Windows XP → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a1
Attached patch proposed fixSplinter Review
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.

Attachment #417876 - Flags: review?(jorendorff)
Comment on attachment 417876 [details] [diff] [review]
proposed fix

r=me with some comments, which I'll provide. Thanks for the patch.
Attachment #417876 - Flags: review?(jorendorff) → review+
Flags: blocking1.9.2? → blocking1.9.2+
Actually I checked in less delta to jsops.cpp than the patch has.
Whiteboard: [crashkill-automation] → [crashkill-automation][fixed-in-tracemonkey]
(In reply to comment #10)
> Actually I checked in less delta to jsops.cpp than the patch has.

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?

Closed: 11 years ago
Resolution: --- → FIXED
(In reply to comment #13)
> (In reply to comment #12)
> >
> 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?
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.

This doesn't bite on the branch, because the no-middle-deletes patch didn't land there.
Flags: wanted1.9.2?
Flags: blocking1.9.2-
Flags: blocking1.9.2+
Depends on: 538275
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.
The cset that hit tm is now in m-c:

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

If I'm interpreting comment 16 right, the branches are unaffected so we can now unhide this bug?
blocking2.0: ? → alpha1
Duplicate of this bug: 533945
Group: core-security
Keywords: testcase
Whiteboard: [crashkill-automation][fixed-in-tracemonkey] → [crashkill-automation][fixed-in-tracemonkey][sg:critical]
Flags: wanted1.9.2?
Automatically extracted testcase for this bug was committed:
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.