678978 - memory leak in nsMultiMixedConv.cpp

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[David Volgyes]

Attachment: 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]]

jduell, this falls under your domain, yes?

Comment 2

[Josh Matthews [:jdm]]

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]

According to comment 2, this bug seems confirmed.

Comment 4

[Andres Cordova]

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]

Attachment: bug-678978.patch

Comment 6

[Andres Cordova]

Comment on attachment 554677 [details] [diff] [review]
bug-678978.patch

buffer made nsAutoPtr; uses of ERR_OUT eliminated.

Comment 7

[Josh Matthews [:jdm]]

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]

Alright thanks a lot for the help! I'll have the corrections implemented this Sunday.

Comment 9

[Andres Cordova]

Attachment: 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]]

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]

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]]

Andres, any update here?

Comment 13

[Andres Cordova]

(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]

Attachment: bug-678978.patch (added AutoFree)

Comment 15

[Andres Cordova]

Added the AutoFree class like you suggested.

Comment 16

[Josh Matthews [:jdm]]

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]

Oh, I guess I misunderstood you. I'll try again.

Comment 18

[Panu Horsmalahti]

Attachment: Patch proposal

Comment 19

[Josh Matthews [:jdm]]

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]

Attachment: Updated patch

Implemented requested changes.

Comment 21

[Honza Bambas (:mayhemer)]

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)]

Attachment: 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 [:bzbarsky]]

Comment on attachment 571748 [details] [diff] [review]
test

s/boundery/boundary/ in the test name.

r=me with that

Comment 24

[Boris Zbarsky [:bzbarsky]]

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)]

Attachment: test v1.1

Thanks, Boris.

Comment 26

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ea87da3d5af

Comment 27

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/4ea87da3d5af