303794 - potential OOM crash [@ nsActivePlugin::~nsActivePlugin] [@ nsPluginTag::nsPluginTag] [@ nsPluginHostImpl::GetPluginFactory] [@ nsPluginHostImpl::ScanPluginsDirectory] [@ nsPluginHostImpl::ReadPluginInfo]

Status: RESOLVED WONTFIX

(Core Graveyard :: Plug-ins, defect)

Description

[timeless]

this is an audit, in some cases i've only marked the allocations and not the
dereferences. in a couple of cases i'm flagging returning NS_OK for an OOM case,
i'm not sure if that's proper, i know dom doesn't like throwing exceptions, but
the world isn't going to be happy when you run out of memory, so i think dying
sooner is probably worth considering

Comment 1

[Bastiaan Jacques]

Taking.

Comment 2

[Bastiaan Jacques]

Attachment: rev. 1 (untested)

With this patch, I've:

* Added three nsPluginTag::Init() methods and moved code from constructors into
those methods, so OOM errors can be signaled. The default constructor is now
always used, which allowed me to remove some duplicated initialization code.
* Switched classes with solely public members and methods to structs.
* Changed some raw-AddRefed, -Released, allocated and deleted pointers over to
nsCOMPtrs, nsRefPtrs and nsAutoPtrs, where appropriate.
* Added OOM checks throughout the code, particularly in the areas marked in
timeless's URL.
* Slightly optimized the inlined function new_str() with NS_[UN]LIKELY.
* Since all the functions in which nsActivePlugin::mPeer is used assume that it
is of type nsPluginInstancePeerImpl, I've changed it to that. This also allows
us to avoid some ugly casting.

Comment 3

[Bastiaan Jacques]

Attachment: rev. 2

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

> * Switched classes with solely public members and methods to structs.

why?

Comment 5

[Bastiaan Jacques]

Because I was taught to only use classes when I'm using features that only
classes provide :)

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

there are no features that only classes provide...

Comment 7

[Bastiaan Jacques]

biesi taught me the finer points of C++ structs/classes on IRC and it turns out
all I'm doing is saving us one line of code.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 192585 [details] [diff] [review]
rev. 2

> nsActivePlugin::nsActivePlugin(nsPluginTag* aPluginTag,
>                                nsIPluginInstance* aInstance,
>                                const char * url,
>                                PRBool aDefaultPlugin,
>-                               nsIPluginInstancePeer* peer)
>+                               nsPluginInstancePeerImpl* peer)
>+ : mPeer(peer),
>+   mInstance(aInstance)
> {
>   mNext = nsnull;
>-  mPeer = nsnull;
>   mPluginTag = aPluginTag;
> 
>   mURL = PL_strdup(url);
>-  mInstance = aInstance;
>-  if(aInstance && peer)
>-  {
>-    mPeer = peer;
>-    NS_ADDREF(mPeer);
>-    NS_ADDREF(aInstance);
>-  }
>   mXPConnected = PR_FALSE;
>   mDefaultPlugin = aDefaultPlugin;
>   mStopped = PR_FALSE;
>   mllStopTime = LL_ZERO;
> }

I know it's existing code, but since you are deCOMtaminating and using member
initializers for mPeer and mInstance, why not fix up the rest to match?  1.9
alpha-stage trunk is open, no time to minimize change for this sort of cleanup.

>       nsresult rv = NS_OK;
>-      nsCOMPtr<nsPIPluginInstancePeer> peer(do_QueryInterface(mPeer));
>       nsCOMPtr<nsIPluginInstanceOwner> owner;
>-      rv = peer->GetOwner(getter_AddRefs(owner));
>-      if (owner)
>+      rv = mPeer->GetOwner(getter_AddRefs(owner));

1.  Don't initialize rv if it's set right away two lines later.
2.  Consider avoiding rv altogether by just null-testing owner as the previous
code did.  There's no risk of non-null out param thanks to getter_AddRefs, and
there is no other need for rv.

>+      if (NS_SUCCEEDED(rv))
>         owner->SetInstance(nsnull);
>     }

Testing owner non-null instead of messing with rv would match this hunk, too:

>       // If we've been passed an array to return, lets collect all our documents,
>       // removing duplicates. These will be reframed (embedded) or reloaded (full-page) later
>       // to kickstart our instances.
>       if (aReloadDocs && p->mPeer) {
>-        nsCOMPtr<nsPIPluginInstancePeer> peer(do_QueryInterface(p->mPeer));
>         nsCOMPtr<nsIPluginInstanceOwner> owner;
>-        peer->GetOwner(getter_AddRefs(owner));
>+        p->mPeer->GetOwner(getter_AddRefs(owner));
>         if (owner) {
>           nsCOMPtr<nsIDocument> doc;
>           owner->GetDocument(getter_AddRefs(doc));
>           if (doc && aReloadDocs->IndexOf(doc) == -1)  // don't allow for duplicates
>             aReloadDocs->AppendElement(doc);
>         }
>       }
>     }

[. . .]

>+// NOTE: new_str() requires a null-terminated string.
> inline char* new_str(const char* str)
> {
>-  if(str == nsnull)
>+  if(NS_UNLIKELY(str == nsnull)) 
>     return nsnull;
> 
>   char* result = new char[strlen(str) + 1];
>-  if (result != nsnull)
>+  if (NS_LIKELY(result != nsnull))
>     return strcpy(result, str);

If you used the default-not-taken sense of the condition here (result ==
nsnull) as in the previous if, you wouldn't need NS_LIKELY -- that would be
prettier, and it would make the case that compilers should (and do!) select
branch-likely ops around then clauses (and make loop closing branches likely,
etc.).

It's prettier not only because of symmetry but because the normal-case control
flow then stays least indented throughout this little helper function, instead
of suddenly veering rightward one indentation level and returning early.

>   return result;
> }
> 
> 
> ////////////////////////////////////////////////////////////////////////
>-nsPluginTag::nsPluginTag(nsPluginTag* aPluginTag)
>+
>+nsresult
>+nsPluginTag::Init(nsPluginTag* aPluginTag)
> {
>-  mPluginHost = nsnull;
>-  mNext = nsnull;
>   mName = new_str(aPluginTag->mName);
>+  NS_ENSURE_TRUE(mName, NS_ERROR_OUT_OF_MEMORY);
>+
>   mDescription = new_str(aPluginTag->mDescription);
>-  mVariants = aPluginTag->mVariants;
>+  NS_ENSURE_TRUE(mDescription, NS_ERROR_OUT_OF_MEMORY);
> 
>-  mMimeTypeArray = nsnull;
>-  mMimeDescriptionArray = nsnull;
>-  mExtensionsArray = nsnull;
>+  mVariants = aPluginTag->mVariants;
> 
>-  if(aPluginTag->mMimeTypeArray != nsnull)
>-  {
>+  if(aPluginTag->mMimeTypeArray) {
>     mMimeTypeArray = new char*[mVariants];
>-    for (int i = 0; i < mVariants; i++)
>+    NS_ENSURE_TRUE(mMimeTypeArray, NS_ERROR_OUT_OF_MEMORY);
>+    for (int i = 0; i < mVariants; i++) {
>       mMimeTypeArray[i] = new_str(aPluginTag->mMimeTypeArray[i]);
>+      NS_ENSURE_TRUE(mMimeTypeArray[i], NS_ERROR_OUT_OF_MEMORY);
>+    }
>   }

Who cleans up in all these hidden early return cases buried in the
NS_ENSURE_TRUE calls?  That is, mMimeTypeArray has been new'ed successfully,
but is not zero'ed, and the loop stops at i = 1 where mVariants is 3 because
the new_str in the for loop failed.  Who frees the first element, at i=0, and
how does that code know not to march into uninitialized slots at i >= 1?

One fix would be to set mVariants to 0 before the loop, then to i+1 in the loop
body as you get past each NS_ENSURE_TRUE. Or, set it to i in the loop before
the new_str call, then to i after the loop (the loop condition should prolly
use a local numVariants = aPluginTag->mVariants).

> 
>-  if(aPluginTag->mMimeDescriptionArray != nsnull)
>-  {
>+  if(aPluginTag->mMimeDescriptionArray) {
>     mMimeDescriptionArray = new char*[mVariants];
>-    for (int i = 0; i < mVariants; i++)
>+    NS_ENSURE_TRUE(mMimeDescriptionArray, NS_ERROR_OUT_OF_MEMORY);
>+
>+    for (int i = 0; i < mVariants; i++) {
>       mMimeDescriptionArray[i] = new_str(aPluginTag->mMimeDescriptionArray[i]);
>+      NS_ENSURE_TRUE(mMimeDescriptionArray[i], NS_ERROR_OUT_OF_MEMORY);
>+    }
>   }

More of the same here.

>-  if(aPluginTag->mExtensionsArray != nsnull)
>-  {
>+  if(aPluginTag->mExtensionsArray) {
>     mExtensionsArray = new char*[mVariants];
>-    for (int i = 0; i < mVariants; i++)
>+    NS_ENSURE_TRUE(mExtensionsArray, NS_ERROR_OUT_OF_MEMORY);
>+    for (int i = 0; i < mVariants; i++) {
>       mExtensionsArray[i] = new_str(aPluginTag->mExtensionsArray[i]);
>+      NS_ENSURE_TRUE(mExtensionsArray[i], NS_ERROR_OUT_OF_MEMORY);
>+    }
>   }

And here.

> 
>-  mLibrary = nsnull;
>-  mCanUnloadLibrary = PR_TRUE;
>-  mEntryPoint = nsnull;
>-  mFlags = NS_PLUGIN_FLAG_ENABLED;
>-  mXPConnected = PR_FALSE;
>   mFileName = new_str(aPluginTag->mFileName);
>+  NS_ENSURE_TRUE(mFileName, NS_ERROR_OUT_OF_MEMORY);

Another hidden early return here, but that leaves mFullPath default initialized
to null, I hope.

>   mFullPath = new_str(aPluginTag->mFullPath);
>-}
>+  NS_ENSURE_TRUE(!aPluginTag->mFullPath || mFullPath, NS_ERROR_OUT_OF_MEMORY);
> 
>+  return NS_OK;
>+}
> 
>-////////////////////////////////////////////////////////////////////////
>-nsPluginTag::nsPluginTag(nsPluginInfo* aPluginInfo)
>+nsresult
>+nsPluginTag::Init(nsPluginInfo* aPluginInfo)
> {
>-  mPluginHost = nsnull;
>-  mNext = nsnull;
>   mName = new_str(aPluginInfo->fName);
>+  NS_ENSURE_TRUE(mName, NS_ERROR_OUT_OF_MEMORY);
>+
>   mDescription = new_str(aPluginInfo->fDescription);
>-  mVariants = aPluginInfo->fVariantCount;
>+  NS_ENSURE_TRUE(mDescription, NS_ERROR_OUT_OF_MEMORY);
> 
>-  mMimeTypeArray = nsnull;
>-  mMimeDescriptionArray = nsnull;
>-  mExtensionsArray = nsnull;
>+  mVariants = aPluginInfo->fVariantCount;
> 
>-  if(aPluginInfo->fMimeTypeArray != nsnull)
>-  {
>+  if(aPluginInfo->fMimeTypeArray) {
>     mMimeTypeArray = new char*[mVariants];
>-    for (int i = 0; i < mVariants; i++)
>+    NS_ENSURE_TRUE(mMimeTypeArray, NS_ERROR_OUT_OF_MEMORY);
>+
>+    for (int i = 0; i < mVariants; i++) {
>       mMimeTypeArray[i] = new_str(aPluginInfo->fMimeTypeArray[i]);
>+      NS_ENSURE_TRUE(mMimeTypeArray[i], NS_ERROR_OUT_OF_MEMORY);
>+    }
>   }

Awfully similar code here -- factor common stuff out where possible and
worthwhile.

I stopped here, perhaps a new patch and a clean sweep for the patterns cited
above would be worth doing before anyone else reviews.	I can sr, but jst
should review.

/be

Comment 9

[Bastiaan Jacques]

The patch has bitrotted extensively, but more importantly, most or all of the issues it addressed have been taken care of in other bugs. With infallible malloc, this bug should now be a non-issue anyway.