Closed Bug 678978 Opened 8 years ago Closed 8 years ago

memory leak in nsMultiMixedConv.cpp

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: david.volgyes, Assigned: andrescordova87)

References

Details

(Whiteboard: [good first bug] [mentor=jdm][MemShrink:P3][lang=c++])

Attachments

(2 files, 6 obsolete files)

Attached patch nsMultiMixedConv.diff (obsolete) — Splinter Review
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
jduell, this falls under your domain, yes?
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking
I think a much better solution here would be to make buffer an nsAutoPtr and remove all the existing uses of ERR_OUT.
Whiteboard: [good first bug] [mentor=jdm]
Blocks: cppcheck
According to comment 2, this bug seems confirmed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attached patch bug-678978.patch (obsolete) — Splinter Review
Comment on attachment 554677 [details] [diff] [review]
bug-678978.patch

buffer made nsAutoPtr; uses of ERR_OUT eliminated.
Attachment #554677 - Attachment description: buffer made nsAutoPtr; uses of ERR_OUT eliminated. → bug-678978.patch
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.
Alright thanks a lot for the help! I'll have the corrections implemented this Sunday.
Attached patch bug-678978.patch (updated) (obsolete) — Splinter Review
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
Attachment #554677 - Attachment is obsolete: true
Attachment #553166 - Attachment is obsolete: true
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.
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.
Whiteboard: [good first bug] [mentor=jdm] → [good first bug] [mentor=jdm][MemShrink]
Assignee: nobody → andrescordova87
Whiteboard: [good first bug] [mentor=jdm][MemShrink] → [good first bug] [mentor=jdm][MemShrink:P3]
Andres, any update here?
Whiteboard: [good first bug] [mentor=jdm][MemShrink:P3] → [good first bug] [mentor=jdm][MemShrink:P3][lang=c++]
(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.
Added the AutoFree class like you suggested.
Hmm, there's no actual implementation for the members of AutoFree, and you don't actually use it in the patch.
Oh, I guess I misunderstood you. I'll try again.
Attached patch Patch proposal (obsolete) — Splinter Review
Attachment #569576 - Flags: review?(josh)
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.
Attachment #569576 - Flags: review?(josh)
Attachment #567317 - Attachment is obsolete: true
Attachment #554738 - Attachment is obsolete: true
Attached patch Updated patchSplinter Review
Implemented requested changes.
Attachment #569642 - Flags: review?(josh)
Attachment #569642 - Flags: review?(josh) → review?(honzab.moz)
Attachment #569576 - Attachment is obsolete: true
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.
Attachment #569642 - Flags: review?(honzab.moz) → review+
Attached patch test (obsolete) — Splinter Review
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.
Attachment #571748 - Flags: review?(bzbarsky)
Comment on attachment 571748 [details] [diff] [review]
test

s/boundery/boundary/ in the test name.

r=me with that
Attachment #571748 - Flags: review?(bzbarsky) → review+
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.
Attached patch test v1.1Splinter Review
Thanks, Boris.
Attachment #571758 - Flags: review+
Attachment #571748 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4ea87da3d5af
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.