Summary: | WebCore crashes when element is removed from within 2-dimensional mouse wheel event handler. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pavel Feldman <pfeldman> | ||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, adele, aestes, ap, commit-queue, darin, eric, hyatt, sam | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Pavel Feldman
2010-02-08 02:16:59 PST
Sounds like use of freed memory, which could be exploitable for remote code execution. Also seems like using RefPtr instead of a raw pointer may be almost enough to fix it. Maybe I'm missing something, but it doesn't seem like scrollAndAcceptEvent actually dispatches an event. I'm not sure how to remove the node at that point. Do you have a test case? (In reply to comment #4) > Maybe I'm missing something, but it doesn't seem like scrollAndAcceptEvent > actually dispatches an event. I'm not sure how to remove the node at that > point. Do you have a test case? I did not manage to come up with reduction, so here is a larger app it crashes with: http://repenaxa.com/viewer/crash.html (make sure both scrollers are visible, then make diagonal scroll on MBP). Removing node in between of those lines was a wild guess. It is just that I know that I am removing nodes in the scroll event. I can see wheel event being dispatched before both invocations though (EventHandler:1855 though). The renderer is nil in scrollAndAcceptEvent, but the node is still present. I think scrollAndAcceptEvent just needs to check for that. I don't think this is a security bug. I'll send a fix out soon. Adding that nil check in scrollAndAcceptEvent fixes the crash, so making a test is the hard part here. Created attachment 48734 [details]
test case
I don't think there's any way to turn this into a layout test without enhancing DRT. I think you need to send a wheel event, and initWheelEvent isn't exposed in JS.
Created attachment 48736 [details]
patch
parking this here in case I don't get around to fix DRT soon.
It’s kind of strange that the wheel event is one kind you can't create from the DOM. The problem is that the DOM event-creating API we implemented is sort of a patchwork of things found in various versions of the DOM events standard and none included the wheel event. Adele pointed me to http://www.w3.org/TR/DOM-Level-3-Events/#events-wheelevents, which I'm going to attempt to implement. This will help test both this and https://bugs.webkit.org/show_bug.cgi?id=29601. By the way, I can no longer repro the crash. Is it just me? Did the page change? (In reply to comment #12) > Adele pointed me to > http://www.w3.org/TR/DOM-Level-3-Events/#events-wheelevents, which I'm going to > attempt to implement. This will help test both this and > https://bugs.webkit.org/show_bug.cgi?id=29601. > > By the way, I can no longer repro the crash. Is it just me? Did the page > change? Page did not change, crash is still there on ToT on my MBP. you should be able to repro with the attached test case too. It can be a little tricky sometimes to make sure you're scrolling diagonally. (In reply to comment #12) > Adele pointed me to > http://www.w3.org/TR/DOM-Level-3-Events/#events-wheelevents, which I'm going to > attempt to implement. This will help test both this and > https://bugs.webkit.org/show_bug.cgi?id=29601. > > By the way, I can no longer repro the crash. Is it just me? Did the page > change? Created attachment 50139 [details]
Patch with test case
Comment on attachment 50139 [details]
Patch with test case
You should file bugs for the disabled tests and the missing DRT implementations for the other platforms. And did you confirm that this test will crash without the WebCore patch? I'll cq+ this after that answer...
Comment on attachment 50139 [details]
Patch with test case
Andy said "yes" to my previous question and will file those follow-up bugs
Sadly the commit-queue does not work with security patches as the commit-bot account does not have security rights. This is not a security bug, so I'm moving the component. This bug contains security-related information of a sensitive nature. will need to be unchecked for the commit-queue to see it. Comment on attachment 50139 [details] Patch with test case Rejecting patch 50139 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Adele Peterson', '--force']" exit_code: 1 Last 500 characters of output: ests/platform/gtk/Skipped Hunk #1 FAILED at 5803. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/gtk/Skipped.rej patching file LayoutTests/platform/mac-tiger/Skipped patching file LayoutTests/platform/qt/Skipped Hunk #1 FAILED at 5095. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej patching file LayoutTests/platform/win/Skipped Hunk #1 FAILED at 774. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/win/Skipped.rej Full output: http://webkit-commit-queue.appspot.com/results/342205 Created attachment 50275 [details] Patch with test case (v2) Resolved the conflicts in the Skipped files and re-baselined the patch against r55704. Comment on attachment 50275 [details] Patch with test case (v2) Rejecting patch 50275 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Adele Peterson', '--force']" exit_code: 1 Last 500 characters of output: e LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/events/remove-child-onscroll-expected.txt patching file LayoutTests/fast/events/remove-child-onscroll.html patching file LayoutTests/platform/gtk/Skipped patching file LayoutTests/platform/mac-tiger/Skipped patching file LayoutTests/platform/qt/Skipped Hunk #1 FAILED at 5103. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej patching file LayoutTests/platform/win/Skipped Full output: http://webkit-commit-queue.appspot.com/results/475001 Comment on attachment 50275 [details]
Patch with test case (v2)
Clearing the commit-queue flag. I'll just commit this myself
Sorry for the long delay in response. The commit-queue had some trouble with bug 35905 yesterday. It's running again fine now. Committed revision 55737. The commit-queue just failed it's "does the current trunk build work" check: fast/events/remove-child-onscroll.html -> failed I assume it's related to this patch and that the bots will roll red shortly. :( ok will look into it Missed checking in the DRT changes - Committed revision 55739. |