[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] sort.c: Fix minor memory leak, "files" is never free'd
From: |
Joey Degges |
Subject: |
Re: [PATCH] sort.c: Fix minor memory leak, "files" is never free'd |
Date: |
Wed, 17 Feb 2010 18:48:38 -0800 |
On Tue, Feb 16, 2010 at 11:23 AM, Jim Meyering <address@hidden> wrote:
> Joey Degges wrote:
> > On Tue, Feb 16, 2010 at 5:05 AM, Eric Blake <address@hidden> wrote:
> >
> >> According to Joey Degges on 2/16/2010 2:02 AM:
> >> > Hello,
> >> >
> >> > At sort.c:3271 'files' is allocated but it is not free'd before main
> >> exits:
> >> > files = xnmalloc (argc, sizeof *files);
> >>
> >> Thanks for the patches. However, calling free() immediately before
> exit()
> >> is a lint-like activity - it is actually SLOWER to explicitly free
> memory
> >> rather than just exiting and letting the OS cleanup reclaim the memory
> as
> >> part of process death. If we accept patches like this, it will be to
> make
> >> other leak detections easier, but as such, it should probably be
> properly
> >> guarded by #if LINT or something similar to make it apparent that it is
> >> only needed when looking for leaks and not in the common case.
> >
> >
> > Thanks for your insight -- I was not aware of 'lint' before. I have
> > reformatted the patch with #ifdef lint so that this will only be used if
> > gcc-warnings is enabled. If this looks good I will also resubmit the
> other
> > two patches.
> >
> >>From 0018a314269bc8a9b89e82be2cbf17a08d28f297 Mon Sep 17 00:00:00 2001
> > From: Joey Degges <address@hidden>
> > Date: Mon, 15 Feb 2010 23:30:31 -0800
> > Subject: [PATCH 1/3] sort.c: Fix minor memory leak, 'files' is never
> free'd
>
> Thanks for caring, but at least in my book, this is not even a "minor
> leak".
> A more apt one-line summary would be "sort: free memory allocated in main"
>
> > src/sort.c | 5 +++++
> > 1 files changed, 5 insertions(+), 0 deletions(-)
> >
> > diff --git a/src/sort.c b/src/sort.c
> > index 481fdb8..a2cba05 100644
> > --- a/src/sort.c
> > +++ b/src/sort.c
> > @@ -3692,6 +3692,11 @@ main (int argc, char **argv)
> > else
> > sort (files, nfiles, outfile);
> >
> > +#ifdef lint
> > + if (nfiles != 0)
> > + free (files);
> > +#endif
>
> Also, the above will malfunction when nfiles is initially 0,
> since we set file = &minus and "nfiles = 1".
> With your proposed addition, that would make sort
> call free(&minus), which would probably segfault.
>
> Considering that freeing "files" is not at all important,
> I think it would be better to tell whatever tool you're
> using that this particular case is not a problem.
> In valgrind, you can add a so-called "suppression".
>
Thank you for the comments. I know that it would easier to just suppress the
errors and move on but I would like to see this fixed.
This fixes the issues where nfiles is 0 and also when --files0-from is
specified. In addition, this may speed up the case where nfiles is 0 since
files would never be free'd.
diff --git a/src/sort.c b/src/sort.c
index 481fdb8..435f6be 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -3166,6 +3166,7 @@ main (int argc, char **argv)
bool posixly_correct = (getenv ("POSIXLY_CORRECT") != NULL);
bool obsolete_usage = (posix2_version () < 200112);
char **files;
+ char **files_ptr;
char *files_from = NULL;
struct Tokens tok;
char const *outfile = NULL;
@@ -3268,7 +3269,7 @@ main (int argc, char **argv)
gkey.month = gkey.reverse = false;
gkey.skipsblanks = gkey.skipeblanks = false;
- files = xnmalloc (argc, sizeof *files);
+ files_ptr = files = xnmalloc (argc, sizeof *files);
for (;;)
{
@@ -3559,6 +3560,7 @@ main (int argc, char **argv)
{
size_t i;
free (files);
+ files_ptr = NULL;
files = tok.tok;
nfiles = tok.n_tok;
for (i = 0; i < nfiles; i++)
@@ -3651,7 +3653,6 @@ main (int argc, char **argv)
{
static char *minus = (char *) "-";
nfiles = 1;
- free (files);
files = −
}
@@ -3692,6 +3693,12 @@ main (int argc, char **argv)
else
sort (files, nfiles, outfile);
+#ifdef lint
+ free (files_ptr);
+ if (files_ptr == NULL)
+ readtokens0_free (&tok);
+#endif
+
if (have_read_stdin && fclose (stdin) == EOF)
die (_("close failed"), "-");