157845 - Crash involving document.open()

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Jesse Ruderman]

To reproduce:
1. Load the testcase.
2. Type something into the text field. (Empty will not crash.)
3. Press enter.

This is a scary crash.  The top of the stack, equal to the EIP, is a bogus 
address.  Changing the testcase changes the EIP.  If I don't change the 
testcase, the EIP doesn't change.  I don't know if I can make EIP be anything I 
want.  The crash dialog says "The instruction at '0x51000024' referenced memory 
at '0x51000024'. The memory could not be 'read'."  I am filing this bug as 
security-sensitive.

The testcase is the simplest I could find:
Move form onsubmit to input onkeyup      no crash
Don't type anything in the text field    no crash
Remove position:absolute style           crash becomes unreliable or happens 
later
Remove javascript error                  no crash
Remove document.open                     no crash
Replace document.open with location=     no crash


Talkbalk: 8386540, 8351815, 8385459


Top of the stack for original page (8351815):

0x51000024 
nsEventStateManager::GetEventTargetContent 
[c:/builds/seamonkey/mozilla/content/events/src/nsEventStateManager.cpp, line 
3515] 
nsDOMEvent::GetTarget 
[c:/builds/seamonkey/mozilla/content/events/src/nsDOMEvent.cpp, line 336] 
nsXULElement::HandleDOMEvent 
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp, line 
3411] 
nsXULElement::HandleChromeEvent 
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp, line 
4676] 
GlobalWindowImpl::HandleDOMEvent 
[c:/builds/seamonkey/mozilla/dom/src/base/nsGlobalWindow.cpp, line 741] 
NS_ScriptErrorReporter 
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp, line 187] 
js_ReportErrorAgain [c:/builds/seamonkey/mozilla/js/src/jscntxt.c, line 688] 
js_ReportUncaughtException [c:/builds/seamonkey/mozilla/js/src/jsexn.c, line 
1056] 


Full stack for attached testcase (8386540):

0x0000078f 
nsEventStateManager::GetEventTargetContent 
[c:/builds/seamonkey/mozilla/content/events/src/nsEventStateManager.cpp, line 
3515] 
nsDOMEvent::GetTarget 
[c:/builds/seamonkey/mozilla/content/events/src/nsDOMEvent.cpp, line 336] 
nsXULElement::HandleDOMEvent 
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp, line 
3411] 
nsXULElement::HandleChromeEvent 
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp, line 
4676] 
GlobalWindowImpl::HandleDOMEvent 
[c:/builds/seamonkey/mozilla/dom/src/base/nsGlobalWindow.cpp, line 741] 
NS_ScriptErrorReporter 
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp, line 187] 
js_ReportErrorAgain [c:/builds/seamonkey/mozilla/js/src/jscntxt.c, line 688] 
ReportError [c:/builds/seamonkey/mozilla/js/src/jscntxt.c, line 327] 
js_ReportErrorNumberVA [c:/builds/seamonkey/mozilla/js/src/jscntxt.c, line 645] 
JS_ReportErrorNumber [c:/builds/seamonkey/mozilla/js/src/jsapi.c, line 3706] 
js_ReportUncaughtException [c:/builds/seamonkey/mozilla/js/src/jsexn.c, line 
1050] 
JS_CallFunctionValue [c:/builds/seamonkey/mozilla/js/src/jsapi.c, line 3433] 
nsJSContext::CallEventHandler 
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1045] 
nsJSEventListener::HandleEvent 
[c:/builds/seamonkey/mozilla/dom/src/events/nsJSEventListener.cpp, line 184] 
nsEventListenerManager::HandleEventSubType 
[c:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp, 
line 1222] 
nsEventListenerManager::HandleEvent 
[c:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp, 
line 1832] 
nsGenericElement::HandleDOMEvent 
[c:/builds/seamonkey/mozilla/content/base/src/nsGenericElement.cpp, line 1819] 
nsHTMLFormElement::HandleDOMEvent 
[c:/builds/seamonkey/mozilla/content/html/content/src/nsHTMLFormElement.cpp, 
line 632] 
PresShell::HandleDOMEventWithTarget 
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6240] 
nsHTMLInputElement::HandleDOMEvent 
[c:/builds/seamonkey/mozilla/content/html/content/src/nsHTMLInputElement.cpp, 
line 1610] 
PresShell::HandleEventInternal 
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6192] 
PresShell::HandleEvent 
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6115] 
nsViewManager::HandleEvent 
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 2039] 
nsView::HandleEvent [c:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 306] 
nsViewManager::DispatchEvent 
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 1896] 
HandleEvent [c:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 83] 
nsWindow::DispatchEvent 
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1037] 
nsWindow::DispatchWindowEvent 
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1054] 
nsWindow::DispatchKeyEvent 
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 2832] 
nsWindow::OnChar [c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, 
line 3002] 
nsWindow::ProcessMessage 
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 3648] 
nsWindow::WindowProc 
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1299] 
USER32.DLL + 0x1b60 (0x77e11b60) 
USER32.DLL + 0x1cca (0x77e11cca) 
USER32.DLL + 0x83f1 (0x77e183f1) 
nsAppShellService::Run 
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp, line 458] 
main1 [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1472] 
main [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1808] 
WinMain [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1826] 
WinMainCRTStartup() 
KERNEL32.DLL + 0xd326 (0x77e8d326)

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Perhaps related to bug 135498?

Comment 3

[Brendan Eich [:brendan]]

Seems to me jst should be on the cc list, if not owning the bug.

Jesse mentioned this bug to me yesterday, didn't remember to cc: me, so I'm
adding myself.  Setting keywords.

/be

Comment 4

[Jesse Ruderman]

Original page in debug build (top of stack is *not* a bogus address):

nsEventStateManager::GetEventTargetContent(nsEventStateManager * const 
0x038046b8, nsEvent * 0x0012e4d0, nsIContent * * 0x0012df18) line 3516 + 33 
bytes
nsDOMEvent::GetTarget(nsDOMEvent * const 0x038b63d8, nsIDOMEventTarget * * 
0x0012e294) line 333 + 54 bytes

Testcase in debug build:

cccccccc()
nsEventStateManager::GetEventTargetContent(nsEventStateManager * const 
0x03902bc8, nsEvent * 0x0012e438, nsIContent * * 0x0012de80) line 3517

Does the 0xcccccccc provide any hints?

Comment 5

[Johnny Stenback (:jst)]

Uninitialized variables on the stack (on Win32 at least) will have the value
0xCCCCCCCC in debug builds.

Comment 6

[georgi - hopefully not receiving bugspam]

0xcc is also the opcode for software breakpoint on x86 (int 3 I think).
Executing it on linux gives 
"Trace/breakpoint trap"

Comment 7

[Jaime Rodriguez, Jr.]

jst: is this one you should own, as brendan suggest? if so, can you pls take it
from joki. thanks!

Comment 8

[Johnny Stenback (:jst)]

This is looking like a false alarm to me, this crash is from us calling a method
on a pointer to a deleted frame and I don't see how this would be any different
from a security point of view from any other similar bugs that we have tons. I'm
opening this up as a normal bug and I suggest that we don't worry about this for
MachV.

This crash is due to nsEventStateManager's mCurrentFrame pointing at a deleted
frame, but I don't know why. I'm leaving this on joki's list since it's in his
code, and he'd be the best person to look at this.

Comment 9

[Johnny Stenback (:jst)]

s/tons/tons of/

Comment 10

[joki (gone)]

Attachment: Proposed patch

I agree with jst that this probably isn't a security issue.  It is, however, a
fairly important crash.  I believe this issue as another topcrasher (bug
135498) that I've been trying to reproduce.  This one reproduced easily and
gave me a fix that may fix the topcrash as well.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 92264 [details] [diff] [review]
Proposed patch

sr=jst

Seems like this is a change that we should push in on the branch as well. This
could fix other random crashes as well.

Comment 12

[saari (gone)]

Comment on attachment 92264 [details] [diff] [review]
Proposed patch

r=saari

Comment 13

[chris hofmann]

Comment on attachment 92264 [details] [diff] [review]
Proposed patch

a=chofmann for trunk and 1.0.1. add the fixed1.0.1 keyword after checking into
branch.

Comment 14

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch. pls check
this in asap, then replace "mozilla1.0.1" with
"fixed1.0.1". thanks!

Comment 15

[Jaime Rodriguez, Jr.]

joki/saari - would it be possible to get this landed today? thanks!

Comment 16

[joki (gone)]

Checked into trunk and branch.

Comment 17

[Vladimir Ermakov]

verifying on 2002-08-01-08-trunk on linux red hat and 2002-07-25-14-trunk on win 2k

Comment 18

[Jaime Rodriguez, Jr.]

vladimire: can you pls verify this as fixed for the 1.0 branch, as well, then
replace "fixed1.0.1" with "verified1.0.1"? thanks!

Comment 19

[Vladimir Ermakov]

verifying on 2002-08-07-08-1.0 branch on win 2k