Closed Bug 481139 Opened 13 years ago Closed 13 years ago

Crash [@ nsWSRunObject::GetNextWSNode] with contenteditable, changing root

Categories

(Core :: DOM: Editor, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: smaug)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, regression, testcase, Whiteboard: [sg:critical?])

Crash Data

Attachments

(2 files)

I've been hitting this a lot lately.
(I tested this on latest-trunk mozilla-central nightly build on WinXP SP3)

Turning security-sensitive and nominating blocking1.9.1? just-in-case, due to
!exploitable PROBABLY_EXPLOITABLE result.


0:000> !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:105f2668 mov edx,dword ptr [eax]

Basic Block:
    105f2668 mov edx,dword ptr [eax]
       Tainted Input Operands: eax
    105f266a push ecx
    105f266b mov ecx,eax
       Tainted Input Operands: eax
    105f266d call dword ptr [edx+44h]
       Tainted Input Operands: ecx, edx

Exception Hash (Major/Minor): 0x195a0d66.0x53611e5d

Stack Trace:
xul!nsWSRunObject::GetNextWSNode+0x7f
xul!nsWSRunObject::GetNextWSNode+0x33
xul!nsWSRunObject::GetWSNodes+0x4de
xul!nsWSRunObject::nsWSRunObject+0x7b
xul!nsHTMLEditor::NormalizeEOLInsertPosition+0x35
xul!nsHTMLEditor::InsertElementAtSelection+0x1cf
xul!nsInsertTagCommand::DoCommand+0xbc
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
js3250!js_DefineNativeProperty+0x73d
js3250!js_DefineProperty+0x34
js3250!JS_DefinePropertyById+0x2f
xul!DefinePropertyIfFound+0x34d
xul!XPC_WN_CallMethod+0x114
js3250!js_Invoke+0x43b
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: 0x105f2668

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!nsWSRunObject::GetNextWSNode+0x7f (Hash=0x195a0d66.0x53611e5d)

The data from the faulting address is later used as the target for a branch.
Group: core-security
Flags: blocking1.9.1?
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [sg:critical?]
Assignee: nobody → Olli.Pettay
Attached patch null checkSplinter Review
After thinking few options, null check is really the safest here.
Shouldn't regress anything, but does fix the crash.
Attachment #369877 - Flags: superreview?(peterv)
Attachment #369877 - Flags: review?(peterv)
aStartNode is document here.
Whiteboard: [sg:critical?] → [sg:nse] null deref
Whiteboard: [sg:nse] null deref → [sg:critical?]
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Would it work to QI to nsINode?
I was thinking that, but it wouldn't be as safe as this fix, since it would 
introduce new behavior in some cases.
This is just a null pointer crash, but perhaps we want this at least for
1.9.1, so I'd like to keep changes at minimum
Blocks: 485540
Comment on attachment 369877 [details] [diff] [review]
null check

Please file a follow-up bug to QI to nsINode and add a comment here that refers to that bug.
Attachment #369877 - Flags: superreview?(peterv)
Attachment #369877 - Flags: superreview+
Attachment #369877 - Flags: review?(peterv)
Attachment #369877 - Flags: review+
Bug 487796
http://hg.mozilla.org/mozilla-central/rev/1a8349d0cff8
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Crash Signature: [@ nsWSRunObject::GetNextWSNode]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.