[PATCH 0/5] Memory leaks amended

[PATCH 0/5] Memory leaks amended

[Gui Hecheng]

Memory leaks found by the tool--valgrind along with static reviewing.

Based on Daivd's branch 'integration-20130903'.

Gui Hecheng (5):
  btrfs-progs:free local variable buf upon unsuccessful returns
  btrfs-progs:local variable memory freed
  btrfs-progs: missing tree-freeing statements added
  btrfs-progs:free the local list pending_list in btrfs_scan_one_dir
  btrfs-progs:free strdup()s that are not freed

 btrfs-image.c    |  2 ++
 cmds-check.c     |  5 +++++
 cmds-subvolume.c | 48 ++++++++++++++++++++++++++++++++++--------------
 mkfs.c           | 12 +++++++++++-
 utils.c          | 10 ++++++++--
 5 files changed, 60 insertions(+), 17 deletions(-)

-- 
1.8.0.1

[PATCH 1/5] btrfs-progs:free local variable buf upon unsuccessful returns

[Gui Hecheng]

The variable "buf" passed into find_collision() as parameter "name" should be freed on unsuccessful returns.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 btrfs-image.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/btrfs-image.c b/btrfs-image.c
index 3ea3730..1a52aa6 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -284,6 +284,7 @@ static char *find_collision(struct metadump_struct *md, char *name,
 	val = malloc(sizeof(struct name));
 	if (!val) {
 		fprintf(stderr, "Couldn't sanitize name, enomem\n");
+		free(name);
 		return NULL;
 	}
 
@@ -295,6 +296,7 @@ static char *find_collision(struct metadump_struct *md, char *name,
 	if (!val->sub) {
 		fprintf(stderr, "Couldn't sanitize name, enomem\n");
 		free(val);
+		free(name);
 		return NULL;
 	}
 
-- 
1.8.0.1

[PATCH 2/5] btrfs-progs:local variable memory freed

[Gui Hecheng]

The local probe variable in is_ssd() freed upon unsuccessful return;
The local dir_head list in make_image() freed upon unsuccessful return.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 mkfs.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/mkfs.c b/mkfs.c
index 1701783..415889e 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -1024,6 +1024,8 @@ static int make_image(char *source_dir, struct btrfs_root *root, int out_fd)
 
 	struct directory_name_entry dir_head;
 
+	struct directory_name_entry *dir_entry = NULL;
+
 	ret = lstat(source_dir, &root_st);
 	if (ret) {
 		fprintf(stderr, "unable to lstat the %s\n", source_dir);
@@ -1043,6 +1045,12 @@ static int make_image(char *source_dir, struct btrfs_root *root, int out_fd)
 	printf("Making image is completed.\n");
 	return 0;
 fail:
+	while (!list_empty(&dir_head.list)) {
+		dir_entry = list_entry(dir_head.list.next,
+				       struct directory_name_entry, list);
+		list_del(&dir_entry->list);
+		free(dir_entry);
+	}
 	fprintf(stderr, "Making image is aborted.\n");
 	return -1;
 }
@@ -1161,8 +1169,10 @@ static int is_ssd(const char *file)
 
 	/* Device number of this disk (possibly a partition) */
 	devno = blkid_probe_get_devno(probe);
-	if (!devno)
+	if (!devno) {
+		blkid_free_probe(probe);
 		return 0;
+	}
 
 	/* Get whole disk name (not full path) for this devno */
 	blkid_devno_to_wholedisk(devno, wholedisk, sizeof(wholedisk), NULL);
-- 
1.8.0.1

[PATCH 3/5] btrfs-progs: missing tree-freeing statements added

[Gui Hecheng]

The seen cache_tree in run_next_block freed.
Originally, this "missing" causes memory leaks, reported by valgrind.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 cmds-check.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/cmds-check.c b/cmds-check.c
index 6cbd5a6..0cba4cc 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -3595,6 +3595,11 @@ static int run_next_block(struct btrfs_root *root,
 		remove_cache_extent(nodes, cache);
 		free(cache);
 	}
+	cache = lookup_cache_extent(seen, bytenr, size);
+	if (cache) {
+		remove_cache_extent(seen, cache);
+		free(cache);
+	}
 
 	/* fixme, get the real parent transid */
 	buf = read_tree_block(root, bytenr, size, 0);
-- 
1.8.0.1

[PATCH 4/5] btrfs-progs:free the local list pending_list in btrfs_scan_one_dir

[Gui Hecheng]

Originally the local pending_list is not guaranteed to be freed upon fails, it should be emptyed and the elements should be freed.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 utils.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/utils.c b/utils.c
index d022d58..134bf80 100644
--- a/utils.c
+++ b/utils.c
@@ -1066,8 +1066,8 @@ again:
 	dirp = opendir(dirname);
 	if (!dirp) {
 		fprintf(stderr, "Unable to open %s for scanning\n", dirname);
-		free(fullpath);
-		return -ENOENT;
+		ret = -ENOENT;
+		goto fail;
 	}
 	while(1) {
 		dirent = readdir(dirp);
@@ -1133,6 +1133,12 @@ again:
 fail:
 	free(pending);
 	free(fullpath);
+	while (!list_empty(&pending_list)) {
+		pending = list_entry(pending_list.next, struct pending_dir,
+				     list);
+		list_del(&pending->list);
+		free(pending);
+	}
 	if (dirp)
 		closedir(dirp);
 	return ret;
-- 
1.8.0.1

[PATCH 5/5] btrfs-progs:free strdup()s that are not freed

[Gui Hecheng]

The strdup()s not freed are reported as memory leaks by valgrind.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 cmds-subvolume.c | 48 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index e1fa81a..51c529c 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -75,6 +75,8 @@ static int cmd_subvol_create(int argc, char **argv)
 {
 	int	retval, res, len;
 	int	fddst = -1;
+	char	*dupname = NULL;
+	char	*dupdir = NULL;
 	char	*newname;
 	char	*dstdir;
 	char	*dst;
@@ -119,10 +121,10 @@ static int cmd_subvol_create(int argc, char **argv)
 		goto out;
 	}
 
-	newname = strdup(dst);
-	newname = basename(newname);
-	dstdir = strdup(dst);
-	dstdir = dirname(dstdir);
+	dupname = strdup(dst);
+	newname = basename(dupname);
+	dupdir = strdup(dst);
+	dstdir = dirname(dupdir);
 
 	if (!strcmp(newname, ".") || !strcmp(newname, "..") ||
 	     strchr(newname, '/') ){
@@ -175,6 +177,11 @@ out:
 	close_file_or_dir(fddst, dirstream);
 	free(inherit);
 
+	if (dupname != NULL)
+		free(dupname);
+	if (dupdir != NULL)
+		free(dupdir);
+
 	return retval;
 }
 
@@ -208,6 +215,8 @@ static int cmd_subvol_delete(int argc, char **argv)
 	int	res, fd, len, e, cnt = 1, ret = 0;
 	struct btrfs_ioctl_vol_args	args;
 	char	*dname, *vname, *cpath;
+	char	*dupdname = NULL;
+	char	*dupvname = NULL;
 	char	*path;
 	DIR	*dirstream = NULL;
 
@@ -230,10 +239,10 @@ again:
 	}
 
 	cpath = realpath(path, NULL);
-	dname = strdup(cpath);
-	dname = dirname(dname);
-	vname = strdup(cpath);
-	vname = basename(vname);
+	dupdname = strdup(cpath);
+	dname = dirname(dupdname);
+	dupvname = strdup(cpath);
+	vname = basename(dupvname);
 	free(cpath);
 
 	if( !strcmp(vname,".") || !strcmp(vname,"..") ||
@@ -274,6 +283,10 @@ again:
 	}
 
 out:
+	if (dupdname != NULL)
+		free(dupdname);
+	if (dupvname != NULL)
+		free(dupvname);
 	cnt++;
 	if (cnt < argc)
 		goto again;
@@ -495,6 +508,8 @@ static int cmd_snapshot(int argc, char **argv)
 	int	res, retval;
 	int	fd = -1, fddst = -1;
 	int	len, readonly = 0;
+	char	*dupname = NULL;
+	char	*dupdir = NULL;
 	char	*newname;
 	char	*dstdir;
 	struct btrfs_ioctl_vol_args_v2	args;
@@ -562,14 +577,14 @@ static int cmd_snapshot(int argc, char **argv)
 	}
 
 	if (res > 0) {
-		newname = strdup(subvol);
-		newname = basename(newname);
+		dupname = strdup(subvol);
+		newname = basename(dupname);
 		dstdir = dst;
 	} else {
-		newname = strdup(dst);
-		newname = basename(newname);
-		dstdir = strdup(dst);
-		dstdir = dirname(dstdir);
+		dupname = strdup(dst);
+		newname = basename(dupname);
+		dupdir = strdup(dst);
+		dstdir = dirname(dupdir);
 	}
 
 	if (!strcmp(newname, ".") || !strcmp(newname, "..") ||
@@ -630,6 +645,11 @@ out:
 	close_file_or_dir(fd, dirstream2);
 	free(inherit);
 
+	if (dupname != NULL)
+		free(dupname);
+	if (dupdir != NULL)
+		free(dupdir);
+
 	return retval;
 }
 
-- 
1.8.0.1

Re: [PATCH 5/5] btrfs-progs:free strdup()s that are not freed

[Stefan Behrens]

On Thu, 5 Sep 2013 10:38:58 +0800, Gui Hecheng wrote:
> The strdup()s not freed are reported as memory leaks by valgrind.
> 
> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> ---
>  cmds-subvolume.c | 48 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> index e1fa81a..51c529c 100644
> --- a/cmds-subvolume.c
> +++ b/cmds-subvolume.c
> @@ -75,6 +75,8 @@ static int cmd_subvol_create(int argc, char **argv)
>  {
>  	int	retval, res, len;
>  	int	fddst = -1;
> +	char	*dupname = NULL;
> +	char	*dupdir = NULL;
>  	char	*newname;
>  	char	*dstdir;
>  	char	*dst;
> @@ -119,10 +121,10 @@ static int cmd_subvol_create(int argc, char **argv)
>  		goto out;
>  	}
>  
> -	newname = strdup(dst);
> -	newname = basename(newname);
> -	dstdir = strdup(dst);
> -	dstdir = dirname(dstdir);
> +	dupname = strdup(dst);
> +	newname = basename(dupname);
> +	dupdir = strdup(dst);
> +	dstdir = dirname(dupdir);
>  
>  	if (!strcmp(newname, ".") || !strcmp(newname, "..") ||
>  	     strchr(newname, '/') ){
> @@ -175,6 +177,11 @@ out:
>  	close_file_or_dir(fddst, dirstream);
>  	free(inherit);
>  
> +	if (dupname != NULL)
> +		free(dupname);
> +	if (dupdir != NULL)
> +		free(dupdir);
> +

free(3) already checks the pointer for NULL, no need to do it on your own.


>  	return retval;
>  }
>  
> @@ -208,6 +215,8 @@ static int cmd_subvol_delete(int argc, char **argv)
>  	int	res, fd, len, e, cnt = 1, ret = 0;
>  	struct btrfs_ioctl_vol_args	args;
>  	char	*dname, *vname, *cpath;
> +	char	*dupdname = NULL;
> +	char	*dupvname = NULL;
>  	char	*path;
>  	DIR	*dirstream = NULL;
>  
> @@ -230,10 +239,10 @@ again:
>  	}
>  
>  	cpath = realpath(path, NULL);
> -	dname = strdup(cpath);
> -	dname = dirname(dname);
> -	vname = strdup(cpath);
> -	vname = basename(vname);
> +	dupdname = strdup(cpath);
> +	dname = dirname(dupdname);
> +	dupvname = strdup(cpath);
> +	vname = basename(dupvname);
>  	free(cpath);
>  
>  	if( !strcmp(vname,".") || !strcmp(vname,"..") ||
> @@ -274,6 +283,10 @@ again:
>  	}
>  
>  out:
> +	if (dupdname != NULL)
> +		free(dupdname);
> +	if (dupvname != NULL)
> +		free(dupvname);

Here again.


>  	cnt++;
>  	if (cnt < argc)
>  		goto again;
> @@ -495,6 +508,8 @@ static int cmd_snapshot(int argc, char **argv)
>  	int	res, retval;
>  	int	fd = -1, fddst = -1;
>  	int	len, readonly = 0;
> +	char	*dupname = NULL;
> +	char	*dupdir = NULL;
>  	char	*newname;
>  	char	*dstdir;
>  	struct btrfs_ioctl_vol_args_v2	args;
> @@ -562,14 +577,14 @@ static int cmd_snapshot(int argc, char **argv)
>  	}
>  
>  	if (res > 0) {
> -		newname = strdup(subvol);
> -		newname = basename(newname);
> +		dupname = strdup(subvol);
> +		newname = basename(dupname);
>  		dstdir = dst;
>  	} else {
> -		newname = strdup(dst);
> -		newname = basename(newname);
> -		dstdir = strdup(dst);
> -		dstdir = dirname(dstdir);
> +		dupname = strdup(dst);
> +		newname = basename(dupname);
> +		dupdir = strdup(dst);
> +		dstdir = dirname(dupdir);
>  	}
>  
>  	if (!strcmp(newname, ".") || !strcmp(newname, "..") ||
> @@ -630,6 +645,11 @@ out:
>  	close_file_or_dir(fd, dirstream2);
>  	free(inherit);
>  
> +	if (dupname != NULL)
> +		free(dupname);
> +	if (dupdir != NULL)
> +		free(dupdir);
> +

And here.


>  	return retval;
>  }
>  
> 

Re: [PATCH 5/5] btrfs-progs:free strdup()s that are not freed

[Stefan Behrens]

On Thu, 05 Sep 2013 09:00:07 +0200, Stefan Behrens wrote:
> On Thu, 5 Sep 2013 10:38:58 +0800, Gui Hecheng wrote:
>> The strdup()s not freed are reported as memory leaks by valgrind.
>>
>> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
>> ---
>>  cmds-subvolume.c | 48 ++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 34 insertions(+), 14 deletions(-)

Just noticed that you had already sent a V2 with the things fixed that I
have commented, but you sent the 5/5 as a 1/5 and added the changelog
v1->v2 "none" which made it difficult to recognize :). But maybe David
Sterba is smart enough when he picks up the patches.


>>
>> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
>> index e1fa81a..51c529c 100644
>> --- a/cmds-subvolume.c
>> +++ b/cmds-subvolume.c
[...]

>> @@ -75,6 +75,8 @@ static int cmd_subvol_create(int argc, char **argv)
>>  {
>>  	int	retval, res, len;
>>  	int	fddst = -1;
>> +	char	*dupname = NULL;
>> +	char	*dupdir = NULL;
>>  	char	*newname;
>>  	char	*dstdir;
>>  	char	*dst;
>> @@ -119,10 +121,10 @@ static int cmd_subvol_create(int argc, char **argv)
>>  		goto out;
>>  	}
>>  
>> -	newname = strdup(dst);
>> -	newname = basename(newname);
>> -	dstdir = strdup(dst);
>> -	dstdir = dirname(dstdir);
>> +	dupname = strdup(dst);
>> +	newname = basename(dupname);
>> +	dupdir = strdup(dst);
>> +	dstdir = dirname(dupdir);
>>  
>>  	if (!strcmp(newname, ".") || !strcmp(newname, "..") ||
>>  	     strchr(newname, '/') ){
>> @@ -175,6 +177,11 @@ out:
>>  	close_file_or_dir(fddst, dirstream);
>>  	free(inherit);
>>  
>> +	if (dupname != NULL)
>> +		free(dupname);
>> +	if (dupdir != NULL)
>> +		free(dupdir);
>> +
> 
> free(3) already checks the pointer for NULL, no need to do it on your own.
> 
> 
>>  	return retval;
>>  }
>>  
>> @@ -208,6 +215,8 @@ static int cmd_subvol_delete(int argc, char **argv)
>>  	int	res, fd, len, e, cnt = 1, ret = 0;
>>  	struct btrfs_ioctl_vol_args	args;
>>  	char	*dname, *vname, *cpath;
>> +	char	*dupdname = NULL;
>> +	char	*dupvname = NULL;
>>  	char	*path;
>>  	DIR	*dirstream = NULL;
>>  
>> @@ -230,10 +239,10 @@ again:
>>  	}
>>  
>>  	cpath = realpath(path, NULL);
>> -	dname = strdup(cpath);
>> -	dname = dirname(dname);
>> -	vname = strdup(cpath);
>> -	vname = basename(vname);
>> +	dupdname = strdup(cpath);
>> +	dname = dirname(dupdname);
>> +	dupvname = strdup(cpath);
>> +	vname = basename(dupvname);
>>  	free(cpath);
>>  
>>  	if( !strcmp(vname,".") || !strcmp(vname,"..") ||
>> @@ -274,6 +283,10 @@ again:
>>  	}
>>  
>>  out:
>> +	if (dupdname != NULL)
>> +		free(dupdname);
>> +	if (dupvname != NULL)
>> +		free(dupvname);
> 
> Here again.
> 
> 
>>  	cnt++;
>>  	if (cnt < argc)
>>  		goto again;
>> @@ -495,6 +508,8 @@ static int cmd_snapshot(int argc, char **argv)
>>  	int	res, retval;
>>  	int	fd = -1, fddst = -1;
>>  	int	len, readonly = 0;
>> +	char	*dupname = NULL;
>> +	char	*dupdir = NULL;
>>  	char	*newname;
>>  	char	*dstdir;
>>  	struct btrfs_ioctl_vol_args_v2	args;
>> @@ -562,14 +577,14 @@ static int cmd_snapshot(int argc, char **argv)
>>  	}
>>  
>>  	if (res > 0) {
>> -		newname = strdup(subvol);
>> -		newname = basename(newname);
>> +		dupname = strdup(subvol);
>> +		newname = basename(dupname);
>>  		dstdir = dst;
>>  	} else {
>> -		newname = strdup(dst);
>> -		newname = basename(newname);
>> -		dstdir = strdup(dst);
>> -		dstdir = dirname(dstdir);
>> +		dupname = strdup(dst);
>> +		newname = basename(dupname);
>> +		dupdir = strdup(dst);
>> +		dstdir = dirname(dupdir);
>>  	}
>>  
>>  	if (!strcmp(newname, ".") || !strcmp(newname, "..") ||
>> @@ -630,6 +645,11 @@ out:
>>  	close_file_or_dir(fd, dirstream2);
>>  	free(inherit);
>>  
>> +	if (dupname != NULL)
>> +		free(dupname);
>> +	if (dupdir != NULL)
>> +		free(dupdir);
>> +
> 
> And here.
> 
> 
>>  	return retval;
>>  }

Re: [PATCH 5/5] btrfs-progs:free strdup()s that are not freed

[Gui Hecheng]

On Thu, 2013-09-05 at 09:10 +0200, Stefan Behrens wrote:
> On Thu, 05 Sep 2013 09:00:07 +0200, Stefan Behrens wrote:
> > On Thu, 5 Sep 2013 10:38:58 +0800, Gui Hecheng wrote:
> >> The strdup()s not freed are reported as memory leaks by valgrind.
> >>
> >> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> >> ---
> >>  cmds-subvolume.c | 48 ++++++++++++++++++++++++++++++++++--------------
> >>  1 file changed, 34 insertions(+), 14 deletions(-)
> 
> Just noticed that you had already sent a V2 with the things fixed that I
> have commented, but you sent the 5/5 as a 1/5 and added the changelog
> v1->v2 "none" which made it difficult to recognize :). But maybe David
> Sterba is smart enough when he picks up the patches.

Thank you for your friendly advice. I will handle it snow.

> >>
> >> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> >> index e1fa81a..51c529c 100644
> >> --- a/cmds-subvolume.c
> >> +++ b/cmds-subvolume.c
> [...]
> 
> >> @@ -75,6 +75,8 @@ static int cmd_subvol_create(int argc, char **argv)
> >>  {
> >>  	int	retval, res, len;
> >>  	int	fddst = -1;
> >> +	char	*dupname = NULL;
> >> +	char	*dupdir = NULL;
> >>  	char	*newname;
> >>  	char	*dstdir;
> >>  	char	*dst;
> >> @@ -119,10 +121,10 @@ static int cmd_subvol_create(int argc, char **argv)
> >>  		goto out;
> >>  	}
> >>  
> >> -	newname = strdup(dst);
> >> -	newname = basename(newname);
> >> -	dstdir = strdup(dst);
> >> -	dstdir = dirname(dstdir);
> >> +	dupname = strdup(dst);
> >> +	newname = basename(dupname);
> >> +	dupdir = strdup(dst);
> >> +	dstdir = dirname(dupdir);
> >>  
> >>  	if (!strcmp(newname, ".") || !strcmp(newname, "..") ||
> >>  	     strchr(newname, '/') ){
> >> @@ -175,6 +177,11 @@ out:
> >>  	close_file_or_dir(fddst, dirstream);
> >>  	free(inherit);
> >>  
> >> +	if (dupname != NULL)
> >> +		free(dupname);
> >> +	if (dupdir != NULL)
> >> +		free(dupdir);
> >> +
> > 
> > free(3) already checks the pointer for NULL, no need to do it on your own.
> > 
> > 
> >>  	return retval;
> >>  }
> >>  
> >> @@ -208,6 +215,8 @@ static int cmd_subvol_delete(int argc, char **argv)
> >>  	int	res, fd, len, e, cnt = 1, ret = 0;
> >>  	struct btrfs_ioctl_vol_args	args;
> >>  	char	*dname, *vname, *cpath;
> >> +	char	*dupdname = NULL;
> >> +	char	*dupvname = NULL;
> >>  	char	*path;
> >>  	DIR	*dirstream = NULL;
> >>  
> >> @@ -230,10 +239,10 @@ again:
> >>  	}
> >>  
> >>  	cpath = realpath(path, NULL);
> >> -	dname = strdup(cpath);
> >> -	dname = dirname(dname);
> >> -	vname = strdup(cpath);
> >> -	vname = basename(vname);
> >> +	dupdname = strdup(cpath);
> >> +	dname = dirname(dupdname);
> >> +	dupvname = strdup(cpath);
> >> +	vname = basename(dupvname);
> >>  	free(cpath);
> >>  
> >>  	if( !strcmp(vname,".") || !strcmp(vname,"..") ||
> >> @@ -274,6 +283,10 @@ again:
> >>  	}
> >>  
> >>  out:
> >> +	if (dupdname != NULL)
> >> +		free(dupdname);
> >> +	if (dupvname != NULL)
> >> +		free(dupvname);
> > 
> > Here again.
> > 
> > 
> >>  	cnt++;
> >>  	if (cnt < argc)
> >>  		goto again;
> >> @@ -495,6 +508,8 @@ static int cmd_snapshot(int argc, char **argv)
> >>  	int	res, retval;
> >>  	int	fd = -1, fddst = -1;
> >>  	int	len, readonly = 0;
> >> +	char	*dupname = NULL;
> >> +	char	*dupdir = NULL;
> >>  	char	*newname;
> >>  	char	*dstdir;
> >>  	struct btrfs_ioctl_vol_args_v2	args;
> >> @@ -562,14 +577,14 @@ static int cmd_snapshot(int argc, char **argv)
> >>  	}
> >>  
> >>  	if (res > 0) {
> >> -		newname = strdup(subvol);
> >> -		newname = basename(newname);
> >> +		dupname = strdup(subvol);
> >> +		newname = basename(dupname);
> >>  		dstdir = dst;
> >>  	} else {
> >> -		newname = strdup(dst);
> >> -		newname = basename(newname);
> >> -		dstdir = strdup(dst);
> >> -		dstdir = dirname(dstdir);
> >> +		dupname = strdup(dst);
> >> +		newname = basename(dupname);
> >> +		dupdir = strdup(dst);
> >> +		dstdir = dirname(dupdir);
> >>  	}
> >>  
> >>  	if (!strcmp(newname, ".") || !strcmp(newname, "..") ||
> >> @@ -630,6 +645,11 @@ out:
> >>  	close_file_or_dir(fd, dirstream2);
> >>  	free(inherit);
> >>  
> >> +	if (dupname != NULL)
> >> +		free(dupname);
> >> +	if (dupdir != NULL)
> >> +		free(dupdir);
> >> +
> > 
> > And here.
> > 
> > 
> >>  	return retval;
> >>  }
> 

Re: [PATCH 5/5] btrfs-progs:free strdup()s that are not freed

[Wang Shilong]

On 09/05/2013 04:08 PM, Gui Hecheng wrote:
> On Thu, 2013-09-05 at 09:10 +0200, Stefan Behrens wrote:
>> On Thu, 05 Sep 2013 09:00:07 +0200, Stefan Behrens wrote:
>>> On Thu, 5 Sep 2013 10:38:58 +0800, Gui Hecheng wrote:
>>>> The strdup()s not freed are reported as memory leaks by valgrind.
>>>>
>>>> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
>>>> ---
>>>>   cmds-subvolume.c | 48 ++++++++++++++++++++++++++++++++++--------------
>>>>   1 file changed, 34 insertions(+), 14 deletions(-)
>> Just noticed that you had already sent a V2 with the things fixed that I
>> have commented, but you sent the 5/5 as a 1/5 and added the changelog
>> v1->v2 "none" which made it difficult to recognize :). But maybe David
>> Sterba is smart enough when he picks up the patches.
> Thank you for your friendly advice. I will handle it snow.
             s/snow/soon ^_^

Thanks,
Wang
>
>>>> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
>>>> index e1fa81a..51c529c 100644
>>>> --- a/cmds-subvolume.c
>>>> +++ b/cmds-subvolume.c
>> [...]
>>
>>>> @@ -75,6 +75,8 @@ static int cmd_subvol_create(int argc, char **argv)
>>>>   {
>>>>   	int	retval, res, len;
>>>>   	int	fddst = -1;
>>>> +	char	*dupname = NULL;
>>>> +	char	*dupdir = NULL;
>>>>   	char	*newname;
>>>>   	char	*dstdir;
>>>>   	char	*dst;
>>>> @@ -119,10 +121,10 @@ static int cmd_subvol_create(int argc, char **argv)
>>>>   		goto out;
>>>>   	}
>>>>   
>>>> -	newname = strdup(dst);
>>>> -	newname = basename(newname);
>>>> -	dstdir = strdup(dst);
>>>> -	dstdir = dirname(dstdir);
>>>> +	dupname = strdup(dst);
>>>> +	newname = basename(dupname);
>>>> +	dupdir = strdup(dst);
>>>> +	dstdir = dirname(dupdir);
>>>>   
>>>>   	if (!strcmp(newname, ".") || !strcmp(newname, "..") ||
>>>>   	     strchr(newname, '/') ){
>>>> @@ -175,6 +177,11 @@ out:
>>>>   	close_file_or_dir(fddst, dirstream);
>>>>   	free(inherit);
>>>>   
>>>> +	if (dupname != NULL)
>>>> +		free(dupname);
>>>> +	if (dupdir != NULL)
>>>> +		free(dupdir);
>>>> +
>>> free(3) already checks the pointer for NULL, no need to do it on your own.
>>>
>>>
>>>>   	return retval;
>>>>   }
>>>>   
>>>> @@ -208,6 +215,8 @@ static int cmd_subvol_delete(int argc, char **argv)
>>>>   	int	res, fd, len, e, cnt = 1, ret = 0;
>>>>   	struct btrfs_ioctl_vol_args	args;
>>>>   	char	*dname, *vname, *cpath;
>>>> +	char	*dupdname = NULL;
>>>> +	char	*dupvname = NULL;
>>>>   	char	*path;
>>>>   	DIR	*dirstream = NULL;
>>>>   
>>>> @@ -230,10 +239,10 @@ again:
>>>>   	}
>>>>   
>>>>   	cpath = realpath(path, NULL);
>>>> -	dname = strdup(cpath);
>>>> -	dname = dirname(dname);
>>>> -	vname = strdup(cpath);
>>>> -	vname = basename(vname);
>>>> +	dupdname = strdup(cpath);
>>>> +	dname = dirname(dupdname);
>>>> +	dupvname = strdup(cpath);
>>>> +	vname = basename(dupvname);
>>>>   	free(cpath);
>>>>   
>>>>   	if( !strcmp(vname,".") || !strcmp(vname,"..") ||
>>>> @@ -274,6 +283,10 @@ again:
>>>>   	}
>>>>   
>>>>   out:
>>>> +	if (dupdname != NULL)
>>>> +		free(dupdname);
>>>> +	if (dupvname != NULL)
>>>> +		free(dupvname);
>>> Here again.
>>>
>>>
>>>>   	cnt++;
>>>>   	if (cnt < argc)
>>>>   		goto again;
>>>> @@ -495,6 +508,8 @@ static int cmd_snapshot(int argc, char **argv)
>>>>   	int	res, retval;
>>>>   	int	fd = -1, fddst = -1;
>>>>   	int	len, readonly = 0;
>>>> +	char	*dupname = NULL;
>>>> +	char	*dupdir = NULL;
>>>>   	char	*newname;
>>>>   	char	*dstdir;
>>>>   	struct btrfs_ioctl_vol_args_v2	args;
>>>> @@ -562,14 +577,14 @@ static int cmd_snapshot(int argc, char **argv)
>>>>   	}
>>>>   
>>>>   	if (res > 0) {
>>>> -		newname = strdup(subvol);
>>>> -		newname = basename(newname);
>>>> +		dupname = strdup(subvol);
>>>> +		newname = basename(dupname);
>>>>   		dstdir = dst;
>>>>   	} else {
>>>> -		newname = strdup(dst);
>>>> -		newname = basename(newname);
>>>> -		dstdir = strdup(dst);
>>>> -		dstdir = dirname(dstdir);
>>>> +		dupname = strdup(dst);
>>>> +		newname = basename(dupname);
>>>> +		dupdir = strdup(dst);
>>>> +		dstdir = dirname(dupdir);
>>>>   	}
>>>>   
>>>>   	if (!strcmp(newname, ".") || !strcmp(newname, "..") ||
>>>> @@ -630,6 +645,11 @@ out:
>>>>   	close_file_or_dir(fd, dirstream2);
>>>>   	free(inherit);
>>>>   
>>>> +	if (dupname != NULL)
>>>> +		free(dupname);
>>>> +	if (dupdir != NULL)
>>>> +		free(dupdir);
>>>> +
>>> And here.
>>>
>>>
>>>>   	return retval;
>>>>   }
>
> --
> 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 2/5] btrfs-progs:local variable memory freed

[David Sterba]

On Thu, Sep 05, 2013 at 10:38:55AM +0800, Gui Hecheng wrote:
> The local probe variable in is_ssd() freed upon unsuccessful return;

FYI, I've applied a patch from Wang Shilong "Btrfs-progs: fix compile
warning in is_ssd()" instead of the hunk below and will remove the
respective changelog line. The rest of your patch stays.

> @@ -1161,8 +1169,10 @@ static int is_ssd(const char *file)
>  
>  	/* Device number of this disk (possibly a partition) */
>  	devno = blkid_probe_get_devno(probe);
> -	if (!devno)
> +	if (!devno) {
> +		blkid_free_probe(probe);
>  		return 0;
> +	}
>  
>  	/* Get whole disk name (not full path) for this devno */
>  	blkid_devno_to_wholedisk(devno, wholedisk, sizeof(wholedisk), NULL);

Re: [PATCH 2/5] btrfs-progs:local variable memory freed

[David Sterba]

On Mon, Sep 09, 2013 at 04:18:55PM +0200, David Sterba wrote:
> On Thu, Sep 05, 2013 at 10:38:55AM +0800, Gui Hecheng wrote:
> > The local probe variable in is_ssd() freed upon unsuccessful return;
> 
> FYI, I've applied a patch from Wang Shilong "Btrfs-progs: fix compile
> warning in is_ssd()" instead of the hunk below and will remove the
> respective changelog line. The rest of your patch stays.

Scratch that, the changes are not identical.

Re: [PATCH 5/5] btrfs-progs:free strdup()s that are not freed

[David Sterba]

On Thu, Sep 05, 2013 at 09:10:51AM +0200, Stefan Behrens wrote:
> Just noticed that you had already sent a V2 with the things fixed that I
> have commented, but you sent the 5/5 as a 1/5 and added the changelog
> v1->v2 "none" which made it difficult to recognize :). But maybe David
> Sterba is smart enough when he picks up the patches.

I watch out for updates and replace the patches with the latest version.
V3 of "free strdup()s that are not freed" has been merged.

Thanks for the pointer anyway.
david