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)
Core
DOM: Editor
Tracking
()
VERIFIED
FIXED
People
(Reporter: jruderman, Assigned: smaug)
References
Details
(6 keywords, Whiteboard: [sg:dos])
Crash Data
Attachments
(2 files)
517 bytes,
text/html
|
Details | |
1.05 KB,
patch
|
peterv
:
review+
peterv
:
superreview+
dbaron
:
approval1.9.1+
samuel.sidler+old
:
approval1.9.0.11+
|
Details | Diff | Splinter Review |
No description provided.
![]() |
||
Comment 1•16 years ago
|
||
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
![]() |
||
Comment 2•16 years ago
|
||
(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?
![]() |
||
Updated•16 years ago
|
Whiteboard: [sg:critical?]
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → Olli.Pettay
Assignee | ||
Comment 3•16 years ago
|
||
As far as I see, this is just a null pointer crash.
Attachment #369875 -
Flags: superreview?(peterv)
Attachment #369875 -
Flags: review?(peterv)
![]() |
||
Comment 4•16 years ago
|
||
(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
![]() |
||
Comment 5•16 years ago
|
||
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•16 years ago
|
||
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]
Assignee | ||
Comment 7•16 years ago
|
||
Ah, right, virtual call, yes.
Comment 8•16 years ago
|
||
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 9•16 years ago
|
||
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+
Assignee | ||
Comment 10•16 years ago
|
||
(In reply to comment #9)
> Maybe make this
"Maybe make" or "make"?
Assignee | ||
Comment 11•16 years ago
|
||
(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.
Assignee | ||
Comment 12•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
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+
Assignee | ||
Comment 14•16 years ago
|
||
Keywords: fixed1.9.1
Comment 15•16 years ago
|
||
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]
Comment 17•16 years ago
|
||
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 18•16 years ago
|
||
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+
Assignee | ||
Updated•16 years ago
|
Keywords: fixed1.9.0.10
Comment 19•16 years ago
|
||
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
Comment 20•16 years ago
|
||
This doesn't crash Firefox 2 and is not wanted on 1.8.1.
Flags: wanted1.8.1.x-
Updated•16 years ago
|
Keywords: regression
Updated•16 years ago
|
Group: core-security
Reporter | ||
Comment 21•16 years ago
|
||
Flags: in-testsuite+
Updated•14 years ago
|
Crash Signature: [@ nsTransactionItem::UndoChildren]
You need to log in
before you can comment on or make changes to this bug.
Description
•