The default bug view has changed. See this FAQ.

memory leak in nsMultiMixedConv.cpp

RESOLVED FIXED in mozilla11

Status

()

Core
Networking
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: David Volgyes, Assigned: Andres Cordova)

Tracking

(Blocks: 1 bug)

Trunk
mozilla11
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

6 years ago
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
jduell, this falls under your domain, yes?

Updated

6 years ago
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking

Comment 2

6 years ago
I think a much better solution here would be to make buffer an nsAutoPtr and remove all the existing uses of ERR_OUT.

Updated

6 years ago
Whiteboard: [good first bug] [mentor=jdm]

Updated

6 years ago
Blocks: 679417

Comment 3

6 years ago
According to comment 2, this bug seems confirmed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
Created attachment 554677 [details] [diff] [review]
bug-678978.patch
(Assignee)

Comment 6

6 years ago
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 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
Alright thanks a lot for the help! I'll have the corrections implemented this Sunday.
(Assignee)

Comment 9

6 years ago
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

Updated

6 years ago
Attachment #554677 - Attachment is obsolete: true

Updated

6 years ago
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.
(Assignee)

Comment 11

6 years ago
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++]
(Assignee)

Comment 13

6 years ago
(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.
(Assignee)

Comment 14

6 years ago
Created attachment 567317 [details] [diff] [review]
bug-678978.patch (added AutoFree)
(Assignee)

Comment 15

6 years ago
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.
(Assignee)

Comment 17

6 years ago
Oh, I guess I misunderstood you. I'll try again.

Comment 18

6 years ago
Created attachment 569576 [details] [diff] [review]
Patch proposal
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)

Updated

6 years ago
Attachment #567317 - Attachment is obsolete: true

Updated

6 years ago
Attachment #554738 - Attachment is obsolete: true

Comment 20

6 years ago
Created attachment 569642 [details] [diff] [review]
Updated patch

Implemented requested changes.
Attachment #569642 - Flags: review?(josh)

Updated

6 years ago
Attachment #569642 - Flags: review?(josh) → review?(honzab.moz)

Updated

6 years ago
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+
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.
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.
Created attachment 571758 [details] [diff] [review]
test v1.1

Thanks, Boris.
Attachment #571758 - Flags: review+
Attachment #571748 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ea87da3d5af
https://hg.mozilla.org/mozilla-central/rev/4ea87da3d5af
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.