Re: [PATCH v2] Btrfs-progs: fix memory leak in open_file_or_dir()

[anand jain <anand.jain@oracle.com>]

On 14/07/2013 21:58, Wang Shilong wrote:
> Hello Anand and David,
> 
> I use the tool valgrind to whether there exists memory leak:
> 
> valgrind  --tool=memcheck  --trace-origins=yes --leak-chek=full btrfs sub list /mnt
> There still exists memory leak related to open_file_or_dir() (After applying this patch).
> 
> I guess it is because we should call closedir(dirstram) rather than close(fd) if opening a directory.
> Maybe we can make open_file_or_dir() something like:

> open_file_or_dir(char *name, DIR **dirstream)
> 
> if we opened a directory, we close it by calling closeidr(dirstream).
> elsewise, close(fd) is ok!..
> 
> I am wondering why close(fd) can not just finish this work……

 now I get your broader point of view. For some reason it
 gave me an impression you are trying to handle a proper
 cleanup operation if when dirfd(dirstream) fails and below
 patch would just address that.

 Now when open_file_or_dir() is successful we still need a
 proper closing code.  May be its a good idea to write a new
 patch to include that.. it touches a lot of places.

Thanks, Anand


> Thanks,
> Wang
>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>
>> After calling opendir() successfully, closedir() should be
>> also called to free memory. Otherwise, memory leak happens.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>> Reviewed-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> utils.c |    6 ++++--
>> 1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/utils.c b/utils.c
>> index d3bec9b..4b3c778 100644
>> --- a/utils.c
>> +++ b/utils.c
>> @@ -1503,7 +1503,7 @@ int open_file_or_dir(const char *fname)
>> {
>> 	int ret;
>> 	struct stat st;
>> -	DIR *dirstream;
>> +	DIR *dirstream = NULL;
>> 	int fd;
>>
>> 	ret = stat(fname, &st);
>> @@ -1520,7 +1520,9 @@ int open_file_or_dir(const char *fname)
>> 		fd = open(fname, O_RDWR);
>> 	}
>> 	if (fd < 0) {
>> -		return -3;
>> +		fd = -3;
>> +		if (dirstream)
>> +			closedir(dirstream);
>> 	}
>> 	return fd;
>> }
>> -- 
>> 1.7.7.6
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>