Closed Bug 458104 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Plug-ins, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: jaas)

References

Details

(Keywords: valgrind)

Attachments

(2 files, 1 obsolete file)

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
Assignee: nobody → joshmoz
Attached patch fix v1.0 (obsolete) — Splinter Review
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.
Attachment #341492 - Flags: review?(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.
Attachment #341492 - Flags: review?(mstange) → review+
Attached patch fix v1.1Splinter Review
Attachment #341492 - Attachment is obsolete: true
Attachment #341885 - Flags: superreview?(jst)
Attachment #341885 - Flags: superreview?(jst) → superreview?(doug.turner)
Attachment #341885 - Flags: superreview?(doug.turner) → superreview-
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. ;-)
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 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?
Attachment #341885 - Flags: superreview- → superreview?(doug.turner)
Comment on attachment 341885 [details] [diff] [review]
fix v1.1

file a new bug regarding the unlikely mem leak.  cite it in the comment.
Attachment #341885 - Flags: superreview?(doug.turner) → superreview+
Error-case leaks are bug 462023.
landed on trunk
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.