458104 - [Mac] Allocator mismatches in nsPluginFile (nsPluginsDirDarwin.cpp)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jesse Ruderman]

Attachment: valgrind output showing the stack for each allocation

Steps to reproduce:
1. Install Valgrind for Mac: http://www.sealiesoftware.com/valgrind/
2. Run an self opt build of Firefox trunk under Valgrind.  (Don't use a debug build because it won't work, and don't use a nightly because nightlies lack symbols.)
3. Load http://acid3.acidtests.org/

Result: Valgrind complains about 10 "operator delete[]" calls in nsPluginFile::FreePluginInfo, saying that the allocations were done with "malloc".

Should the allocations be changed to match the deallocations or vice versa?  I assume it's easier to change the deallocations, since the allocations happen through things like PL_strdup.

http://mxr.mozilla.org/mozilla-central/source/modules/plugin/base/src/nsPluginsDirDarwin.cpp

Comment 1

[Josh Aas]

Attachment: fix v1.0

This fixes the new/alloc/free mismatches. I also fixed cases where we were not checking allocation success. I left one case where (unlikely) error conditions could lead to leaks, but fixing that is tricky and would make this patch a lot bigger. I documented that case with a comment, otherwise this patch is a big improvement and addresses the bug described here.

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 341492 [details] [diff] [review]
fix v1.0

-    char* cstr = new char[len + 1];
-    if (cstr != NULL) {
+    char* cstr = (char*)NS_Alloc(len + 1);
+    if (cstr) {

Please use static_cast like you do in the rest of the file.

@@ -466,16 +466,18 @@ nsresult nsPluginFile::GetPluginInfo(nsP
   if (pLibrary) {
     NP_GETMIMEDESCRIPTION pfnGetMimeDesc = (NP_GETMIMEDESCRIPTION)PR_FindFunctionSymbol(pLibrary, NP_GETMIMEDESCRIPTION_NAME); 
     if (pfnGetMimeDesc)
       ParsePluginMimeDescription(pfnGetMimeDesc(), info);
     if (info.fVariantCount)
       return NS_OK;
   }
 
+  //XXXJOSH past this point, error cases will leak memory
+

I think this is an odd place for that comment - as far as I can see, the first allocations that would be leaked are more than 30 lines further down, aren't they?

Looks good to me otherwise.

Comment 3

[Josh Aas]

Attachment: fix v1.1

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 341885 [details] [diff] [review]
fix v1.1

+  //XXX FIXME: past this point some (unlikely) error cases will leak memory

so, fix it. ;-)

Comment 5

[Josh Aas]

I certainly plan to fix it but my strategy is to go for one problem at a time with separate patches (comment #1). I think that makes things more clear. If you really want me to do it in this same patch I can.

Comment 6

[Jesse Ruderman]

Comment on attachment 341885 [details] [diff] [review]
fix v1.1

I agree with Josh about "one issue per bug".  Would it help if the FIXME comment included a bug number?

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 341885 [details] [diff] [review]
fix v1.1

file a new bug regarding the unlikely mem leak.  cite it in the comment.

Comment 8

[Josh Aas]

Error-case leaks are bug 462023.

Comment 9

[Josh Aas]

landed on trunk