Bug 160808

Summary: FocusController multiple dereferenced NULL pointers
Product: WebKit Reporter: Jonathan Bedard <jbedard>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: commit-queue
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Jonathan Bedard 2016-08-12 10:49:32 PDT
In FocusController, there are a number of static functions which accept references to objects and are usually called by dereferencing pointers.  Since many of the functions which call these static functions accept pointers, and in at least one case, these pointers can be NULL, these functions are undefined behavior.
Comment 1 Jonathan Bedard 2016-08-12 11:07:48 PDT
Created attachment 285921 [details]
Patch
Comment 2 Jonathan Bedard 2016-08-12 12:00:50 PDT
Created attachment 285925 [details]
Patch
Comment 3 Daniel Bates 2016-08-12 12:50:48 PDT
Comment on attachment 285925 [details]
Patch

This does not seem like the correct approach. We want to move towards more usage of references, not less. Can you please identify the call sites that convert a null pointer to a null-reference?
Comment 4 Jonathan Bedard 2016-08-12 13:07:35 PDT
Here are the two places where the behavior has been observed:

.../FocusController.cpp:672:80: runtime error: reference binding to null pointer of type 'WebCore::KeyboardEvent'
.../FocusController.cpp:586:53: runtime error: reference binding to null pointer of type 'WebCore::KeyboardEvent'

The motivation for this change is that while we check if the Elements are NULL, we basically never do so for the KeyBoardEvents.  Even just a quick overview will show that lines 507, 509, 520, 543, 558, 564, 647, 694, 952 and 704 all have this same problem (note that those line numbers are in the unedited version).  It is true that the Elements are already checked, I thought that the static functions should either both be references or both be pointers, which is why I switched Element references to pointers as well.
Comment 5 Jonathan Bedard 2016-08-12 13:55:43 PDT
A bit of additional information:

The stack (at least for one of the errors) indicates that WebPage.cpp line 2523 contains the call-site where a NULL event is sent into the focus controller in the event that the received keyboard event is invalid.  It is, without a doubt, deliberate behavior.

Furthermore, EventHandler::isKeyboardOptionTab(KeyboardEvent*) seems to be the only function which actually consumes aa keyboard event in this code-path, and it preforms a NULL check on event.
Comment 6 Jonathan Bedard 2016-08-12 14:07:58 PDT
Created attachment 285945 [details]
Patch
Comment 7 Jonathan Bedard 2016-08-24 14:28:15 PDT
rdar://16327307 is also tracking this bug.
Comment 8 Alexey Proskuryakov 2016-08-24 15:33:54 PDT
rdar://problem/16327307
Comment 9 WebKit Commit Bot 2016-08-24 15:35:05 PDT
Comment on attachment 285945 [details]
Patch

Rejecting attachment 285945 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 285945, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/1936400
Comment 10 Jonathan Bedard 2016-08-24 15:45:08 PDT
Created attachment 286900 [details]
Patch
Comment 11 WebKit Commit Bot 2016-08-24 16:28:05 PDT
Comment on attachment 286900 [details]
Patch

Clearing flags on attachment: 286900

Committed r204941: <http://trac.webkit.org/changeset/204941>
Comment 12 WebKit Commit Bot 2016-08-24 16:28:08 PDT
All reviewed patches have been landed.  Closing bug.