[PATCH 0/3] btrfs-progs: Small fixes for code using strdup()

[PATCH 0/3] btrfs-progs: Small fixes for code using strdup()

[Byongho Lee]

1. fix memory leak in btrfs-convert main()
2. fix memory leak in btrfs-map-logical main()
3. add memory allocation fail check in btrfs_add_to_fsid()

Byongho Lee (3):
  btrfs-progs: fix memory leak in btrfs-convert main()
  btrfs-progs: fix memory leak in btrfs-map-logical main()
  btrfs-progs: add memory allocation fail check in btrfs_add_to_fsid()

 btrfs-convert.c     |  9 +++++----
 btrfs-map-logical.c |  2 ++
 utils.c             | 20 ++++++++++++--------
 3 files changed, 19 insertions(+), 12 deletions(-)

-- 
2.5.0

[PATCH 1/3] btrfs-progs: fix memory leak in btrfs-convert main()

[Byongho Lee]

In btrfs-convert main(), strdup() allocates memory to fslabel but that
memory is not freed. We could fix it by adding free() calls to every
return point, but that would make the code messy because there are
several return paths.
So I fix it by changing the code using strdup() with local array and
strncpy().

And btrfs-convert main() guarantees that string length of fslabel is not
to exceed 'BTRFS_LABEL_SIZE', so it's enough to use strcpy() instead of
strncpy() to copy fslabel in do_convert().

Signed-off-by: Byongho Lee <bhlee.kernel@gmail.com>
---
 btrfs-convert.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/btrfs-convert.c b/btrfs-convert.c
index 917bbc1b74d2..25ae424ea73b 100644
--- a/btrfs-convert.c
+++ b/btrfs-convert.c
@@ -2428,7 +2428,7 @@ static int do_convert(const char *devname, int datacsum, int packing, int noxatt
 		fprintf(stderr, "copy label '%s'\n",
 				root->fs_info->super_copy->label);
 	} else if (copylabel == -1) {
-		strncpy(root->fs_info->super_copy->label, fslabel, BTRFS_LABEL_SIZE);
+		strcpy(root->fs_info->super_copy->label, fslabel);
 		fprintf(stderr, "set label to '%s'\n", fslabel);
 	}
 
@@ -2868,7 +2868,7 @@ int main(int argc, char *argv[])
 	int usage_error = 0;
 	int progress = 1;
 	char *file;
-	char *fslabel = NULL;
+	char fslabel[BTRFS_LABEL_SIZE+1];
 	u64 features = BTRFS_MKFS_DEFAULT_FEATURES;
 
 	while(1) {
@@ -2910,8 +2910,9 @@ int main(int argc, char *argv[])
 				break;
 			case 'l':
 				copylabel = -1;
-				fslabel = strdup(optarg);
-				if (strlen(fslabel) > BTRFS_LABEL_SIZE) {
+				fslabel[BTRFS_LABEL_SIZE] = 0;
+				strncpy(fslabel, optarg, sizeof(fslabel));
+				if (fslabel[BTRFS_LABEL_SIZE]) {
 					fprintf(stderr,
 						"warning: label too long, trimmed to %d bytes\n",
 						BTRFS_LABEL_SIZE);
-- 
2.5.0

Re: [PATCH 1/3] btrfs-progs: fix memory leak in btrfs-convert main()

[David Sterba]

On Fri, Aug 28, 2015 at 12:38:16AM +0900, Byongho Lee wrote:
> In btrfs-convert main(), strdup() allocates memory to fslabel but that
> memory is not freed. We could fix it by adding free() calls to every
> return point, but that would make the code messy because there are
> several return paths.
> So I fix it by changing the code using strdup() with local array and
> strncpy().
> 
> And btrfs-convert main() guarantees that string length of fslabel is not
> to exceed 'BTRFS_LABEL_SIZE', so it's enough to use strcpy() instead of
> strncpy() to copy fslabel in do_convert().
> 
> Signed-off-by: Byongho Lee <bhlee.kernel@gmail.com>

Applied, thanks.

[PATCH 2/3] btrfs-progs: fix memory leak in btrfs-map-logical main()

[Byongho Lee]

In btrfs-map-logical main(), strdup() allocates memory to output_file,
but that memory is not freed.
So add missing free() calls before return.

Signed-off-by: Byongho Lee <bhlee.kernel@gmail.com>
---
 btrfs-map-logical.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/btrfs-map-logical.c b/btrfs-map-logical.c
index a88e56e39dbb..d9fa6b29d3c9 100644
--- a/btrfs-map-logical.c
+++ b/btrfs-map-logical.c
@@ -262,6 +262,7 @@ int main(int ac, char **av)
 	root = open_ctree(dev, 0, 0);
 	if (!root) {
 		fprintf(stderr, "Open ctree failed\n");
+		free(output_file);
 		exit(1);
 	}
 
@@ -354,6 +355,7 @@ out_close_fd:
 	if (output_file && out_fd != 1)
 		close(out_fd);
 close:
+	free(output_file);
 	close_ctree(root);
 	if (ret < 0)
 		ret = 1;
-- 
2.5.0

Re: [PATCH 2/3] btrfs-progs: fix memory leak in btrfs-map-logical main()

[David Sterba]

On Fri, Aug 28, 2015 at 12:38:17AM +0900, Byongho Lee wrote:
> In btrfs-map-logical main(), strdup() allocates memory to output_file,
> but that memory is not freed.
> So add missing free() calls before return.
> 
> Signed-off-by: Byongho Lee <bhlee.kernel@gmail.com>

Applied, thanks.

[PATCH 3/3] btrfs-progs: add memory allocation fail check in btrfs_add_to_fsid()

[Byongho Lee]

In btrfs_add_to_fsid(), strdup() allocates memory to device->name, but
the return value is not checked.
So add the return value check and error handling code.
And clean-up error handling code for ENOMEM case.

Signed-off-by: Byongho Lee <bhlee.kernel@gmail.com>
---
 utils.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/utils.c b/utils.c
index 280637d896c1..160a8c273da8 100644
--- a/utils.c
+++ b/utils.c
@@ -729,21 +729,18 @@ int btrfs_add_to_fsid(struct btrfs_trans_handle *trans,
 	struct btrfs_super_block *super = root->fs_info->super_copy;
 	struct btrfs_device *device;
 	struct btrfs_dev_item *dev_item;
-	char *buf;
+	char *buf = NULL;
 	u64 total_bytes;
 	u64 num_devs;
 	int ret;
 
 	device = kzalloc(sizeof(*device), GFP_NOFS);
 	if (!device)
-		return -ENOMEM;
-	buf = kmalloc(sectorsize, GFP_NOFS);
-	if (!buf) {
-		kfree(device);
-		return -ENOMEM;
-	}
+		goto err_nomem;
+	buf = kzalloc(sectorsize, GFP_NOFS);
+	if (!buf)
+		goto err_nomem;
 	BUG_ON(sizeof(*disk_super) > sectorsize);
-	memset(buf, 0, sectorsize);
 
 	disk_super = (struct btrfs_super_block *)buf;
 	dev_item = &disk_super->dev_item;
@@ -761,6 +758,8 @@ int btrfs_add_to_fsid(struct btrfs_trans_handle *trans,
 	device->total_ios = 0;
 	device->dev_root = root->fs_info->dev_root;
 	device->name = strdup(path);
+	if (!device->name)
+		goto err_nomem;
 
 	ret = btrfs_add_device(trans, root, device);
 	BUG_ON(ret);
@@ -790,6 +789,11 @@ int btrfs_add_to_fsid(struct btrfs_trans_handle *trans,
 	list_add(&device->dev_list, &root->fs_info->fs_devices->devices);
 	device->fs_devices = root->fs_info->fs_devices;
 	return 0;
+
+err_nomem:
+	kfree(device);
+	kfree(buf);
+	return -ENOMEM;
 }
 
 static void btrfs_wipe_existing_sb(int fd)
-- 
2.5.0

Re: [PATCH 3/3] btrfs-progs: add memory allocation fail check in btrfs_add_to_fsid()

[David Sterba]

On Fri, Aug 28, 2015 at 12:38:18AM +0900, Byongho Lee wrote:
> In btrfs_add_to_fsid(), strdup() allocates memory to device->name, but
> the return value is not checked.
> So add the return value check and error handling code.
> And clean-up error handling code for ENOMEM case.
> 
> Signed-off-by: Byongho Lee <bhlee.kernel@gmail.com>

Applied, thanks. Btw, the kmalloc/kfree functions come from the kernel
code and should not be used in the pure-userspace code, like utils.c,
but I don't mind for now.