Bug 55704

Summary: QuickTime plugin should opt in to a 32-bit non-executable heap
Product: WebKit Reporter: Damian Kaleta <dkaleta>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: andersca, ayao, commit-queue, dkaleta, ike, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Macintosh Intel   
OS: OS X 10.6   
Attachments:
Description Flags
Patch
andersca: review-
Proposed patch.
none
Proposed patch. andersca: review+, commit-queue: commit-queue-

Description Damian Kaleta 2011-03-03 13:47:48 PST
Not all plugins should be opt out of 32-bit NX.
Comment 1 Damian Kaleta 2011-03-03 13:54:33 PST
Created attachment 84619 [details]
Patch
Comment 2 Anders Carlsson 2011-03-03 13:59:36 PST
Comment on attachment 84619 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84619&action=review

> WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:222
> +bool PluginProcessProxy::pluginNeedsExecutableHeap(const PluginInfoStore::Plugin& pluginInfo) const

This doesn't need to be a member function of PluginProcessProxy, it can just be a static free function (and I'd put it above the call site.
Comment 3 Simon Fraser (smfr) 2011-03-03 14:03:42 PST
Bug title makes no sense. What is NX?
Comment 4 Damian Kaleta 2011-03-03 14:08:32 PST
Simon, 
Take a look at this: <rdar://problem/8105706>
and that: http://en.wikipedia.org/wiki/NX_bit
Comment 5 Simon Fraser (smfr) 2011-03-03 14:11:55 PST
You should spell out "non-executable", and clarify what 32-bit NX is. Does this only apply to 32-bit plug-in, or does it mean something else? And is the plugin opting out, or is the browser forcing this on the plug-in?
Comment 6 Damian Kaleta 2011-03-03 15:18:25 PST
Created attachment 84638 [details]
Proposed patch.
Comment 7 Ivan Krstić 2011-03-03 18:08:20 PST
This patch is confused. The ChangeLog states that Silverlight and QuickTime should opt *in* to a 32-bit NX heap, which means their heap should not be executable. But then it opts them *out* by making pluginNeedsExecutableHeap() claim that those two plugins need an executable heap — i.e. that they cannot run with NX. It also opts all other plugins into running with an NX heap.

Assuming you're really trying to say "QuickTime and Silverlight should run with a non-executable heap (opt-in), and other plugins should have an executable one (opt-out)", you need to invert your true/false returns in pluginNeedsExecutableHeap.
Comment 8 Damian Kaleta 2011-03-04 09:41:00 PST
I misunderstood this bug, let me clarify the ChangeLog and make changes to the patch.

(In reply to comment #7)
> This patch is confused. The ChangeLog states that Silverlight and QuickTime should opt *in* to a 32-bit NX heap, which means their heap should not be executable. But then it opts them *out* by making pluginNeedsExecutableHeap() claim that those two plugins need an executable heap — i.e. that they cannot run with NX. It also opts all other plugins into running with an NX heap.
> 
> Assuming you're really trying to say "QuickTime and Silverlight should run with a non-executable heap (opt-in), and other plugins should have an executable one (opt-out)", you need to invert your true/false returns in pluginNeedsExecutableHeap.
Comment 9 Damian Kaleta 2011-03-04 09:53:07 PST
Created attachment 84766 [details]
Proposed patch.
Comment 10 Ivan Krstić 2011-03-04 09:59:42 PST
Looks good to me.
Comment 11 WebKit Commit Bot 2011-03-04 18:14:29 PST
Comment on attachment 84766 [details]
Proposed patch.

Rejecting attachment 84766 [details] from commit-queue.

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

Last 500 characters of output:
.......................................................................................................................................................................................................................
inspector ...........
inspector/audits .
inspector/audits/audits-panel-functional.html -> failed

Exiting early after 1 failures. 12761 tests run.
301.30s total testing time

12760 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
8 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8082846
Comment 12 Anders Carlsson 2011-03-07 18:33:02 PST
Committed r80523: <http://trac.webkit.org/changeset/80523>