Bug 54304 – Check for empty scheme before accessing URLSchemeMaps in SchemeRegistry

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Klein]

Check for empty scheme before accessing URLSchemeMaps in SchemeRegistry

[Adam Klein]

Created attachment 82159 [details]
Patch

[Darin Adler]

Comment on attachment 82159 [details]
Patch

This change is correct, but can you please construct a test case that shows the problem? Normally we require regression tests for all bug fixes.

[WebKit Commit Bot]

Comment on attachment 82159 [details]
Patch

Rejecting attachment 82159 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'land-a..." exit_code: 2

Last 500 characters of output:
ebKit
   2d3c60d..e45ab5e  master     -> origin/master
	M	LayoutTests/platform/chromium/test_expectations.txt
	M	LayoutTests/ChangeLog
	M	Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp
	M	Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp
	M	Tools/ChangeLog
	M	Tools/Scripts/webkitpy/layout_tests/port/chromium_win.py
r78359 = e45ab5efb940172a265712fe69fb923eef21d2df (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/7869732

[Adam Klein]

Thanks for the quick review. It's not currently possible to exercise this code with a layout test.  For shouldTreatURLSchemeAsSecure, the only caller is (in FrameLoader.cpp):

  if (!url.isValid || SchemeRegistry::shouldTreatURLSchemeAsSecure(url.protocol())

so the case is never hit.  The situation is similar for shouldLoadURLSchemeAsSecure in MainResourceLoader.cpp:

  return url.isEmpty() || SchemeRegistry::shouldLoadURLSchemeAsEmptyDocument(url.protocol());

Finally, shouldTreatURLSchemeAsNoAccess has no callers at all.

Nevertheless, I think this patch is a good idea, to avoid new, less careful callers causing crashes.  I just ran into this problem when trying to get https://bugs.webkit.org/show_bug.cgi?id=53529 landed (SecurityOrigin.cpp calls these methods without pre-validating the scheme).  And it appears to me that Adam Barth recently resolved the same issue for another method in http://trac.webkit.org/changeset/76637.

[Eric Seidel (no email)]

Comment on attachment 82159 [details]
Patch

I think the cq might be sick.  It did that to me yesterday as well.

[WebKit Commit Bot]

Comment on attachment 82159 [details]
Patch

Clearing flags on attachment: 82159

Committed r78367: <http://trac.webkit.org/changeset/78367>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.