Closed Bug 676187 Opened 8 years ago Closed 8 years ago

memory leak in toolkit/mozapps/readstrings/readstrings.cpp

Categories

(Toolkit :: Application Update, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: david.volgyes, Assigned: hugo.t.r)

References

Details

(Keywords: memory-leak, Whiteboard: [good first bug] [mentor=jdm][MemShrink:P3][lang=c++][fixed-in-fx-team])

Attachments

(1 file, 2 obsolete files)

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

Steps to reproduce:

I run cppcheck 1.49 and found several leaks.
This is one of them.


Actual results:

in toolkit/mozapps/readstrings/readstrings.cpp in line #175 there is a memory allocation. (fileContents=new char[..];) Below, there are several exit point, e.g. when file read error happens, and there is no delete[] before exit.
As far as I see, before the final exit point (when no error happened) should use a delete[] also, but maybe I am lost in the memory management.


Expected results:

You should free memory, when it is not needed anymore.(Or use some kind of smart pointer.)
Keywords: mlk
Product: Firefox → Toolkit
QA Contact: general → general
Blocks: cppcheck
Whiteboard: [good first bug] [mentor=jdm]
Hugo, thanks for the patch! It's correct, but I think it would be better to use a class that will automatically delete the memory in its destructor, ie.

class AutoDelete {
public:
  AutoDelete(char *buffer);
  ~AutoDelete();
private:
  char *mBuffer;
};

Then we can be sure that if this code ever grows any more exit points, the memory will still be freed.
Attached patch ensure fileContents is freed (obsolete) — Splinter Review
Attachment #554286 - Attachment is obsolete: true
Comment on attachment 554576 [details] [diff] [review]
ensure fileContents is freed

patch updated to follow Josh's suggested implementation
Comment on attachment 554576 [details] [diff] [review]
ensure fileContents is freed

This looks really good to me Hugo, I'm just going to request you remove if |if (ptr_)| before we get an official review from a Toolkit peer on this patch. delete is a no-op on null pointers, so the check is useless.
Whiteboard: [good first bug] [mentor=jdm] → [good first bug] [mentor=jdm][MemShrink]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → hugo.t.r
Whiteboard: [good first bug] [mentor=jdm][MemShrink] → [good first bug] [mentor=jdm][MemShrink:P3]
This code is used in the updater I believe.
Component: General → Application Update
QA Contact: general → application.update
Attachment #554576 - Flags: review?(robert.bugzilla)
I would propose to move the 'new' into the constructor of the AutoCharArray class so that new and delete are in one place.
Comment on attachment 554576 [details] [diff] [review]
ensure fileContents is freed

Good call on comment #7. With that this will get an r+ and thanks!
Attachment #554576 - Flags: review?(robert.bugzilla) → review-
Hugo, are you going to be able to address comment 7?
Whiteboard: [good first bug] [mentor=jdm][MemShrink:P3] → [good first bug] [mentor=jdm][MemShrink:P3][lang=c++]
I've made the changes to Hugo's patch that were recommended in the previous reviews.
Attachment #580724 - Flags: review?(robert.bugzilla)
Nice!
Comment on attachment 580724 [details] [diff] [review]
hugo's patch with the suggested changes

Brian, please take this review so I can focus on the silent update reviews. Thanks
Attachment #580724 - Flags: review?(robert.bugzilla) → review?(netzen)
Comment on attachment 580724 [details] [diff] [review]
hugo's patch with the suggested changes

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

Looks good, it's too bad we can't use nsAutoArrayPtr<char> here :(
I debugged it also just to make sure it was working correctly and it was.
Attachment #580724 - Flags: review?(netzen) → review+
Attachment #554576 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [good first bug] [mentor=jdm][MemShrink:P3][lang=c++] → [waiting on bug 709193] [good first bug] [mentor=jdm][MemShrink:P3][lang=c++]
edmorley: This is marked as checkin-needed, but also waiting on bug 709193. Is this OK to land or should I hold off?
The trees were reopened a couple of hours ago, so this ok to land (subject to asking the sheriff for a place in the queue) :-)
Whiteboard: [waiting on bug 709193] [good first bug] [mentor=jdm][MemShrink:P3][lang=c++] → [good first bug] [mentor=jdm][MemShrink:P3][lang=c++]
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/19772fffc1f5
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [good first bug] [mentor=jdm][MemShrink:P3][lang=c++] → [good first bug] [mentor=jdm][MemShrink:P3][lang=c++][fixed-in-fx-team]
Thanks for your contribution Hugo and congrats on your first patch :)
https://hg.mozilla.org/mozilla-central/rev/19772fffc1f5

woot!
Status: ASSIGNED → 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.