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>
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.
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)?
*** Bug 32822 has been marked as a duplicate of this bug. ***
*** Bug 32424 has been marked as a duplicate of this bug. ***
*** Bug 32425 has been marked as a duplicate of this bug. ***
*** Bug 33049 has been marked as a duplicate of this bug. ***
Appears to be fixed - I cannot repro in Chrome 4.0.249.89 unknown (38071)
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
Forgot to update the status in previous comment :(
I just filed bug 41601, which is probably a duplicate of this and indicates this may be a security issue.
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).
Created attachment 62666 [details] Repro - WebCore::ApplyStyleCommand::applyBlockStyle ReadAV@NULL (eba56e4a5c0f11cedaba7aea0468d82a)
Created attachment 62667 [details] Repro - WebCore::ReplaceSelectionCommand::doApply ReadAV@NULL (badacdea771d24a7e25e9ab3859da7a2)
Created attachment 62668 [details] Repro - WebCore::positionInParentBeforeNode ReadAV@NULL (98882320ea2b731f10f353b23849bd6a)
Created attachment 62669 [details] Repro - WebCore::ApplyStyleCommand::removeInlineStyle ReadAV@NULL (18fa82a60d48c7965db66b916a546328)
Created attachment 62670 [details] Repro - WebCore::executeToggleStyleInList ReadAV@NULL (95ebfffa464da44f33912d9597442825)
[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).
Lets not overstate the case here. I think we can fix these all in a pretty straightforward manner without an "overhaul".
But having all in a single bug report may be impractical.
Every reproduction steps until comment #11 do not crash on TOT.
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.
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.
#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.
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.
@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?
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.
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.
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.
(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.
(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?
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.
(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?
SkyLined, could you unmark this bug as a security-related? As far as I'm concerned, there are no "exploitable" crashes here.
Created attachment 63394 [details] Patch
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.
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.
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.
Committed r66032: <http://trac.webkit.org/changeset/66032>
<rdar://problem/9135704>
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!
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!
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?