Last Comment Bug 678978 - memory leak in nsMultiMixedConv.cpp
: memory leak in nsMultiMixedConv.cpp
Status: RESOLVED FIXED
[good first bug] [mentor=jdm][MemShri...
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla11
Assigned To: Andres Cordova
:
Mentors:
Depends on:
Blocks: cppcheck
  Show dependency treegraph
 
Reported: 2011-08-15 07:14 PDT by David Volgyes
Modified: 2011-11-10 03:17 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
nsMultiMixedConv.diff (931 bytes, patch)
2011-08-15 07:14 PDT, David Volgyes
no flags Details | Diff | Splinter Review
bug-678978.patch (5.40 KB, patch)
2011-08-20 16:14 PDT, Andres Cordova
no flags Details | Diff | Splinter Review
bug-678978.patch (updated) (5.89 KB, patch)
2011-08-21 10:58 PDT, Andres Cordova
no flags Details | Diff | Splinter Review
bug-678978.patch (added AutoFree) (6.00 KB, patch)
2011-10-15 21:05 PDT, Andres Cordova
no flags Details | Diff | Splinter Review
Patch proposal (5.69 KB, patch)
2011-10-25 18:34 PDT, Panu Horsmalahti
no flags Details | Diff | Splinter Review
Updated patch (5.61 KB, patch)
2011-10-26 04:48 PDT, Panu Horsmalahti
honzab.moz: review+
Details | Diff | Splinter Review
test (3.50 KB, patch)
2011-11-03 12:50 PDT, Honza Bambas (:mayhemer)
bzbarsky: review+
Details | Diff | Splinter Review
test v1.1 (1.76 KB, patch)
2011-11-03 13:32 PDT, Honza Bambas (:mayhemer)
honzab.moz: review+
Details | Diff | Splinter Review

Description David Volgyes 2011-08-15 07:14:49 PDT
Created attachment 553166 [details] [diff] [review]
nsMultiMixedConv.diff

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110622232440

Steps to reproduce:

cppcheck found a memory leak in
netwerk/streamconv/converters/nsMultiMixedConv.cpp
in method 'nsMultiMixedConv::OnDataAvailable' at line 476.



Actual results:

Method returns without memory release:
 if (NS_FAILED(rv) || read == 0) return rv;



Expected results:

You should release the memory before 'return':
        if (NS_FAILED(rv) || read == 0) {
           free(buffer);
           return rv;
       }

My proposed fix is attached.
(I think it's clearer than use a macro, but an ERR_OUT macro is also defined in the file, for similar purpose. With this marco the solution looks like this:
 if (NS_FAILED(rv) || read == 0) ERR_OUT
Comment 1 Ryan VanderMeulen [:RyanVM] 2011-08-15 18:27:51 PDT
jduell, this falls under your domain, yes?
Comment 2 Josh Matthews [:jdm] 2011-08-16 00:46:46 PDT
I think a much better solution here would be to make buffer an nsAutoPtr and remove all the existing uses of ERR_OUT.
Comment 3 :aceman 2011-08-16 11:02:27 PDT
According to comment 2, this bug seems confirmed.
Comment 4 Andres Cordova 2011-08-16 19:49:38 PDT
I'll try to work out a fix for this bug. Josh's solution seems pretty reasonable to me, so I'll try to implement what he proposed.
Comment 5 Andres Cordova 2011-08-20 16:14:23 PDT
Created attachment 554677 [details] [diff] [review]
bug-678978.patch
Comment 6 Andres Cordova 2011-08-20 16:16:16 PDT
Comment on attachment 554677 [details] [diff] [review]
bug-678978.patch

buffer made nsAutoPtr; uses of ERR_OUT eliminated.
Comment 7 Josh Matthews [:jdm] 2011-08-20 16:28:48 PDT
Comment on attachment 554677 [details] [diff] [review]
bug-678978.patch

Thanks for the patch Andres! I'm going to ask for a few changes before it gets an official review from a netwerk peer.

>+#include "nsAutoPtr.h"
>+

Remove this extra blank line.

>+    nsAutoPtr<char *>buffer = nsnull;

Add a space after >.

>-        if (NS_FAILED(rv)) ERR_OUT
>-
>+       

I'm sorry I wasn't clear earlier - we should get rid of ERR_OUT usages and just change them to be |return rv;| (without the || of course).

>+            rv = ParseHeaders(channel, cursor, bufLen, &done);  
...
>+                rv = SendStart(channel);               

You've added a bunch of whitespace at the end of these lines.

>     free(buffer);
>     return rv;

You'll need to get rid of the free(buffer) here, because the nsAutoPtr will do that for us and this will cause a double-free error. There's another place this should happen on line 538.

If you could make these changes and upload a new version of the patch, that would be great.
Comment 8 Andres Cordova 2011-08-20 19:41:34 PDT
Alright thanks a lot for the help! I'll have the corrections implemented this Sunday.
Comment 9 Andres Cordova 2011-08-21 10:58:15 PDT
Created attachment 554738 [details] [diff] [review]
bug-678978.patch (updated)

Updated previous patch for  bug 678978 -> Erased white spaces, additional line under include, substituted ERR_OUT message for return rv; and erased free(buffer) ocurrences
Comment 10 Josh Matthews [:jdm] 2011-08-21 11:08:55 PDT
This is great, Andres, thank you. However, I just remembered that nsAutoPtr uses delete to free the memory, while the buffer is being allocated with malloc/realloc. You're going to need to create an AutoFree class like so:

class AutoFree
{
public:
  AutoFree(char *buffer);
  ~AutoFree();
  AutoFree& operator=(char *buffer);
  operator char*() const;
private:
  char *mBuffer;
};

Sorry to keep asking for changes.
Comment 11 Andres Cordova 2011-08-22 13:30:22 PDT
Oh no don't be sorry, actually I feel I should have noticed this! haha, sorry if I was a little careless, I'm in exams week (doing summer course) so things have been a little hectic. Alright, I'll implement what you asked, though I have a couple of questions I'll send tonight on an email. I expect to have it ready by Wednesday 24th the latest, but I'll work on it today and tomorrow.
Comment 12 Josh Matthews [:jdm] 2011-10-15 12:11:34 PDT
Andres, any update here?
Comment 13 Andres Cordova 2011-10-15 18:07:27 PDT
(In reply to Josh Matthews [:jdm] from comment #12)
> Andres, any update here?

Oooh!...yes yes, I'll post it in a couple of hours, I apologize.
Comment 14 Andres Cordova 2011-10-15 21:05:50 PDT
Created attachment 567317 [details] [diff] [review]
bug-678978.patch (added AutoFree)
Comment 15 Andres Cordova 2011-10-15 21:06:34 PDT
Added the AutoFree class like you suggested.
Comment 16 Josh Matthews [:jdm] 2011-10-15 21:18:56 PDT
Hmm, there's no actual implementation for the members of AutoFree, and you don't actually use it in the patch.
Comment 17 Andres Cordova 2011-10-15 21:28:29 PDT
Oh, I guess I misunderstood you. I'll try again.
Comment 18 Panu Horsmalahti 2011-10-25 18:34:21 PDT
Created attachment 569576 [details] [diff] [review]
Patch proposal
Comment 19 Josh Matthews [:jdm] 2011-10-26 00:16:36 PDT
Comment on attachment 569576 [details] [diff] [review]
Patch proposal

Review of attachment 569576 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch Panu, this is great work! With a couple small changes, this will be ready for a networking module peer to review :)

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ -466,1 +466,5 @@
> > -#define ERR_OUT { free(buffer); return rv; }
> > +// AutoFree implementation to prevent memory leaks
> > +class AutoFree
> > +{
> > +public:
> > +  AutoFree() : mBuffer(NULL) {

Just make the constructor bodies {}, there's no need for the extra whitespace.

@@ -466,1 +466,18 @@
> > -#define ERR_OUT { free(buffer); return rv; }
> > +// AutoFree implementation to prevent memory leaks
> > +class AutoFree
> > +{
> > +public:
NaN more ...

This would ordinarily be a good check to include, but we call realloc on the buffer that mBuffer points to. Since realloc frees the original pointer, we would be trying to free the buffer again here, so we can just get rid of this whole check.
Comment 20 Panu Horsmalahti 2011-10-26 04:48:37 PDT
Created attachment 569642 [details] [diff] [review]
Updated patch

Implemented requested changes.
Comment 21 Honza Bambas (:mayhemer) 2011-11-03 12:46:37 PDT
Comment on attachment 569642 [details] [diff] [review]
Updated patch

Review of attachment 569642 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab but needs a test that ensures we cover all parts of this code (namely the realloc part).  Added shortly in another patch.

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ +466,5 @@
> +// AutoFree implementation to prevent memory leaks
> +class AutoFree
> +{
> +public:
> +  AutoFree() : mBuffer(NULL) {}

Rather use nsnull.

Having some form of class like this side by nsAutoPtr would be nice, but not necessarily in this bug.
Comment 22 Honza Bambas (:mayhemer) 2011-11-03 12:50:21 PDT
Created attachment 571748 [details] [diff] [review]
test

This is a copy of test_multipart_streamconv.js.  This copy is missing the leading boundary in the response body to trigger a part of code that invokes realloc to check the new AutoFree helper class works correctly even in this case.

I did a copy instead of modifying the current test since it is much less work to do, but if you wish I can update the current test instead.
Comment 23 Boris Zbarsky [:bz] (TPAC) 2011-11-03 13:03:52 PDT
Comment on attachment 571748 [details] [diff] [review]
test

s/boundery/boundary/ in the test name.

r=me with that
Comment 24 Boris Zbarsky [:bz] (TPAC) 2011-11-03 13:04:44 PDT
Oh, actually, one more thing.  If you really did a copy and slight modify, it's better to use hg copy and then modify.  That will clearly show where the code came from and what the modification was.  Please make sure to do that before checking in.
Comment 25 Honza Bambas (:mayhemer) 2011-11-03 13:32:17 PDT
Created attachment 571758 [details] [diff] [review]
test v1.1

Thanks, Boris.
Comment 27 Marco Bonardo [::mak] 2011-11-10 03:17:38 PST
https://hg.mozilla.org/mozilla-central/rev/4ea87da3d5af

Note You need to log in before you can comment on or make changes to this bug.