Summary: | Null Ptr Deref @ WebCore::ReplaceSelectionCommand::doApply | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ian Gilbert <iang> | ||||||||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, product-security, rbuis, rniwa, rwlbuis, svillar, webkit-bug-importer, wenson_hsieh, youennf | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Local Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Ian Gilbert
2020-11-03 02:40:34 PST
Created attachment 413027 [details]
Crashing Input
Created attachment 413269 [details]
Reduced crashing input
Created attachment 413970 [details]
More reduced testcase
Created attachment 418028 [details]
Patch
I'm including the test assuming there's no security implication here, since it's just another null check missing. Please confirm.
Comment on attachment 418028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418028&action=review Yep, this test can be included. WebKitLegacy seems to be asserting in fragmentNeedsColorTransformed. We should probably fix that too before landing. The null check seems fine though. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1208 > + if (insertionPos.containerNode() && insertionPos.containerNode()->isTextNode() && insertionPos.offsetInContainerNode() && !insertionPos.atLastEditingPositionForNode()) { We could use if-with-initializer to avoid calling containerNode twice. (In reply to Alex Christensen from comment #7) > Comment on attachment 418028 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418028&action=review > > Yep, this test can be included. > WebKitLegacy seems to be asserting in fragmentNeedsColorTransformed. We > should probably fix that too before landing. The null check seems fine > though. Yes, it's here: RefPtr<Element> editableRoot = insertionPos.rootEditableElement(); ASSERT(editableRoot); if (!editableRoot) return false; I think the assert is wrong and the early return correct, so we can just remove the assert. > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1208 > > + if (insertionPos.containerNode() && insertionPos.containerNode()->isTextNode() && insertionPos.offsetInContainerNode() && !insertionPos.atLastEditingPositionForNode()) { > > We could use if-with-initializer to avoid calling containerNode twice. Sure. Created attachment 418119 [details]
Patch
Now it's hitting another assert, but I can't reproduce it this time. I'll investigate. Comment on attachment 418119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418119&action=review > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1209 > + splitTextNode(*insertionPos.containerText(), insertionPos.offsetInContainerNode()); Do we have a guarantee that insertionPos.containerText() cannot be null? Is it ensured from containerNode->isTextNode()? Looking at Position::containerText(), it seems to depend on anchorType being PositionIsOffsetInAnchor. I was running the wrong test, I can indeed reproduce the new assert too. (In reply to youenn fablet from comment #11) > Comment on attachment 418119 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418119&action=review > > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1209 > > + splitTextNode(*insertionPos.containerText(), insertionPos.offsetInContainerNode()); > > Do we have a guarantee that insertionPos.containerText() cannot be null? Is > it ensured from containerNode->isTextNode()? > Looking at Position::containerText(), it seems to depend on anchorType being > PositionIsOffsetInAnchor. Not an expert in editing code, but I think it's the whole condition: containerNode->isTextNode() && insertionPos.offsetInContainerNode() && !insertionPos.atLastEditingPositionForNode() Created attachment 418137 [details]
Patch for landing
commit-queue failed to commit attachment 418137 [details] to WebKit repository.
Committed r271787: <https://trac.webkit.org/changeset/271787> |