Bug 55704 – QuickTime plugin should opt in to a 32-bit non-executable heap

• Component: WebKit2
• Status: RESOLVED FIXED

[Damian Kaleta]

Not all plugins should be opt out of 32-bit NX.

[Damian Kaleta]

Created attachment 84619 [details]
Patch

[Anders Carlsson]

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.

[Simon Fraser (smfr)]

Bug title makes no sense. What is NX?

[Damian Kaleta]

Simon, 
Take a look at this: <rdar://problem/8105706>
and that: http://en.wikipedia.org/wiki/NX_bit

[Simon Fraser (smfr)]

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?

[Damian Kaleta]

Created attachment 84638 [details]
Proposed patch.

[Ivan Krstić]

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.

[Damian Kaleta]

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.

[Damian Kaleta]

Created attachment 84766 [details]
Proposed patch.

[Ivan Krstić]

Looks good to me.

[WebKit Commit Bot]

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

[Anders Carlsson]

Committed r80523: <http://trac.webkit.org/changeset/80523>