334687 - Possible null pointer dereference in nsPluginHostImpl.cpp

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Kenneth Herron]

Please refer to the sample URL. The |nsActivePlugin| dtor tests a member called |mPeer| for a possible NULL value at line 399, which implies the member may contain NULL. Later at line 418, |mPeer| is passed to |NS_RELEASE|, which unconditionally dereferences |mPeer|.

This was found through a coverity scan of the firefox source. I don't know if it can happen in practice, but the ctor for this class may leave |mPeer| set to nsnull.

It seems to me the fix would be to replace NS_RELEASE with NS_RELEASE_IF, or else move the NS_RELEASE call inside the block beginning at line 400.

Comment 1

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Call NS_RELEASE only if mPeer is not null

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 219479 [details] [diff] [review]
Call NS_RELEASE only if mPeer is not null

+      NS_RELEASE(mPeer);
     }
 
     // now check for cached plugins because they haven't had nsIPluginInstance::Destroy()
     // called yet. For non-cached plugins, nsIPluginInstance::Destroy() is called
     // in either nsObjectFrame::Destroy() or nsActivePluginList::stopRunning()
     PRBool doCache = PR_TRUE;
     mInstance->GetValue(nsPluginInstanceVariable_DoCacheBool, (void *) &doCache);
     if (doCache)
       mInstance->Destroy();
 
     NS_RELEASE(mInstance);
-    NS_RELEASE(mPeer);

I suspect order may matter here, and to play it safe I'd rather see the NS_RELEASE() changed to an NS_IF_RELEASE() w/o changing the release order here.

r- based on that. I'll happily r+sr a patch that doesn't change the order in which we release these objects.

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Keep release order

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 219716 [details] [diff] [review]
Keep release order

r+sr=jst

Comment 5

[:Gavin Sharp [email: [e-mail]]]

mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp 	1.554