Memory leak in binreloc
Andy Buckley
Good to see that autopackage is still going... I've been using
binreloc in particular on three much-used particle physics packages
for the last few years, and it's been *very* useful. However, in a
recent valgrind-type debugging and leak-squashing spree, we found that
a *lot* of leak messages were coming from binreloc. Not much in terms
of memory lost, but the sheer number of messages made it hard to find
the other, more substantial single leaks. Anyway, I've patched my
personal binreloc.c files, but it would be great if you could update
the generating Perl script... here's a diff fragment (since the
context is probably not useful for patching the script):
- path = strdup (path);
+ char * oldpath = path;
+ path = strdup (oldpath);
+ free (oldpath);
For anyone who knows strdup(foo), you can easily see the leak... the
original "path" pointer is overwritten by a pointer to a newly-
malloc'd duplicate of that string: bang, original memory leaked.
Easily fixed with a temporary variable... assuming that there was
actually a reason to be replacing path with a clone of itself in the
first place! :)
Another thing which was a bit of a gotcha for a mainly C++-based
programmer like myself, was knowing that I would need to take
responsibility for deleting the path strings that binreloc provides:
it was only when I looked inside the source that I realised that they
were all returned by strdup... i.e. I was the only person who had any
hope of cleaning up that memory. Hence I had to make changes like this
in my C++ client code:
- const std::string sharedir = br_find_data_dir(DEFAULTDATADIR);
+ char* temp = br_find_data_dir(DEFAULTDATADIR);
+ const std::string sharedir(temp);
+ free (temp);
It would be nice if this could make it onto the website when it's
updated: good luck with that. I do hope autopackage keeps going: lots
of nice ideas, great implementation, it would be a real shame for it
to stagnate.
Best wishes,
Andy
Jan Niklas Hasse
On Sat, Jul 31, 2010 at 12:38 AM, Andy Buckley <an...@insectnation.org> wrote:
> Good to see that autopackage is still going... I've been using
> binreloc in particular on three much-used particle physics packages
> for the last few years, and it's been *very* useful. However, in a
> recent valgrind-type debugging and leak-squashing spree, we found that
> a *lot* of leak messages were coming from binreloc. Not much in terms
> of memory lost, but the sheer number of messages made it hard to find
> the other, more substantial single leaks. Anyway, I've patched my
> personal binreloc.c files, but it would be great if you could update
> the generating Perl script... here's a diff fragment (since the
> context is probably not useful for patching the script):
>
> - path = strdup (path);
> + char * oldpath = path;
> + path = strdup (oldpath);
> + free (oldpath);
>
> For anyone who knows strdup(foo), you can easily see the leak... the
> original "path" pointer is overwritten by a pointer to a newly-
> malloc'd duplicate of that string: bang, original memory leaked.
> Easily fixed with a temporary variable... assuming that there was
> actually a reason to be replacing path with a clone of itself in the
> first place! :)
Ah thanks for the fix :)
I assume this is the only part where it happens?
> Another thing which was a bit of a gotcha for a mainly C++-based
> programmer like myself, was knowing that I would need to take
> responsibility for deleting the path strings that binreloc provides:
> it was only when I looked inside the source that I realised that they
> were all returned by strdup... i.e. I was the only person who had any
> hope of cleaning up that memory. Hence I had to make changes like this
> in my C++ client code:
>
> - const std::string sharedir = br_find_data_dir(DEFAULTDATADIR);
> + char* temp = br_find_data_dir(DEFAULTDATADIR);
> + const std::string sharedir(temp);
> + free (temp);
>
> It would be nice if this could make it onto the website when it's
> updated: good luck with that.
Okay, I will put in there when I update it.
> I do hope autopackage keeps going: lots
> of nice ideas, great implementation, it would be a real shame for it
> to stagnate.
I'm afraid that it will most likely stagnate. But I'm talking with the
Listaller developer atm and hope that Listaller will be a good
replacement as soon as possible.
Andy Buckley
> > Good to see that autopackage is still going... I've been using
> > binreloc in particular on three much-used particle physics packages
> > for the last few years, and it's been *very* useful. However, in a
> > recent valgrind-type debugging and leak-squashing spree, we found that
> > a *lot* of leak messages were coming from binreloc. Not much in terms
> > of memory lost, but the sheer number of messages made it hard to find
> > the other, more substantial single leaks. Anyway, I've patched my
> > personal binreloc.c files, but it would be great if you could update
> > the generating Perl script... here's a diff fragment (since the
> > context is probably not useful for patching the script):
>
> > - path = strdup (path);
> > + char * oldpath = path;
> > + path = strdup (oldpath);
> > + free (oldpath);
>
> > For anyone who knows strdup(foo), you can easily see the leak... the
> > original "path" pointer is overwritten by a pointer to a newly-
> > malloc'd duplicate of that string: bang, original memory leaked.
> > Easily fixed with a temporary variable... assuming that there was
> > actually a reason to be replacing path with a clone of itself in the
> > first place! :)
>
> Ah thanks for the fix :)
> I assume this is the only part where it happens?
associated with the br_* function return values, my code was
completely leak free (according to the amazing Mac dtrace GUI...
incredible leak-checking tool)
> > I do hope autopackage keeps going: lots
> > of nice ideas, great implementation, it would be a real shame for it
> > to stagnate.
>
> I'm afraid that it will most likely stagnate. But I'm talking with the
> Listaller developer atm and hope that Listaller will be a good
> replacement as soon as possible.
and some of these handy tools persist: they really help to solve some
problems with user-installed code: the normal situation with academic
tools!
Andy