Prevent the WebPluginContainer being destroyed inside scriptableObject()

Prevent the WebPluginContainer being destroyed inside scriptableObject()

Current re-entrancy inside WebPluginContainer::scriptableObject can cause
the plugin to be deleted, as well as the WebPluginContainer. This can cause
UAFs. This change holds a reference to the plugin container to prevent it from
being destroyed while in the function. This also prevents the WebPlugin
associated with it from being destroyed since the lifetime of WebPlugin is
managed by the WebPluginContainer.

BUG=458776
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190700

[raymes, 2015-02-16 22:17:55]

raymes@chromium.org changed reviewers:
+ dmichael@chromium.org, jochen@chromium.org

[dmichael (off chromium), 2015-02-17 20:54:20]

https://codereview.chromium.org/933653004/diff/1/Source/web/WebPluginContaine...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/933653004/diff/1/Source/web/WebPluginContaine...
Source/web/WebPluginContainerImpl.cpp:601:
RefPtrWillBeRawPtr<WebPluginContainerImpl> protector(this);
Does this guarantee that dispose() won't be called? Or does it only prevent
destruction? I see there's a 2-stage destruction; dispose sets it to NULL...  if
dispose() may be called but this object is still alive, we could dereference
NULL below...  but I don't know blink stuff well enough.

Also...  might this make us do scripting now in cases where we wouldn't before,
because we will not have been deleted no matter what? Would it make sense to do:
if (hasOneRef())
    return v8::Local<v8::Object>();
...after the call to v8ScriptableObject?

[raymes, 2015-02-17 22:26:01]

https://codereview.chromium.org/933653004/diff/1/Source/web/WebPluginContaine...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/933653004/diff/1/Source/web/WebPluginContaine...
Source/web/WebPluginContainerImpl.cpp:601:
RefPtrWillBeRawPtr<WebPluginContainerImpl> protector(this);
On 2015/02/17 20:54:19, dmichael wrote:
> Does this guarantee that dispose() won't be called? Or does it only prevent
> destruction? I see there's a 2-stage destruction; dispose sets it to NULL... 
if
> dispose() may be called but this object is still alive, we could dereference
> NULL below...  but I don't know blink stuff well enough.
> 

Hmm, good point. I don't know for certain. I do see a few things:
1) When OILPAN isn't defined, we ALWAYS call dispose() in the destructor.
dispose() assumes that m_webPlugin is not NULL because it called destroy on it.
So assuming OILPAN isn't defined, I assume it's ok.
2) WebPluginContainerImpl::handleEvent uses a similar pattern of hold a
reference to the WebPluginContainerImpl on the stack because it makes sync IPCs.
If you look at the functions that it calls into (e.g. handleMouseEvent) they
also assume that m_webPlugin is valid without any checks.

So based on these 2 observations I think it is quite safe.

> Also...  might this make us do scripting now in cases where we wouldn't
before,
> because we will not have been deleted no matter what? Would it make sense to
do:
> if (hasOneRef())
>     return v8::Local<v8::Object>();
> ...after the call to v8ScriptableObject?

That's a good point. I added your suggestion. PTAL :)

[dmichael (off chromium), 2015-02-18 22:36:05]

lgtm

[raymes, 2015-02-18 22:37:17]

jochen@ please review for OWNERS when you can. Thanks!

[raymes, 2015-02-23 02:30:04]

ping :)

[jochen (gone - plz use gerrit), 2015-02-23 12:16:19]

lgtm

[raymes, 2015-02-23 21:56:22]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2015-02-23 21:57:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/933653004/20001

[commit-bot: I haz the power, 2015-02-24 00:26:06]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190700

[sof, 2015-02-24 06:19:29]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-02-24 06:19:29]

https://codereview.chromium.org/933653004/diff/20001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/933653004/diff/20001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.cpp:607: if (hasOneRef())
Would ASSERT(!m_webPlugin) hold if there's one ref left?

[sof, 2015-02-24 06:32:11]

https://codereview.chromium.org/933653004/diff/20001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/933653004/diff/20001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.cpp:599: // v8ScriptableObject below.
crbug.com/458776. Hold a reference to the
When appropriate, could access to bug 458776 please be opened up a bit? Ditto
for 423263 -- unless there are good reasons not to, inaccessible-to-contributors
bug references in source code adds questionable value.