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

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

[Wang Shilong]

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: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
 utils.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/utils.c b/utils.c
index 7b4cd74..0afff55 100644
--- a/utils.c
+++ b/utils.c
@@ -1480,7 +1480,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);
@@ -1496,9 +1496,10 @@ int open_file_or_dir(const char *fname)
 	} else {
 		fd = open(fname, O_RDWR);
 	}
-	if (fd < 0) {
-		return -3;
-	}
+	if (fd < 0)
+		fd = -3;
+	if (dirstream)
+		closedir(dirstream);
 	return fd;
 }
 
-- 
1.7.11.7

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

[Anand Jain]

  Thanks.


Reviewed-by: Anand Jain <anand.jain@oracle.com>


On 07/10/2013 07:01 AM, Wang Shilong wrote:
> 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: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> ---
>   utils.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/utils.c b/utils.c
> index 7b4cd74..0afff55 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1480,7 +1480,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);
> @@ -1496,9 +1496,10 @@ int open_file_or_dir(const char *fname)
>   	} else {
>   		fd = open(fname, O_RDWR);
>   	}
> -	if (fd < 0) {
> -		return -3;
> -	}
> +	if (fd < 0)
> +		fd = -3;
> +	if (dirstream)
> +		closedir(dirstream);
>   	return fd;
>   }
>
>

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

[anand jain]

  I am sorry that this has missed my eyes.
  There is a regression in this patch and most
  the commands are failing.

---
# btrfs fi df /btrfs
ERROR: couldn't get space info on '/btrfs' - Bad file descriptor
---

 >> +    if (fd < 0)
 >> +        fd = -3;
 >> +    if (dirstream)
 >> +        closedir(dirstream);

   We have to closedir only when fd < 0.


Thanks, Anand


On 10/07/2013 11:43, Anand Jain wrote:
>
>   Thanks.
>
>
> Reviewed-by: Anand Jain <anand.jain@oracle.com>
>
>
> On 07/10/2013 07:01 AM, Wang Shilong wrote:
>> 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: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>> ---
>>   utils.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/utils.c b/utils.c
>> index 7b4cd74..0afff55 100644
>> --- a/utils.c
>> +++ b/utils.c
>> @@ -1480,7 +1480,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);
>> @@ -1496,9 +1496,10 @@ int open_file_or_dir(const char *fname)
>>       } else {
>>           fd = open(fname, O_RDWR);
>>       }
>> -    if (fd < 0) {
>> -        return -3;
>> -    }
>> +    if (fd < 0)
>> +        fd = -3;
>> +    if (dirstream)
>> +        closedir(dirstream);
>>       return fd;
>>   }
>>
>>
> --
> 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

[PATCH] From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>

[Anand Jain]

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

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

[Anand Jain]

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

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

[Wang Shilong]

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……


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

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

[anand jain]

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
> 

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

[Wang Shilong]

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

Yeah, i try to do something like:

open_file_or_dir(char *name, DIR **dirstream)

It really touches a lot of places.. So i hesitate if there is a better method to
do this..

Thanks
Wang
> 
> 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
>>