Wang Shilong
2013-Jul-09 23:01 UTC
[PATCH] Btrfs-progs: fix memory leak in open_file_or_dir()
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 -- 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
Anand Jain
2013-Jul-10 03:43 UTC
Re: [PATCH] Btrfs-progs: fix memory leak in open_file_or_dir()
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
anand jain
2013-Jul-14 08:33 UTC
Re: [PATCH] Btrfs-progs: fix memory leak in open_file_or_dir()
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-- 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
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
Anand Jain
2013-Jul-14 08:48 UTC
[PATCH v2] Btrfs-progs: fix memory leak in open_file_or_dir()
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
Wang Shilong
2013-Jul-14 13:58 UTC
Re: [PATCH v2] Btrfs-progs: fix memory leak in open_file_or_dir()
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-- 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
anand jain
2013-Jul-14 14:40 UTC
Re: [PATCH v2] Btrfs-progs: fix memory leak in open_file_or_dir()
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 >-- 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
Wang Shilong
2013-Jul-14 14:51 UTC
Re: [PATCH v2] Btrfs-progs: fix memory leak in open_file_or_dir()
> > > 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 >>-- 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