Bug 160808 - FocusController multiple dereferenced NULL pointers
Summary: FocusController multiple dereferenced NULL pointers
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Major
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2016-08-12 10:49 PDT by Jonathan Bedard
Modified: 2016-08-24 16:28 PDT (History)
1 user (show)

See Also:

Patch (15.03 KB, patch)
2016-08-12 11:07 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (15.30 KB, patch)
2016-08-12 12:00 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (11.47 KB, patch)
2016-08-12 14:07 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (11.54 KB, patch)
2016-08-24 15:45 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Comment 2 Jonathan Bedard 2016-08-12 12:00:50 PDT
Created attachment 285925 [details]
Comment 3 Daniel Bates 2016-08-12 12:50:48 PDT
Comment on attachment 285925 [details]

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]
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
Comment 9 WebKit Commit Bot 2016-08-24 15:35:05 PDT
Comment on attachment 285945 [details]

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]
Comment 11 WebKit Commit Bot 2016-08-24 16:28:05 PDT
Comment on attachment 286900 [details]

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.