Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14829006:
Use after free in WebCore::dispatchEditableContentChangedEvents (Closed)
|
Created:
7 years, 7 months ago by Will Harris Modified:
7 years, 7 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptiondispatchEvent call can execute javascript and blow away endRoot from underneath. Make the argument startRoot and endRoot as PassRefPtr.
BUG=237429
TEST=webkit/tools/layout_tests/run_webkit_tests.sh editing/undo/undo-after-setting-value.html editing/undo/undo-after-event-edited.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150380
Patch Set 1 #Patch Set 2 : add test for dispatchEvent UAF #
Total comments: 33
Patch Set 3 : fix formatting. remove unneeded exception handlers. simplify calculations #
Messages
Total messages: 20 (0 generated)
please add any blink reviewers if needed...?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wfh@chromium.org/14829006/1
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wfh@chromium.org/14829006/1
I updated the description and subject. Please use detailed description so that someone can look at the codereview and understand the bug.
not lgtm w/o a test.
If this can't be tested (or not reasonably within our tools), that's fine, but we should explain that in the description. I don't mean to block your change, but this one is a bit dubious. 1. It uses PassRefPtr in a slightly odd way (normally you're supposed to immediately stuff them in RefPtrs, as they're very easy to end up clearing by accident). 2. It has no test (or explanation of why no test). Either of those can be fine, but I was a bit surprised to see this going in with both. :) Thanks.
On 2013/05/10 17:11:07, Eric Seidel wrote: > not lgtm w/o a test. This crash came from Bj's fuzzer and minimized testcase is like 6 kb. it didn't minimize furthur and one of the reasons is recursive mutations with DOM events like DOMCharacterDataModified, etc. Will can explain more on this.
Historically we've held the position that if it's not tested, it's broken. :) Obviously with cluster-fuzz, theoretically we have coverage for this and more. If a test case is impractical, that's fine, but often that just means we don't understand the bug and could be making the wrong fix. :)
Anyway, as noted above, I'm not going to stand in the way here. Feel free to lg again and land if you feel testing is impractical/not-worth it here.
On 2013/05/10 17:23:03, Eric Seidel wrote: > Anyway, as noted above, I'm not going to stand in the way here. Feel free to lg > again and land if you feel testing is impractical/not-worth it here. On 2013/05/10 17:21:28, Eric Seidel wrote: > Historically we've held the position that if it's not tested, it's broken. :) > Obviously with cluster-fuzz, theoretically we have coverage for this and more. > > If a test case is impractical, that's fine, but often that just means we don't > understand the bug and could be making the wrong fix. :) I'm pretty sure I can minimise the test case as I stepped through it pretty exhaustively in order to fully understand the bug before pushing the change. I can push this test in if you like, but it's probably a day's work as inferno says the fuzzer is quite recursive. I'll leave it up to you guys.
On 2013/05/10 17:28:32, Will Harris wrote: > On 2013/05/10 17:23:03, Eric Seidel wrote: > > Anyway, as noted above, I'm not going to stand in the way here. Feel free to > lg > > again and land if you feel testing is impractical/not-worth it here. > On 2013/05/10 17:21:28, Eric Seidel wrote: > > Historically we've held the position that if it's not tested, it's broken. :) > > Obviously with cluster-fuzz, theoretically we have coverage for this and more. > > > > If a test case is impractical, that's fine, but often that just means we don't > > understand the bug and could be making the wrong fix. :) > > I'm pretty sure I can minimise the test case as I stepped through it pretty > exhaustively in order to fully understand the bug before pushing the change. I > can push this test in if you like, but it's probably a day's work as inferno > says the fuzzer is quite recursive. I'll leave it up to you guys. Yes, please give it another shot at minimizing the test.
Retried try job too often on mac_layout for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Added a minimized test - still needs two interleaved callbacks to reliably hit so the code is a bit large. Interestingly another test - undo-after-setting-value.html also hits this UAF bug in debug-ASAN but very rarely... I saw it once in all my runs. This newly added test should hit the UAF bug time though.
https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... File LayoutTests/editing/undo/undo-after-event-edited.html (right): https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:1: <head> Please add <!DOCTYPE html> and <html> opening, closing tag. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:3: * {display:run-in;} spaces around { } https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:7: // BUG=237429 No need of bug number here. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:15: var aoScriptElements = document.getElementsByTagName("script"); s/aoScriptElements/scriptElements https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:17: var event_handler_webkitEditableContentChanged_active = false; s/event_handler_webkitEditableContentChanged_active/eventHandleEnabled https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:20: if (event_handler_webkitEditableContentChanged_active) return ; return on new line and indented. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:22: var oElement = event.srcElement; s/oElement/srcElement https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:24: try{oElement.textContent = "PASS"} is try, catch required. if yes, please indent and put on like try { statement; } catch(e) { } https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:24: try{oElement.textContent = "PASS"} s/PASS/Pass. Test didn't crash. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:31: function event_handler_DOMCharacterDataModified() { new lines before and after function declaration. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:35: setTimeout(function(){ space before { https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:36: var oSelection=window.getSelection(); s/oSelection/selection https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:37: if (!oSelection.rangeCount) { no need of braces for single line statmenents. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:40: var oRange = oSelection.rangeCount ? oSelection.getRangeAt(2 % oSelection.rangeCount) : null; s/2 % oSelection.rangeCount/ can you hardcode this value, you can do an alert to see what needs to com ehere. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:42: try{var oDocumentFragment = oRange.extractContents()} is try, catch required here. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:47: no need of new line. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:53: FAIL0 FAIL0, FAIL1 make no sense here. Since this a crasher test, we can just have regular string like ABCD, etc.
fix formatting. remove unneeded exception handlers. simplify calculations. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... File LayoutTests/editing/undo/undo-after-event-edited.html (right): https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:1: <head> On 2013/05/14 18:00:00, inferno wrote: > Please add <!DOCTYPE html> and <html> opening, closing tag. Done. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:3: * {display:run-in;} On 2013/05/14 18:00:00, inferno wrote: > spaces around { } Done. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:7: // BUG=237429 On 2013/05/14 18:00:00, inferno wrote: > No need of bug number here. Done. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:15: var aoScriptElements = document.getElementsByTagName("script"); On 2013/05/14 18:00:00, inferno wrote: > s/aoScriptElements/scriptElements Done. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:17: var event_handler_webkitEditableContentChanged_active = false; On 2013/05/14 18:00:00, inferno wrote: > s/event_handler_webkitEditableContentChanged_active/eventHandleEnabled Done. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:20: if (event_handler_webkitEditableContentChanged_active) return ; On 2013/05/14 18:00:00, inferno wrote: > return on new line and indented. Done. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:22: var oElement = event.srcElement; On 2013/05/14 18:00:00, inferno wrote: > s/oElement/srcElement Done. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:24: try{oElement.textContent = "PASS"} removed exception handler, it never gets used in the test https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:31: function event_handler_DOMCharacterDataModified() { On 2013/05/14 18:00:00, inferno wrote: > new lines before and after function declaration. Done. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:35: setTimeout(function(){ On 2013/05/14 18:00:00, inferno wrote: > space before { Done. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:36: var oSelection=window.getSelection(); On 2013/05/14 18:00:00, inferno wrote: > s/oSelection/selection Done. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:37: if (!oSelection.rangeCount) { On 2013/05/14 18:00:00, inferno wrote: > no need of braces for single line statmenents. Done. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:40: var oRange = oSelection.rangeCount ? oSelection.getRangeAt(2 % oSelection.rangeCount) : null; On 2013/05/14 18:00:00, inferno wrote: > s/2 % oSelection.rangeCount/ can you hardcode this value, you can do an alert to > see what needs to com ehere. Done. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:42: try{var oDocumentFragment = oRange.extractContents()} removed exception handler, it never gets used in the test https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:47: On 2013/05/14 18:00:00, inferno wrote: > no need of new line. Done. https://codereview.chromium.org/14829006/diff/25001/LayoutTests/editing/undo/... LayoutTests/editing/undo/undo-after-event-edited.html:53: FAIL0 On 2013/05/14 18:00:00, inferno wrote: > FAIL0, FAIL1 make no sense here. Since this a crasher test, we can just have > regular string like ABCD, etc. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wfh@chromium.org/14829006/30001
Message was sent while issue was closed.
Change committed as 150380 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
