Closed Bug 461049 Opened 17 years ago Closed 16 years ago

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

Categories

(Core :: DOM: Editor, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: smaug)

References

Details

(6 keywords, Whiteboard: [sg:dos])

Crash Data

Attachments

(2 files)

No description provided.
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
Keywords: assertion
OS: Mac OS X → All
Hardware: x86 → All
Summary: Crash [@ nsTransactionItem::UndoChildren] with mutation events → Crash [@ nsTransactionItem::UndoChildren] or "ASSERTION: no frame, see bug #188946: 'frame'" with mutation events
(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.
Group: core-security
Flags: blocking1.9.1?
Whiteboard: [sg:critical?]
Assignee: nobody → Olli.Pettay
Attached patch null checksSplinter Review
As far as I see, this is just a null pointer crash.
Attachment #369875 - Flags: superreview?(peterv)
Attachment #369875 - Flags: review?(peterv)
(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.
Whiteboard: [sg:critical?] → [sg:nse] null deref
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?
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?
Whiteboard: [sg:nse] null deref → [sg:critical]
Ah, right, virtual call, yes.
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?
Whiteboard: [sg:critical] → [sg:investigate]
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.
Attachment #369875 - Flags: superreview?(peterv)
Attachment #369875 - Flags: superreview+
Attachment #369875 - Flags: review?(peterv)
Attachment #369875 - Flags: review+
(In reply to comment #9) > Maybe make this "Maybe make" or "make"?
(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.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attachment #369875 - Flags: approval1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Comment on attachment 369875 [details] [diff] [review] null checks a1.9.1=dbaron
Attachment #369875 - Flags: approval1.9.1? → approval1.9.1+
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.
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.10?
Whiteboard: [sg:investigate] → [sg:dos]
Not blocking, but do want a patch.
Flags: blocking1.9.0.10?
Comment on attachment 369875 [details] [diff] [review] null checks I assume this patch is still good for the 1.9.0 branch?
Attachment #369875 - Flags: approval1.9.0.10?
Comment on attachment 369875 [details] [diff] [review] null checks Going on Dan's assumption... Approved for 1.9.0.10. a=ss
Attachment #369875 - Flags: approval1.9.0.10? → approval1.9.0.10+
Keywords: fixed1.9.0.10
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
Status: RESOLVED → VERIFIED
This doesn't crash Firefox 2 and is not wanted on 1.8.1.
Flags: wanted1.8.1.x-
Group: core-security
Flags: in-testsuite+
Crash Signature: [@ nsTransactionItem::UndoChildren]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: