461049 - Crash [@ nsTransactionItem::UndoChildren] or "ASSERTION: no frame, see bug #188946: 'frame'" with mutation events

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I get:

###!!! ASSERTION: no frame, see bug #188946: 'frame', file c:/tracemonkey/editor
/libeditor/base/nsEditor.cpp, line 4101

on Windows XP, latest TM debug build.

Stack Trace:
ntdll!DbgBreakPoint+0x0
xpcom_core!Break+0x233
xpcom_core!NS_DebugBreak_P+0x2b2
gklayout!nsEditor::IsPreformatted+0xe1
gklayout!nsWSRunObject::GetRuns+0x2c
gklayout!nsWSRunObject::nsWSRunObject+0xda
gklayout!nsHTMLEditor::BeginningOfDocument+0x135
gklayout!nsEditor::Init+0x25f
gklayout!nsPlaintextEditor::Init+0x8f
gklayout!nsHTMLEditor::Init+0xc2
composer!nsEditingSession::SetupEditorOnWindow+0xb9a
composer!nsEditingSession::MakeWindowEditable+0x276
gklayout!nsHTMLDocument::EditingStateChanged+0x250
gklayout!nsHTMLDocument::MaybeEditingStateChanged+0x48
gklayout!nsHTMLDocument::EndUpdate+0x1b
gklayout!mozAutoDocUpdate::~mozAutoDocUpdate+0x37
gklayout!nsContentSink::NotifyAppend+0x8d
gklayout!HTMLContentSink::OpenBody+0x1c6
gklayout!HTMLContentSink::OpenContainer+0xbb
gkparser!CNavDTD::OpenBody+0xa7
gkparser!CNavDTD::OpenContainer+0x140
gkparser!CNavDTD::HandleDefaultStartToken+0x3e4
gkparser!CNavDTD::HandleStartToken+0x382
gkparser!CNavDTD::HandleToken+0x49d
gkparser!CNavDTD::BuildModel+0x295
gkparser!nsParser::BuildModel+0xe2
gkparser!nsParser::ResumeParse+0x1bc
gkparser!nsParser::OnDataAvailable+0x228
docshell!nsDocumentOpenInfo::OnDataAvailable+0x4c
necko!nsBaseChannel::OnDataAvailable+0x6d
necko!nsInputStreamPump::OnStateTransfer+0x23d
necko!nsInputStreamPump::OnInputStreamReady+0x80
xpcom_core!nsInputStreamReadyEvent::Run+0x4a
xpcom_core!nsThread::ProcessNextEvent+0x1fa
xpcom_core!NS_ProcessNextEvent_P+0x53
gkwidget!nsBaseAppShell::Run+0x5d
tkitcmps!nsAppStartup::Run+0x6b
xul!XRE_main+0x3388
firefox!NS_internal_main+0x2b2
firefox!wmain+0x119
firefox!__tmainCRTStartup+0x1a8
firefox!wmainCRTStartup+0xf
kernel32!BaseProcessStart+0x23
Instruction Address: 0x7c90120e

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(I tested this on a mozilla-central latest-trunk nightly build in WinXP SP3)

!exploitable shows this is probably exploitable - turning security-sensitive and nominating blocking1.9.1?

1:002> !load winext\msec.dll
1:002> !exploitable -v
HostMachine\HostUser
Executing Processor Architecture is x86
Debuggee is in User Mode
Debuggee is a live user mode debugging session on the local machine
Event Type: Exception
*** WARNING: Unable to verify checksum for C:\Documents and Settings\Administrator\Desktop\firefox\js3250.dll
Exception Faulting Address: 0x0
First Chance Exception Type: STATUS_ACCESS_VIOLATION (0xC0000005)
Exception Sub-Type: Read Access Violation

Faulting Instruction:104a714f mov edx,dword ptr [ebx]

Basic Block:
    104a714f mov edx,dword ptr [ebx]
       Tainted Input Operands: ebx
    104a7151 push eax
    104a7152 mov ecx,ebx
       Tainted Input Operands: ebx
    104a7154 call dword ptr [edx+8]
       Tainted Input Operands: ecx, edx

Exception Hash (Major/Minor): 0x335d4e0e.0x7f684e36

Stack Trace:
xul!nsTransactionItem::UndoChildren+0x8e
xul!nsTransactionItem::UndoTransaction+0x10
xul!nsTransactionManager::UndoTransaction+0x102
xul!nsEditor::Undo+0x59
xul!nsPlaintextEditor::Undo+0x83
xul!nsUndoCommand::DoCommand+0x34
xul!nsControllerCommandTable::DoCommand+0x48
xul!nsBaseCommandController::DoCommand+0x64
xul!nsCommandManager::DoCommand+0x81
xul!nsHTMLDocument::ExecCommand+0x176
xul!NS_InvokeByIndex_P+0x27
xul!XPCWrappedNative::CallMethod+0x4fb
xul!NS_NewAtom+0x46
xul!nsCOMPtr_base::~nsCOMPtr_base+0xe
xul!nsDocumentSH::NewResolve+0x78
xul!nsHTMLDocumentSH::NewResolve+0x83
xul!XPCCallContext::XPCCallContext+0x118
js3250!js_LookupPropertyWithFlags+0x454
js3250!js_Interpret+0x31d0
js3250!js_Invoke+0x289
js3250!js_InternalInvoke+0x102
js3250!JS_CallFunctionValue+0x27
xul!nsJSContext::CallEventHandler+0x162
xul!nsJSEventListener::HandleEvent+0x230
xul!nsEventListenerManager::HandleEventSubType+0x6b
xul!nsEventListenerManager::HandleEvent+0x222
xul!nsEventTargetChainItem::HandleEvent+0xb4
xul!nsEventTargetChainItem::HandleEventTargetChain+0x1eb
xul!nsEventDispatcher::Dispatch+0x4f5
xul!DocumentViewerImpl::LoadComplete+0x10c
xul!nsDocShell::EndPageLoad+0x63
xul!nsWebShell::EndPageLoad+0xa9
xul!nsDocShell::OnStateChange+0x83
xul!nsDocLoader::FireOnStateChange+0x123
xul!nsDocLoader::doStopDocumentLoad+0x1c
xul!nsDocLoader::DocLoaderIsEmpty+0x130
xul!nsDocLoader::OnStopRequest+0xc8
xul!nsLoadGroup::RemoveRequest+0xb8
xul!nsDocument::DoUnblockOnload+0x3b
Instruction Address: 0x104a714f

Description: Data from Faulting Address controls Code Flow
Short Description: TaintedDataControlsCodeFlow
Exploitability Classification: PROBABLY_EXPLOITABLE
Recommended Bug Title: Probably Exploitable - Data from Faulting Address controls Code Flow starting at xul!nsTransactionItem::UndoChildren+0x8e (Hash=0x335d4e0e.0x7f684e36)

The data from the faulting address is later used as the target for a branch.

Comment 3

[Olli Pettay [:smaug]]

Attachment: null checks

As far as I see, this is just a null pointer crash.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #3)
> Created an attachment (id=369875) [details]
> null checks
> 
> As far as I see, this is just a null pointer crash.

Changing to "[sg:nse] null deref" on whiteboard, please correct me if otherwise.

Comment 5

[Boris Zbarsky [:bzbarsky]]

While that patch looks ok, the stack in comment 2 is somewhat busted.  Specifically this part:

xul!NS_InvokeByIndex_P+0x27
xul!XPCWrappedNative::CallMethod+0x4fb
xul!NS_NewAtom+0x46
xul!nsCOMPtr_base::~nsCOMPtr_base+0xe
xul!nsDocumentSH::NewResolve+0x78

Anyone know what the deal there is?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, and as far as "just a null pointer crash", this is a virtual function call on a null pointer.  That ought to be exploitable, just like the recent XSLT thing was, no?  Or did I misunderstand something about that bug?

Comment 7

[Olli Pettay [:smaug]]

Ah, right, virtual call, yes.

Comment 8

[Daniel Veditz [:dveditz]]

In the XSLT case the object wasn't null, the vtable gave it a null to call. This one really looks like a null-deref to me, especially given the patch. We crash loading the null item in order to get which function to call.

!exploitable thinks this is PROBABLY EXPLOITABLE because it doesn't trust the source of the null: it KNOWS null is bad, what other bad values could get in there?. If item is completely bogus and just happens to be null in this testcase then it _is_ possibly exploitable, but this patch isn't going to help us one bit when the exploitable case comes along.

If item is always either valid or null then this is the right patch and this bug is a sg:dos null deref.

Where does item come from? Is Peek() correct in returning success AND a null item or should that be an error? Is that causing problems elsewhere? Is the code that allows pushing a null item the problem?

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 369875 [details] [diff] [review]
null checks

>diff --git a/editor/txmgr/src/nsTransactionItem.cpp b/editor/txmgr/src/nsTransactionItem.cpp

>     while (sz-- > 0) {
>       result = mUndoStack->Peek(getter_AddRefs(item));
> 
>-      if (NS_FAILED(result)) {
>+      if (NS_FAILED(result) || !item) {
>         return result;
>       }
> 

Maybe make this
    while (NS_SUCCEEDED(mUndoStack->GetItem(--sz, getter_AddRefs(item)))) {

>   while (sz-- > 0) {
>     result = mRedoStack->Peek(getter_AddRefs(item));
> 
>-    if (NS_FAILED(result)) {
>+    if (NS_FAILED(result) || !item) {
>       return result;
>     }

Same here.

Comment 10

[Olli Pettay [:smaug]]

(In reply to comment #9)
> Maybe make this
"Maybe make" or "make"?

Comment 11

[Olli Pettay [:smaug]]

(In reply to comment #8)
> Where does item come from? Is Peek() correct in returning success AND a null
> item or should that be an error? Is that causing problems elsewhere?
Peek() does return null/NS_OK when the "stack" is empty. Null item is checked elsewhere.

Comment 12

[Olli Pettay [:smaug]]

http://hg.mozilla.org/mozilla-central/rev/25773d9b3c8b

Comment 13

[David Baron :dbaron:]

Comment on attachment 369875 [details] [diff] [review]
null checks

a1.9.1=dbaron

Comment 14

[Olli Pettay [:smaug]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4b0256b82089

Comment 15

[Daniel Veditz [:dveditz]]

Given the answer in comment 11 and the patch I'm going with unexploitable DoS. Would be a nice safe fix to have on the 1.9.0 branch.

Comment 16

[Daniel Veditz [:dveditz]]

Not blocking, but do want a patch.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 369875 [details] [diff] [review]
null checks

I assume this patch is still good for the 1.9.0 branch?

Comment 18

[Samuel Sidler (old account; do not CC)]

Comment on attachment 369875 [details] [diff] [review]
null checks

Going on Dan's assumption... Approved for 1.9.0.10. a=ss

Comment 19

[Bob Clary [:bc] (inactive)]

v 1.9.0, 1.9.1, 1.9.2

2009-04-17 06:07	Olli.Pettay%helsinki.fi
mozilla/editor/txmgr/src/nsTransactionItem.cpp 	1.22

Comment 20

[Samuel Sidler (old account; do not CC)]

This doesn't crash Firefox 2 and is not wanted on 1.8.1.

Comment 21

[Jesse Ruderman]

Crashtest: http://hg.mozilla.org/mozilla-central/rev/297949c05cb7