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

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[David Volgyes]

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

Comment 1

[Hugo Tavares Reis]

Attachment: free fileContents before every exit point

Comment 2

[Josh Matthews [: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.

Comment 3

[Hugo Tavares Reis]

Attachment: ensure fileContents is freed

Comment 4

[Hugo Tavares Reis]

Comment on attachment 554576 [details] [diff] [review]
ensure fileContents is freed

patch updated to follow Josh's suggested implementation

Comment 5

[Josh Matthews [:jdm]]

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.

Comment 6

[(no longer active)]

This code is used in the updater I believe.

Comment 7

[Alfred Kayser]

I would propose to move the 'new' into the constructor of the AutoCharArray class so that new and delete are in one place.

Comment 8

[Robert Strong (they/them - no direct email)]

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!

Comment 9

[Josh Matthews [:jdm]]

Hugo, are you going to be able to address comment 7?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: hugo's patch with the suggested changes

I've made the changes to Hugo's patch that were recommended in the previous reviews.

Comment 11

[Alfred Kayser]

Nice!

Comment 12

[Robert Strong (they/them - no direct email)]

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

Comment 13

[Brian R. Bondy [:bbondy]]

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.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

edmorley: This is marked as checkin-needed, but also waiting on bug 709193. Is this OK to land or should I hold off?

Comment 15

[Ed Morley [:emorley]]

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

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/19772fffc1f5

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for your contribution Hugo and congrats on your first patch :)

Comment 18

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/19772fffc1f5

woot!