32823 – Various designmode="on"/"off" & execCommand("Undo") NULL pointer crashes

RESOLVED FIXED 32823

Various designmode="on"/"off" & execCommand("Undo") NULL pointer crashes

https://bugs.webkit.org/show_bug.cgi?id=32823

Summary Various designmode="on"/"off" & execCommand("Undo") NULL pointer crashes

Berend-Jan Wever

Reported 2009-12-21 07:17:57 PST

Created attachment 45327 [details] Repro - WebCore::caretMaxOffset ReadAV@NULL (5785e692fef72522c3ee976bff525d98) Repro: <BODY></BODY> <SCRIPT> document.execCommand("SelectAll",""); document.designMode= "on"; document.execCommand("InsertHorizontalRule",""); document.execCommand("CreateLink",false, 171); document.execCommand("InsertorderedList",false,undefined); document.execCommand("Unlink",5); document.execCommand("Indent",false,""); document.designMode=""; document.execCommand("Undo",false,""); document.designMode="on"; document.execCommand("ForeColor",false,3); </SCRIPT>

Attachments
Repro - WebCore::caretMaxOffset ReadAV@NULL (5785e692fef72522c3ee976bff525d98) (474 bytes, text/html) 2009-12-21 07:17 PST, Berend-Jan Wever	no flags	Details
reduced test case with comments (519 bytes, text/html) 2009-12-30 09:33 PST, Mike Moretti	no flags	Details
Repro - WebCore::Node::hasTagName ReadAV@NULL (106220ce302426f16b811b1cc735e87a) (277 bytes, text/html) 2010-07-27 02:41 PDT, Berend-Jan Wever	no flags	Details
Repro - WebCore::ApplyStyleCommand::applyBlockStyle ReadAV@NULL (eba56e4a5c0f11cedaba7aea0468d82a) (506 bytes, text/html) 2010-07-27 02:47 PDT, Berend-Jan Wever	no flags	Details
Repro - WebCore::ReplaceSelectionCommand::doApply ReadAV@NULL (badacdea771d24a7e25e9ab3859da7a2) (277 bytes, text/html) 2010-07-27 02:48 PDT, Berend-Jan Wever	no flags	Details
Repro - WebCore::positionInParentBeforeNode ReadAV@NULL (98882320ea2b731f10f353b23849bd6a) (1.24 KB, text/html) 2010-07-27 02:53 PDT, Berend-Jan Wever	no flags	Details
Repro - WebCore::ApplyStyleCommand::removeInlineStyle ReadAV@NULL (18fa82a60d48c7965db66b916a546328) (357 bytes, text/html) 2010-07-27 03:02 PDT, Berend-Jan Wever	no flags	Details
Repro - WebCore::executeToggleStyleInList ReadAV@NULL (95ebfffa464da44f33912d9597442825) (354 bytes, text/html) 2010-07-27 03:04 PDT, Berend-Jan Wever	no flags	Details
fixes the crash (20.76 KB, patch) 2010-07-27 15:24 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch (18.01 KB, patch) 2010-08-03 17:22 PDT, Ryosuke Niwa	darin: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Berend-Jan Wever

Comment 1 2009-12-30 03:24:12 PST

I think the below is a very similar bug, probably same root cause: WebCore::highestAncestor ReadAV@NULL (6fd56a0a3c08931c7b81ea35e70a582a) <BODY></BODY> <SCRIPT> document.designMode="on"; document.execCommand("selectall",Infinity); document.execCommand("subscript",false-5); document.execCommand("Indent",false,1); document.designMode=""; document.execCommand("Undo",""); document.designMode="on"; document.execCommand("JustifyNone",false,""); </SCRIPT> I cannot seem to reproduce it in recent builds of Chromium, just reporting in case.

Mike Moretti

Comment 2 2009-12-30 09:33:12 PST

Created attachment 45668 [details] reduced test case with comments I've been looking into this bug and have a question. What is the expected functionality of execCommand when there is no "contenteditable" field and "designmode" is off? Should it do anything at all in this case? Should only certain commands be allowed to execute? I know there is a command table with specific "enabled" checks specified in it, but is it correct for the undo case? enabledUndo seems to only call "canUndo" on the frame; shouldn't it also be checking if enabledInEditableText? I.e., should undo be disabled for the case when there is no editable field or we're not in designmode (as in this test case), or does it need to function (and the crash really need to be fixed)?

Berend-Jan Wever

Comment 3 2009-12-31 02:25:30 PST

*** Bug 32822 has been marked as a duplicate of this bug. ***

Berend-Jan Wever

Comment 4 2009-12-31 02:32:09 PST

*** Bug 32424 has been marked as a duplicate of this bug. ***

Berend-Jan Wever

Comment 5 2009-12-31 02:32:19 PST

*** Bug 32425 has been marked as a duplicate of this bug. ***

Berend-Jan Wever

Comment 6 2009-12-31 02:32:39 PST

*** Bug 33049 has been marked as a duplicate of this bug. ***

Berend-Jan Wever

Comment 7 2010-03-05 07:16:18 PST

Appears to be fixed - I cannot repro in Chrome 4.0.249.89 unknown (38071)

Berend-Jan Wever

Comment 8 2010-03-12 09:12:50 PST

It appears this is not completely fixed; I am finding new examples such as this one: <BODY></BODY> <SCRIPT> document.designMode = "on"; document.execCommand("selectall"); document.execCommand("InsertParagraph"); document.execCommand("inserthorizontalrule"); document.designMode = ""; document.execCommand("Undo"); document.designMode = "on"; document.execCommand("Strikethrough"); </SCRIPT> Causes: id: WebCore::executeToggleStyleInList ReadAV@NULL (95ebfffa464da44f33912d9597442825) description: Attempt to read from NULL pointer in WebCore::executeToggleStyleInList stack: WebCore::executeToggleStyleInList WebCore::executeStrikethrough WebCore::Editor::Command::execute WebCore::Document::execCommand WebCore::DocumentInternal::execCommandCallback v8::internal::HandleApiCallHelper<0> v8::internal::Builtin_HandleApiCall v8::internal::Invoke v8::internal::Execution::Call v8::Script::Run WebCore::V8Proxy::runScript WebCore::V8Proxy::evaluate WebCore::ScriptController::evaluate WebCore::ScriptController::executeScript WebCore::ScriptController::executeScript WebCore::ScriptController::executeIfJavaScriptURL WebCore::FrameLoader::changeLocation WebCore::RedirectScheduler::timerFired WebCore::Timer<WebCore::RedirectScheduler>::fired WebCore::ThreadTimers::sharedTimerFiredInternal WTF::ThreadSpecific<WebCore::ThreadGlobalData>::operator WebCore::ThreadGlobalData * MessageLoop::RunTask MessageLoop::DoWork base::MessagePumpDefault::Run MessageLoop::RunInternal MessageLoop::Run RendererMain ChromeMain MainDllLoader::Launch wWinMain __tmainCRTStartup BaseProcessStart+0x23

Berend-Jan Wever

Comment 9 2010-05-04 01:21:48 PDT

Forgot to update the status in previous comment :(

Berend-Jan Wever

Comment 10 2010-07-05 03:44:51 PDT

I just filed bug 41601, which is probably a duplicate of this and indicates this may be a security issue.

Berend-Jan Wever

Comment 11 2010-07-27 02:41:56 PDT

Created attachment 62665 [details] Repro - WebCore::Node::hasTagName ReadAV@NULL (106220ce302426f16b811b1cc735e87a) I'm going to be uploading repros for all crashes I find with this general structure: designmode = on execCommand... designmode = off execCommand(undo) designmode = on execCommand... I expect they are all manifestations of similar issues and that the code needs an overhaul, given the number of bugs I am finding. (But I cannot exclude that this may all be caused by only one bug in the code).

Berend-Jan Wever

Comment 12 2010-07-27 02:47:19 PDT

Created attachment 62666 [details] Repro - WebCore::ApplyStyleCommand::applyBlockStyle ReadAV@NULL (eba56e4a5c0f11cedaba7aea0468d82a)

Berend-Jan Wever

Comment 13 2010-07-27 02:48:17 PDT

Created attachment 62667 [details] Repro - WebCore::ReplaceSelectionCommand::doApply ReadAV@NULL (badacdea771d24a7e25e9ab3859da7a2)

Berend-Jan Wever

Comment 14 2010-07-27 02:53:35 PDT

Created attachment 62668 [details] Repro - WebCore::positionInParentBeforeNode ReadAV@NULL (98882320ea2b731f10f353b23849bd6a)

Berend-Jan Wever

Comment 15 2010-07-27 03:02:59 PDT

Created attachment 62669 [details] Repro - WebCore::ApplyStyleCommand::removeInlineStyle ReadAV@NULL (18fa82a60d48c7965db66b916a546328)

Berend-Jan Wever

Comment 16 2010-07-27 03:04:35 PDT

Created attachment 62670 [details] Repro - WebCore::executeToggleStyleInList ReadAV@NULL (95ebfffa464da44f33912d9597442825)

Roland Steiner

Comment 17 2010-07-27 03:08:47 PDT

[CC'ing rniwa for added editing-fu.] I had a brief look at the bug previously - the fundamental problem is what Mike Moretti described in comment #2. The problem is that execCommand("undo") is prevented, which results in an inconsistent state. As Mike wrote, the question is how to best handle this case (and also the case where the contentEditable attribute is being modified).

Darin Adler

Comment 18 2010-07-27 10:01:03 PDT

Lets not overstate the case here. I think we can fix these all in a pretty straightforward manner without an "overhaul".

Darin Adler

Comment 19 2010-07-27 10:01:16 PDT

But having all in a single bug report may be impractical.

Ryosuke Niwa

Comment 20 2010-07-27 11:54:41 PDT

Every reproduction steps until comment #11 do not crash on TOT.

Darin Adler

Comment 21 2010-07-27 11:58:35 PDT

So it seems we have crash logs here, but we are not clear on which reproduction steps go with those logs. There’s a chance this is all fixed already on TOT, but we can’t be sure until we have a particular set of steps to reproduce that definitely cause a crash. Having all these crashes in one bug report is indeed causing confusion and making it hard to narrow this down, so we probably need to make a simpler bug report with one set of steps to reproduce and one crash. If these do indeed have security implications, then this bug should be in the security component instead of in the open.

Ryosuke Niwa

Comment 22 2010-07-27 13:22:47 PDT

It seems like the crash in #11 is caused by our updating selection after undo becauese the nodes pointed by the selection are not in the document.

Ryosuke Niwa

Comment 23 2010-07-27 14:47:05 PDT

#12 doesn't reproduce, but #11, #13-16 are the same type of crashes. They're all caused by changeSelectionAfterCommand incorrectly updating the selection. A patch coming.

Ryosuke Niwa

Comment 24 2010-07-27 15:24:01 PDT

Created attachment 62756 [details] fixes the crash Fixed the crash by adding a check in changeSelectionAfterCommand. The fix is simple and small. I included all tests that crash TOT.

Berend-Jan Wever

Comment 25 2010-07-28 08:14:51 PDT

@Darin: * Sorry for the confusion: I am NOT suggesting we NEED to overhaul the code. I am hoping it is one bug in the code triggered in various ways. However, considering how many crashes I am finding, I am worried that the code may need a little bit extra attention. So, I was trying to suggest that the code is reviewed to make sure it is in good overall shape before closing this bug. * About all the repros: If this is indeed only one bug in the code, we would know when a fix stops all repros from crashing. If it turns out that a fix does not fix all crashes, you can eitehr take the repros that still trigger crashes and move them to a new bug, or continue to fix the code until all crashes are gone. Because I do not know how many bugs there actually are in the code, I have opted to put all repros in one bug: it is the easiest way to make sure we do not lose track of any crashes, so all of them get fixed. * Everything I uploaded that starts with "Repro - " is what I would call a repro: each contains HTML and JavaScript that triggers a crash for me in latest Chromium. I'm not sure what you mean by "crash log". * I have been fuzzing this for months and only found NULL pointers. I am not saying there is a security issue hiding somewhere in the code, but I have failed to find any, hence marked as non-security. @Ryosuke: * I assume this was indeed one bug that could be triggered in various ways, causing the NULL pointers to happen in various other parts of the code and that this is indeed not a security issue? * I also assuem that your fix prevents all repros from crashing in one fell swoop?

Ryosuke Niwa

Comment 26 2010-07-28 09:59:02 PDT

On second thought, I don't think my approach was right here. Let me explain my thoughts as I rely comment #25. (In reply to comment #25) > @Darin: > * Sorry for the confusion: I am NOT suggesting we NEED to overhaul the code. I am hoping it is one bug in the code > triggered in various ways. However, considering how many crashes I am finding, I am worried that the code may need > a little bit extra attention. So, I was trying to suggest that the code is reviewed to make sure it is in good > overall shape before closing this bug. Indeed, all bugs were triggered by undo restoring selection incorrectly. Namely, it was setting the start or the end of selection to a node that's not attached to the document but solely kept alive for undo/redo purposes. And we probably need to come up with a way to cleanly deal with undo/redo inconsistency as well as JavaScript interference during executing composite editing commands. > * About all the repros: If this is indeed only one bug in the code, we would know when a fix stops all repros from > crashing. If it turns out that a fix does not fix all crashes, you can eitehr take the repros that still trigger > crashes and move them to a new bug, or continue to fix the code until all crashes are gone. > Because I do not know how many bugs there actually are in the code, I have opted to put all repros in one bug: it is > the easiest way to make sure we do not lose track of any crashes, so all of them get fixed. So here's a thing. This bug report addresses multiple bugs simultaneously by its own nature because the crashes we are seeing here are caused because 1. Undo restores selection incorrectly to detached nodes. (This does not cause crash by itself). 2. Some editing command can't handle orphaned selections and crashes. My patch certainly fix the problem 1 but does not fix problem 2. What we really need to do here is to go look at each test, and find a way to reproduce the same problem without using undo, e.g. by creating orphaned selection artificially. I'm not sure if this is possible yet, so I'll investigate into this. > * Everything I uploaded that starts with "Repro - " is what I would call a repro: each contains HTML and JavaScript > that triggers a crash for me in latest Chromium. I'm not sure what you mean by "crash log". I think he meant the stack trace. > * I have been fuzzing this for months and only found NULL pointers. I am not saying there is a security issue hiding > somewhere in the code, but I have failed to find any, hence marked as non-security. Indeed, there's no security implications as far as I know. > @Ryosuke: > * I assume this was indeed one bug that could be triggered in various ways, causing the NULL pointers to happen in > various other parts of the code and that this is indeed not a security issue? Yes, the details explained above. > * I also assuem that your fix prevents all repros from crashing in one fell swoop? So my patch has pros and cons. My patch on one hand fixes the crashes by solving the problem 1 but also hides problem 2. But editing commands shouldn't be crashing regardless. So I'll try to come up with tests that cause the same kind of crash for each case, and then prepare a separate test that ensures that undo doesn't restore selection incorrectly.

Ryosuke Niwa

Comment 27 2010-07-28 10:01:43 PDT

Ugh... I misread your comment. > * I assume this was indeed one bug that could be triggered in various ways, causing the NULL pointers to happen in > various other parts of the code and that this is indeed not a security issue? No, the opposite is true. There was one non-crash bug that triggered various crash bugs.

Ryosuke Niwa

Comment 28 2010-07-28 10:23:22 PDT

On another second thought, if either end of selection points to a detached node, it doesn't make much sense to call almost any editing command. So maybe my fix is okay after all. Adding more editing people to see if anyone can comment on this point.

Darin Adler

Comment 29 2010-07-28 11:00:01 PDT

(In reply to comment #28) > On another second thought, if either end of selection points to a detached node, it doesn't make much sense to call almost any editing command. So maybe my fix is okay after all. Yes, that makes sense. The editing command layer can just check for this type of selection and not do any command at all. That should get rid of the crashes, and then we may still have bugs where selection gets into this state, but they should be non-crashing issues.

Ryosuke Niwa

Comment 30 2010-07-28 11:18:46 PDT

(In reply to comment #29) > (In reply to comment #28) > > On another second thought, if either end of selection points to a detached node, it doesn't make much sense to call almost any editing command. So maybe my fix is okay after all. > > Yes, that makes sense. The editing command layer can just check for this type of selection and not do any command at all. That should get rid of the crashes, and then we may still have bugs where selection gets into this state, but they should be non-crashing issues. So do you want me to add two fixes here? One in restoring selection and another in EditCommand to prevent calling any command if selection is orphaned. But I feel like that might break some commands because some simple commands don't refer to selection, and some composite commands may call these simple commands before restoring the selection properly. What do you think?

Darin Adler

Comment 31 2010-07-28 11:26:17 PDT

I don’t think a fix in EditCommand is necessarily a good idea. The individual commands should probably have the fixes instead. I want the check to be as close as possible to the code that relies on assumption in the check, not in some higher level place making assumptions about all editing commands. However, I do think that there could be two types of fixes. 1) Mechanical fixes that prevent the code from crashing in a way that’s clearly correct and closely related to the code that would crash, but may give suboptimal editing behavior. 2) Higher level fixes that give more logical results in strange cases where the low level fixes might lead to unexpected behavior and we can do better.

Ryosuke Niwa

Comment 32 2010-07-28 11:46:04 PDT

(In reply to comment #31) > I don’t think a fix in EditCommand is necessarily a good idea. The individual commands should probably have the fixes instead. We're on the same page. > I want the check to be as close as possible to the code that relies on assumption in the check, not in some higher level place making assumptions about all editing commands. Yeah, I'd like that too. > However, I do think that there could be two types of fixes. > > 1) Mechanical fixes that prevent the code from crashing in a way that’s clearly correct and closely related to the code that would crash, but may give suboptimal editing behavior. For example #11 crash happens after http://trac.webkit.org/browser/trunk/WebCore/editing/ReplaceSelectionCommand.cpp#824 because DeleteSelection clears up the selection and #825 obtains a null visible position. This clearly is a problem but I'm not sure if it's the scope of this bug to fix this kind of bug. We might want to file a separate bug or wait until some test in which this condition holds (i.e. selection existed before the call to DeleteSelection but became null afterwards) by putting ASSERT here for now. > 2) Higher level fixes that give more logical results in strange cases where the low level fixes might lead to unexpected behavior and we can do better. By higher level fixes, you mean something like I did in my patch to prevent undo from restoring bad selection? Or you mean that exiting early at the beginning of each command that relies on selection?

Ryosuke Niwa

Comment 33 2010-08-03 16:53:49 PDT

SkyLined, could you unmark this bug as a security-related? As far as I'm concerned, there are no "exploitable" crashes here.

Ryosuke Niwa

Comment 34 2010-08-03 17:22:39 PDT

Created attachment 63394 [details] Patch

Ryosuke Niwa

Comment 35 2010-08-03 17:25:43 PDT

Darin, I added check for various editing commands. But it turns out that at least 2 tests I'm adding crashes inside the changeSelectionAfterCommand when it tries to create a range to verify the selection so my change in changeSelectionAfterCommand is still necessary to fix this bug.

Darin Adler

Comment 36 2010-08-24 12:49:03 PDT

Comment on attachment 63394 [details] Patch > + if (!m_selectionToDelete.isRange() || m_selectionToDelete.start().isOrphan() || m_selectionToDelete.end().isOrphan()) I'd like to see a helper function for this so we don't have to repeat these three checks every time. One version for the isNone and another for the isRange variant I guess.

Ryosuke Niwa

Comment 37 2010-08-25 12:16:40 PDT

Thanks for the review, Darin. I really appreciate it. (In reply to comment #36) > (From update of attachment 63394 [details]) > > + if (!m_selectionToDelete.isRange() || m_selectionToDelete.start().isOrphan() || m_selectionToDelete.end().isOrphan()) > > I'd like to see a helper function for this so we don't have to repeat these three checks every time. One version for the isNone and another for the isRange variant I guess. Will add isNonOrphanedRange and isNonOrphanedCaretOrRange to VisibleSelection.

Ryosuke Niwa

Comment 38 2010-08-25 12:19:40 PDT

Committed r66032: <http://trac.webkit.org/changeset/66032>

David Kilzer (:ddkilzer)

Comment 39 2011-03-15 10:07:23 PDT

<rdar://problem/9135704>

Ahmad Saleem

Comment 40 2022-06-07 15:50:31 PDT

Hi Team - is it by design that execCommand('undo') can reopen closed tabs in Safari? Because in (non-Private) Safari window, I can reopen closed tabs using single line of HTML from below JSFiddle: https://jsfiddle.net/tg3njfmu/1/show Thanks!

Ahmad Saleem

Comment 41 2022-06-07 16:01:21 PDT

Just to update in all other browser irrespective of Private or Incognito - "undo" button does not reopen old tabs. Further, I noticed that Chrome did some changes on their end to make it on per-frame level rather than per-tab level from security perspective here - https://chromium.googlesource.com/chromium/src/+/512508f0d652a006407ce66aafcd339b296a5276 I know from Interop perspective "execCommand" are still under investigation and there are test cases lacking from WPT but I think Safari should be consistent in behavior to other browsers here and does not open "closed tabs" using HTML. Thanks!

Darin Adler

Comment 42 2022-06-07 16:33:38 PDT

Adding those comments to a 12-year-old bug about null pointer crashes is probably not the right way to make progress. Consider filing a new bug, in the security component perhaps?

Note You need to log in before you can comment on or make changes to this bug.