bug-coreutils
[Top][All Lists]
Advanced

[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: Jim Meyering
Subject: Re: [PATCH] sort.c: Fix minor memory leak, "files" is never free'd
Date: Tue, 16 Feb 2010 20:23:36 +0100

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]