Bug 160808 – FocusController multiple dereferenced NULL pointers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 285921 [details]
Patch

[Jonathan Bedard]

Created attachment 285925 [details]
Patch

[Daniel Bates]

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?

[Jonathan Bedard]

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.

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 285945 [details]
Patch

[Jonathan Bedard]

rdar://16327307 is also tracking this bug.

[Alexey Proskuryakov]

rdar://problem/16327307

[WebKit Commit Bot]

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

[Jonathan Bedard]

Created attachment 286900 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 286900 [details]
Patch

Clearing flags on attachment: 286900

Committed r204941: <http://trac.webkit.org/changeset/204941>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.