btrfs-progs: minor buffer-overrun fixes (v2)

btrfs-progs: minor buffer-overrun fixes (v2)

[Jim Meyering]

Same net result as before, but with the stray-parenthesis fix
moved to 2/4 where it belongs, rather than buried in 3/4.
Thanks to Josef for the reviews.

Also, I fixed this in 3/4:
-       args.name[BTRFS_PATH_NAME_MAX-1] = 0;
+       args.name[BTRFS_SUBVOL_NAME_MAX-1] = 0;

[PATCHv2 1/4] mkfs: use strdup in place of strlen,malloc,strcpy
[PATCHv2 2/4] restore: don't corrupt stack for a zero-length
[PATCHv2 3/4] avoid several strncpy-induced buffer overruns
[PATCHv2 4/4] mkfs: avoid heap-buffer-read-underrun for zero-length

[PATCHv2 1/4] mkfs: use strdup in place of strlen,malloc,strcpy sequence

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

* mkfs.c (traverse_directory): No semantic change.

Reviewed-by: Josef Bacik <josef@redhat.com>
---
 mkfs.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mkfs.c b/mkfs.c
index c531ef2..03239fb 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -911,8 +911,7 @@ static int traverse_directory(struct btrfs_trans_handle *trans,
 	/* Add list for source directory */
 	dir_entry = malloc(sizeof(struct directory_name_entry));
 	dir_entry->dir_name = dir_name;
-	dir_entry->path = malloc(strlen(dir_name) + 1);
-	strcpy(dir_entry->path, dir_name);
+	dir_entry->path = strdup(dir_name);

 	parent_inum = highest_inum + BTRFS_FIRST_FREE_OBJECTID;
 	dir_entry->inum = parent_inum;
-- 
1.7.10.208.gb4267

[PATCHv2 2/4] restore: don't corrupt stack for a zero-length command-line argument

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

Given a zero-length directory name, the trailing-slash removal
code would test dir_name[-1], and if it were found to be a slash,
would set it to '\0'.

Reviewed-by: Josef Bacik <josef@redhat.com>
---
 restore.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/restore.c b/restore.c
index 250c9d3..2674832 100644
--- a/restore.c
+++ b/restore.c
@@ -849,11 +849,9 @@ int main(int argc, char **argv)
 	strncpy(dir_name, argv[optind + 1], 128);

 	/* Strip the trailing / on the dir name */
-	while (1) {
-		len = strlen(dir_name);
-		if (dir_name[len - 1] != '/')
-			break;
-		dir_name[len - 1] = '\0';
+	len = strlen(dir_name);
+	while (len && dir_name[--len] == '/') {
+		dir_name[len] = '\0';
 	}

 	if (find_dir) {
-- 
1.7.10.208.gb4267

[PATCHv2 3/4] avoid several strncpy-induced buffer overruns

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

* restore.c (main): Ensure strncpy-copied dir_name is NUL-terminated.
* btrfsctl.c (main): Likewise, for a command-line argument.
* utils.c (multiple functions): Likewise.
* btrfs-list.c (add_root): Likewise.
* btrfslabel.c (change_label_unmounted): Likewise.
* cmds-device.c (cmd_add_dev, cmd_rm_dev, cmd_scan_dev): Likewise.
* cmds-filesystem.c (cmd_resize): Likewise.
* cmds-subvolume.c (cmd_subvol_create, cmd_subvol_delete, cmd_snapshot):
Likewise.

Reviewed-by: Josef Bacik <josef@redhat.com>
---
 btrfs-list.c      |    2 ++
 btrfsctl.c        |    5 +++--
 btrfslabel.c      |    1 +
 cmds-device.c     |    3 +++
 cmds-filesystem.c |    1 +
 cmds-subvolume.c  |    3 +++
 restore.c         |    3 ++-
 utils.c           |   13 ++++++++++---
 8 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/btrfs-list.c b/btrfs-list.c
index 5f4a9be..35e6139 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -183,6 +183,8 @@ static int add_root(struct root_lookup *root_lookup,
 	ri->root_id = root_id;
 	ri->ref_tree = ref_tree;
 	strncpy(ri->name, name, name_len);
+	if (name_len > 0)
+		ri->name[name_len-1] = 0;

 	ret = tree_insert(&root_lookup->root, root_id, ref_tree, &ri->rb_node);
 	if (ret) {
diff --git a/btrfsctl.c b/btrfsctl.c
index d45e2a7..518684c 100644
--- a/btrfsctl.c
+++ b/btrfsctl.c
@@ -241,9 +241,10 @@ int main(int ac, char **av)
 		fd = open_file_or_dir(fname);
 	 }

-	if (name)
+	if (name) {
                 strncpy(args.name, name, BTRFS_PATH_NAME_MAX + 1);
-	else
+                args.name[BTRFS_PATH_NAME_MAX] = 0;
+	} else
 		args.name[0] = '\0';

 	if (command == BTRFS_IOC_SNAP_CREATE) {
diff --git a/btrfslabel.c b/btrfslabel.c
index c9f4684..da694e1 100644
--- a/btrfslabel.c
+++ b/btrfslabel.c
@@ -58,6 +58,7 @@ static void change_label_unmounted(char *dev, char *nLabel)

        trans = btrfs_start_transaction(root, 1);
        strncpy(root->fs_info->super_copy.label, nLabel, BTRFS_LABEL_SIZE);
+       root->fs_info->super_copy.label[BTRFS_LABEL_SIZE-1] = 0;
        btrfs_commit_transaction(trans, root);

        /* Now we close it since we are done. */
diff --git a/cmds-device.c b/cmds-device.c
index db625a6..771856b 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -117,6 +117,7 @@ static int cmd_add_dev(int argc, char **argv)
 		close(devfd);

 		strncpy(ioctl_args.name, argv[i], BTRFS_PATH_NAME_MAX);
+		ioctl_args.name[BTRFS_PATH_NAME_MAX-1] = 0;
 		res = ioctl(fdmnt, BTRFS_IOC_ADD_DEV, &ioctl_args);
 		e = errno;
 		if(res<0){
@@ -161,6 +162,7 @@ static int cmd_rm_dev(int argc, char **argv)
 		int	res;

 		strncpy(arg.name, argv[i], BTRFS_PATH_NAME_MAX);
+		arg.name[BTRFS_PATH_NAME_MAX-1] = 0;
 		res = ioctl(fdmnt, BTRFS_IOC_RM_DEV, &arg);
 		e = errno;
 		if(res<0){
@@ -226,6 +228,7 @@ static int cmd_scan_dev(int argc, char **argv)
 		printf("Scanning for Btrfs filesystems in '%s'\n", argv[i]);

 		strncpy(args.name, argv[i], BTRFS_PATH_NAME_MAX);
+		args.name[BTRFS_PATH_NAME_MAX-1] = 0;
 		/*
 		 * FIXME: which are the error code returned by this ioctl ?
 		 * it seems that is impossible to understand if there no is
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 1f53d1c..ea9e788 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -489,6 +489,7 @@ static int cmd_resize(int argc, char **argv)

 	printf("Resize '%s' of '%s'\n", path, amount);
 	strncpy(args.name, amount, BTRFS_PATH_NAME_MAX);
+	args.name[BTRFS_PATH_NAME_MAX-1] = 0;
 	res = ioctl(fd, BTRFS_IOC_RESIZE, &args);
 	e = errno;
 	close(fd);
diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 950fa8f..fc749f1 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -111,6 +111,7 @@ static int cmd_subvol_create(int argc, char **argv)

 	printf("Create subvolume '%s/%s'\n", dstdir, newname);
 	strncpy(args.name, newname, BTRFS_PATH_NAME_MAX);
+	args.name[BTRFS_PATH_NAME_MAX-1] = 0;
 	res = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE, &args);
 	e = errno;

@@ -202,6 +203,7 @@ static int cmd_subvol_delete(int argc, char **argv)

 	printf("Delete subvolume '%s/%s'\n", dname, vname);
 	strncpy(args.name, vname, BTRFS_PATH_NAME_MAX);
+	args.name[BTRFS_PATH_NAME_MAX-1] = 0;
 	res = ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &args);
 	e = errno;

@@ -378,6 +380,7 @@ static int cmd_snapshot(int argc, char **argv)

 	args.fd = fd;
 	strncpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX);
+	args.name[BTRFS_PATH_NAME_MAX-1] = 0;
 	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
 	e = errno;

diff --git a/restore.c b/restore.c
index 2674832..d1ac542 100644
--- a/restore.c
+++ b/restore.c
@@ -846,7 +846,8 @@ int main(int argc, char **argv)

 	memset(path_name, 0, 4096);

-	strncpy(dir_name, argv[optind + 1], 128);
+	strncpy(dir_name, argv[optind + 1], sizeof dir_name);
+	dir_name[sizeof dir_name - 1] = 0;

 	/* Strip the trailing / on the dir name */
 	len = strlen(dir_name);
diff --git a/utils.c b/utils.c
index ee7fa1b..5240c2c 100644
--- a/utils.c
+++ b/utils.c
@@ -657,9 +657,11 @@ int resolve_loop_device(const char* loop_dev, char* loop_file, int max_len)
 	ret_ioctl = ioctl(loop_fd, LOOP_GET_STATUS, &loopinfo);
 	close(loop_fd);

-	if (ret_ioctl == 0)
+	if (ret_ioctl == 0) {
 		strncpy(loop_file, loopinfo.lo_name, max_len);
-	else
+		if (max_len > 0)
+			loop_file[max_len-1] = 0;
+	} else
 		return -errno;

 	return 0;
@@ -860,8 +862,10 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 	}

 	/* Did we find an entry in mnt table? */
-	if (mnt && size && where)
+	if (mnt && size && where) {
 		strncpy(where, mnt->mnt_dir, size);
+		where[size-1] = 0;
+	}
 	if (fs_dev_ret)
 		*fs_dev_ret = fs_devices_mnt;

@@ -893,6 +897,8 @@ int get_mountpt(char *dev, char *mntpt, size_t size)
                if (strcmp(dev, mnt->mnt_fsname) == 0)
                {
                        strncpy(mntpt, mnt->mnt_dir, size);
+                       if (size)
+                                mntpt[size-1] = 0;
                        break;
                }
        }
@@ -925,6 +931,7 @@ void btrfs_register_one_device(char *fname)
 		return;
 	}
 	strncpy(args.name, fname, BTRFS_PATH_NAME_MAX);
+	args.name[BTRFS_PATH_NAME_MAX-1] = 0;
 	ret = ioctl(fd, BTRFS_IOC_SCAN_DEV, &args);
 	e = errno;
 	if(ret<0){
-- 
1.7.10.208.gb4267

Re: [PATCHv2 3/4] avoid several strncpy-induced buffer overruns

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 8077 bytes --]

   Hi, Jim, 

   I picked this up to go in integration-*, and Alex Block spotted a
problem, so I did a bit of a more in-depth review. Comments below.

On Fri, Apr 20, 2012 at 09:27:25PM +0200, Jim Meyering wrote:
> From: Jim Meyering <meyering@redhat.com>
> 
> * restore.c (main): Ensure strncpy-copied dir_name is NUL-terminated.
> * btrfsctl.c (main): Likewise, for a command-line argument.
> * utils.c (multiple functions): Likewise.
> * btrfs-list.c (add_root): Likewise.
> * btrfslabel.c (change_label_unmounted): Likewise.
> * cmds-device.c (cmd_add_dev, cmd_rm_dev, cmd_scan_dev): Likewise.
> * cmds-filesystem.c (cmd_resize): Likewise.
> * cmds-subvolume.c (cmd_subvol_create, cmd_subvol_delete, cmd_snapshot):
> Likewise.
> 
> Reviewed-by: Josef Bacik <josef@redhat.com>
> ---
>  btrfs-list.c      |    2 ++
>  btrfsctl.c        |    5 +++--
>  btrfslabel.c      |    1 +
>  cmds-device.c     |    3 +++
>  cmds-filesystem.c |    1 +
>  cmds-subvolume.c  |    3 +++
>  restore.c         |    3 ++-
>  utils.c           |   13 ++++++++++---
>  8 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/btrfs-list.c b/btrfs-list.c
> index 5f4a9be..35e6139 100644
> --- a/btrfs-list.c
> +++ b/btrfs-list.c
> @@ -183,6 +183,8 @@ static int add_root(struct root_lookup *root_lookup,
>  	ri->root_id = root_id;
>  	ri->ref_tree = ref_tree;
>  	strncpy(ri->name, name, name_len);
> +	if (name_len > 0)
> +		ri->name[name_len-1] = 0;

   Alex Block pointed out on IRC that this breaks "btrfs sub list", by
chopping off the last character of the subvol name. This should
probably be:

- 	strncpy(ri->name, name, name_len);
+ 	strncpy(ri->name, name, name_len+1);
+	if (name_len > 0)
+		ri->name[name_len] = 0;

>  	ret = tree_insert(&root_lookup->root, root_id, ref_tree, &ri->rb_node);
>  	if (ret) {
> diff --git a/btrfsctl.c b/btrfsctl.c
> index d45e2a7..518684c 100644
> --- a/btrfsctl.c
> +++ b/btrfsctl.c
> @@ -241,9 +241,10 @@ int main(int ac, char **av)
>  		fd = open_file_or_dir(fname);
>  	 }
> 
> -	if (name)
> +	if (name) {
>                  strncpy(args.name, name, BTRFS_PATH_NAME_MAX + 1);
> -	else
> +                args.name[BTRFS_PATH_NAME_MAX] = 0;
> +	} else
>  		args.name[0] = '\0';
> 
>  	if (command == BTRFS_IOC_SNAP_CREATE) {
> diff --git a/btrfslabel.c b/btrfslabel.c
> index c9f4684..da694e1 100644
> --- a/btrfslabel.c
> +++ b/btrfslabel.c
> @@ -58,6 +58,7 @@ static void change_label_unmounted(char *dev, char *nLabel)
> 
>         trans = btrfs_start_transaction(root, 1);
>         strncpy(root->fs_info->super_copy.label, nLabel, BTRFS_LABEL_SIZE);
> +       root->fs_info->super_copy.label[BTRFS_LABEL_SIZE-1] = 0;
>         btrfs_commit_transaction(trans, root);
> 
>         /* Now we close it since we are done. */
> diff --git a/cmds-device.c b/cmds-device.c
> index db625a6..771856b 100644
> --- a/cmds-device.c
> +++ b/cmds-device.c
> @@ -117,6 +117,7 @@ static int cmd_add_dev(int argc, char **argv)
>  		close(devfd);
> 
>  		strncpy(ioctl_args.name, argv[i], BTRFS_PATH_NAME_MAX);

   Actually, this could go up to BTRFS_PATH_NAME_MAX+1,

> +		ioctl_args.name[BTRFS_PATH_NAME_MAX-1] = 0;

and this to BTRFS_PATH_NAME_MAX, since struct btrfs_ioctl_vol_args'
name member is BTRFS_PATH_NAME_MAX+1 in size...

>  		res = ioctl(fdmnt, BTRFS_IOC_ADD_DEV, &ioctl_args);
>  		e = errno;
>  		if(res<0){
> @@ -161,6 +162,7 @@ static int cmd_rm_dev(int argc, char **argv)
>  		int	res;
> 
>  		strncpy(arg.name, argv[i], BTRFS_PATH_NAME_MAX);
> +		arg.name[BTRFS_PATH_NAME_MAX-1] = 0;

... and here...

>  		res = ioctl(fdmnt, BTRFS_IOC_RM_DEV, &arg);
>  		e = errno;
>  		if(res<0){
> @@ -226,6 +228,7 @@ static int cmd_scan_dev(int argc, char **argv)
>  		printf("Scanning for Btrfs filesystems in '%s'\n", argv[i]);
> 
>  		strncpy(args.name, argv[i], BTRFS_PATH_NAME_MAX);
> +		args.name[BTRFS_PATH_NAME_MAX-1] = 0;

... and here, and the next 3 hunks as well.

>  		/*
>  		 * FIXME: which are the error code returned by this ioctl ?
>  		 * it seems that is impossible to understand if there no is
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 1f53d1c..ea9e788 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -489,6 +489,7 @@ static int cmd_resize(int argc, char **argv)
> 
>  	printf("Resize '%s' of '%s'\n", path, amount);
>  	strncpy(args.name, amount, BTRFS_PATH_NAME_MAX);
> +	args.name[BTRFS_PATH_NAME_MAX-1] = 0;
>  	res = ioctl(fd, BTRFS_IOC_RESIZE, &args);
>  	e = errno;
>  	close(fd);
> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> index 950fa8f..fc749f1 100644
> --- a/cmds-subvolume.c
> +++ b/cmds-subvolume.c
> @@ -111,6 +111,7 @@ static int cmd_subvol_create(int argc, char **argv)
> 
>  	printf("Create subvolume '%s/%s'\n", dstdir, newname);
>  	strncpy(args.name, newname, BTRFS_PATH_NAME_MAX);
> +	args.name[BTRFS_PATH_NAME_MAX-1] = 0;
>  	res = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE, &args);
>  	e = errno;
> 
> @@ -202,6 +203,7 @@ static int cmd_subvol_delete(int argc, char **argv)
> 
>  	printf("Delete subvolume '%s/%s'\n", dname, vname);
>  	strncpy(args.name, vname, BTRFS_PATH_NAME_MAX);
> +	args.name[BTRFS_PATH_NAME_MAX-1] = 0;
>  	res = ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &args);
>  	e = errno;
> 
> @@ -378,6 +380,7 @@ static int cmd_snapshot(int argc, char **argv)
> 
>  	args.fd = fd;
>  	strncpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX);
> +	args.name[BTRFS_PATH_NAME_MAX-1] = 0;

   This, however, is wrong. args here is a struct
btrfs_ioctl_vol_args_v2, and the name field is BTRFS_SUBVOL_NAME_MAX+1
long, so it should be:

-  	strncpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX);
+  	strncpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX+1);
+	args.name[BTRFS_SUBVOL_NAME_MAX] = 0;

>  	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
>  	e = errno;
> 
> diff --git a/restore.c b/restore.c
> index 2674832..d1ac542 100644
> --- a/restore.c
> +++ b/restore.c
> @@ -846,7 +846,8 @@ int main(int argc, char **argv)
> 
>  	memset(path_name, 0, 4096);
> 
> -	strncpy(dir_name, argv[optind + 1], 128);
> +	strncpy(dir_name, argv[optind + 1], sizeof dir_name);
> +	dir_name[sizeof dir_name - 1] = 0;
> 
>  	/* Strip the trailing / on the dir name */
>  	len = strlen(dir_name);
> diff --git a/utils.c b/utils.c
> index ee7fa1b..5240c2c 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -657,9 +657,11 @@ int resolve_loop_device(const char* loop_dev, char* loop_file, int max_len)
>  	ret_ioctl = ioctl(loop_fd, LOOP_GET_STATUS, &loopinfo);
>  	close(loop_fd);
> 
> -	if (ret_ioctl == 0)
> +	if (ret_ioctl == 0) {
>  		strncpy(loop_file, loopinfo.lo_name, max_len);
> -	else
> +		if (max_len > 0)
> +			loop_file[max_len-1] = 0;
> +	} else
>  		return -errno;
> 
>  	return 0;
> @@ -860,8 +862,10 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
>  	}
> 
>  	/* Did we find an entry in mnt table? */
> -	if (mnt && size && where)
> +	if (mnt && size && where) {
>  		strncpy(where, mnt->mnt_dir, size);
> +		where[size-1] = 0;
> +	}
>  	if (fs_dev_ret)
>  		*fs_dev_ret = fs_devices_mnt;
> 
> @@ -893,6 +897,8 @@ int get_mountpt(char *dev, char *mntpt, size_t size)
>                 if (strcmp(dev, mnt->mnt_fsname) == 0)
>                 {
>                         strncpy(mntpt, mnt->mnt_dir, size);
> +                       if (size)
> +                                mntpt[size-1] = 0;
>                         break;
>                 }
>         }
> @@ -925,6 +931,7 @@ void btrfs_register_one_device(char *fname)
>  		return;
>  	}
>  	strncpy(args.name, fname, BTRFS_PATH_NAME_MAX);
> +	args.name[BTRFS_PATH_NAME_MAX-1] = 0;

   Same comment about the length of the name field in struct
btrfs_ioctl_vol_args as the 6 or 7 places above.

>  	ret = ioctl(fd, BTRFS_IOC_SCAN_DEV, &args);
>  	e = errno;
>  	if(ret<0){

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
      --- Your problem is that you've got too much taste to be ---       
                            a web developer.                             

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCHv2 3/4] avoid several strncpy-induced buffer overruns

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

On Wed, Jun 06, 2012 at 08:31:47PM +0100, Hugo Mills wrote:
> > @@ -378,6 +380,7 @@ static int cmd_snapshot(int argc, char **argv)
> > 
> >  	args.fd = fd;
> >  	strncpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX);
> > +	args.name[BTRFS_PATH_NAME_MAX-1] = 0;
> 
>    This, however, is wrong. args here is a struct
> btrfs_ioctl_vol_args_v2, and the name field is BTRFS_SUBVOL_NAME_MAX+1
> long, so it should be:
> 
> -  	strncpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX);
> +  	strncpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX+1);
> +	args.name[BTRFS_SUBVOL_NAME_MAX] = 0;

   Oops, just spotted the v3 with this fix in. Ignore this comment.
(I'm actually using the v3 in integration, but I reviewed the mail
from a different mailbox and got the wrong series...)

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
      --- Your problem is that you've got too much taste to be ---       
                            a web developer.                             

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCHv2 3/4] avoid several strncpy-induced buffer overruns

[Jim Meyering]

Hugo Mills wrote:
>    Hi, Jim,
>
>    I picked this up to go in integration-*, and Alex Block spotted a
> problem, so I did a bit of a more in-depth review. Comments below.
>
> On Fri, Apr 20, 2012 at 09:27:25PM +0200, Jim Meyering wrote:
>> From: Jim Meyering <meyering@redhat.com>
>>
>> * restore.c (main): Ensure strncpy-copied dir_name is NUL-terminated.
>> * btrfsctl.c (main): Likewise, for a command-line argument.
>> * utils.c (multiple functions): Likewise.
>> * btrfs-list.c (add_root): Likewise.
>> * btrfslabel.c (change_label_unmounted): Likewise.
>> * cmds-device.c (cmd_add_dev, cmd_rm_dev, cmd_scan_dev): Likewise.
>> * cmds-filesystem.c (cmd_resize): Likewise.
>> * cmds-subvolume.c (cmd_subvol_create, cmd_subvol_delete, cmd_snapshot):
>> Likewise.
>>
>> Reviewed-by: Josef Bacik <josef@redhat.com>
>> ---
>>  btrfs-list.c      |    2 ++
>>  btrfsctl.c        |    5 +++--
>>  btrfslabel.c      |    1 +
>>  cmds-device.c     |    3 +++
>>  cmds-filesystem.c |    1 +
>>  cmds-subvolume.c  |    3 +++
>>  restore.c         |    3 ++-
>>  utils.c           |   13 ++++++++++---
>>  8 files changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/btrfs-list.c b/btrfs-list.c
>> index 5f4a9be..35e6139 100644
>> --- a/btrfs-list.c
>> +++ b/btrfs-list.c
>> @@ -183,6 +183,8 @@ static int add_root(struct root_lookup *root_lookup,
>>  	ri->root_id = root_id;
>>  	ri->ref_tree = ref_tree;
>>  	strncpy(ri->name, name, name_len);
>> +	if (name_len > 0)
>> +		ri->name[name_len-1] = 0;
>
>    Alex Block pointed out on IRC that this breaks "btrfs sub list", by
> chopping off the last character of the subvol name. This should
> probably be:
>
> - 	strncpy(ri->name, name, name_len);
> + 	strncpy(ri->name, name, name_len+1);
> +	if (name_len > 0)
> +		ri->name[name_len] = 0;

Hi Hugo,

Thanks for the feedback.
I'll start with this one.  More tomorrow.

Upon re-review, I see that the preceding code does this:

	struct root_info *ri;
	struct rb_node *ret;
	ri = malloc(sizeof(*ri) + name_len + 1);
	if (!ri) {
		printf("memory allocation failed\n");
		exit(1);
	}
	memset(ri, 0, sizeof(*ri) + name_len + 1);

Since that memset has already guaranteed that the final byte
of ri->name is a NUL, the original strncpy is fine, and I should
not have tried to NUL-terminate its buffer:

    strncpy(ri->name, name, name_len);

I.e., it does not need to go to any extra lengths to NUL-terminate.
Hence, we could just drop that hunk.  However, depending on that
memset[*] makes this code unnecessarily fragile, and using strncpy
at all is generally best avoided, and most importantly, since
I doubt that it is ok to use a truncated "name" in ri->name if
strlen(name) ever happens to be longer than name_len, ...

What would you think of a patch to reject a name (currently
merely truncated) that would be too long?  If somehow that
truncation is not possible, then there is no need for strncpy
in the first place.

[*] I would remove the memset-0 altogether, if possible, or at least
fold it into the preceding malloc (i.e., s/malloc/calloc/).

[PATCHv4 3/4] avoid several strncpy-induced buffer overruns

[Jim Meyering]

Hugo Mills wrote:
>    Hi, Jim,
>
>    I picked this up to go in integration-*, and Alex Block spotted a
> problem, so I did a bit of a more in-depth review. Comments below.

Hi Hugo,

Thanks again for the review feedback.
...
TL;DR: the length of btrfs_ioctl_vol_args.name depends on #ifdef __CHECKER__
Patch at the end.
-----------------------------------------------------------------

>>  	strncpy(ri->name, name, name_len);
>> +	if (name_len > 0)
>> +		ri->name[name_len-1] = 0;
>
>    Alex Block pointed out on IRC that this breaks "btrfs sub list", by
> chopping off the last character of the subvol name. This should
> probably be:
>
> - 	strncpy(ri->name, name, name_len);
> + 	strncpy(ri->name, name, name_len+1);
> +	if (name_len > 0)
> +		ri->name[name_len] = 0;
>
>>  	ret = tree_insert(&root_lookup->root, root_id, ref_tree, &ri->rb_node);
>>  	if (ret) {
>> diff --git a/btrfsctl.c b/btrfsctl.c
>> index d45e2a7..518684c 100644
>> --- a/btrfsctl.c
>> +++ b/btrfsctl.c
>> @@ -241,9 +241,10 @@ int main(int ac, char **av)
>>  		fd = open_file_or_dir(fname);
>>  	 }
>>
>> -	if (name)
>> +	if (name) {
>>                  strncpy(args.name, name, BTRFS_PATH_NAME_MAX + 1);
>> -	else
>> +                args.name[BTRFS_PATH_NAME_MAX] = 0;
>> +	} else
>>  		args.name[0] = '\0';
>>
>>  	if (command == BTRFS_IOC_SNAP_CREATE) {
>> diff --git a/btrfslabel.c b/btrfslabel.c
>> index c9f4684..da694e1 100644
>> --- a/btrfslabel.c
>> +++ b/btrfslabel.c
>> @@ -58,6 +58,7 @@ static void change_label_unmounted(char *dev, char *nLabel)
>>
>>         trans = btrfs_start_transaction(root, 1);
>>         strncpy(root->fs_info->super_copy.label, nLabel, BTRFS_LABEL_SIZE);
>> +       root->fs_info->super_copy.label[BTRFS_LABEL_SIZE-1] = 0;
>>         btrfs_commit_transaction(trans, root);
>>
>>         /* Now we close it since we are done. */
>> diff --git a/cmds-device.c b/cmds-device.c
>> index db625a6..771856b 100644
>> --- a/cmds-device.c
>> +++ b/cmds-device.c
>> @@ -117,6 +117,7 @@ static int cmd_add_dev(int argc, char **argv)
>>  		close(devfd);
>>
>>  		strncpy(ioctl_args.name, argv[i], BTRFS_PATH_NAME_MAX);
>
>    Actually, this could go up to BTRFS_PATH_NAME_MAX+1,

There's no point in copying or clearing the last byte
when the next statement will clear it, so I've left the above as-is.

>> +		ioctl_args.name[BTRFS_PATH_NAME_MAX-1] = 0;

In the patch below I've adjusted it to clear the byte at the end of the
struct defined in ioctl.h, assuming that the 1-byte-shorter
__CHECKER__-defined struct declarations (see below) are erroneous.

As I write, I realize that I would prefer this instead:

		ioctl_args.name[sizeof(ioctl_args.name)-1] = 0;

since this code is independent of whether __CHECKER__ is defined.
Say the word and I'll be happy to provide a v5.

> and this to BTRFS_PATH_NAME_MAX, since struct btrfs_ioctl_vol_args'
> name member is BTRFS_PATH_NAME_MAX+1 in size...

However, using an index of BTRFS_PATH_NAME_MAX with
struct btrfs_ioctl_vol_args would cause its own overrun whenever
__CHECKER__ is defined, due to these:

    $ git grep 'btrfs_ioctl_vol_args.*MAX'
    btrfs-vol.c:struct btrfs_ioctl_vol_args { char name[BTRFS_VOL_NAME_MAX]; };
    btrfsctl.c:struct btrfs_ioctl_vol_args { char name[BTRFS_VOL_NAME_MAX]; };
    cmds-device.c:struct btrfs_ioctl_vol_args { char name[BTRFS_VOL_NAME_MAX]; };

E.g.,

  #ifdef __CHECKER__
  #define BLKGETSIZE64 0
  #define BTRFS_IOC_SNAP_CREATE 0
  #define BTRFS_IOC_ADD_DEV 0
  #define BTRFS_IOC_RM_DEV 0
  #define BTRFS_VOL_NAME_MAX 255
  struct btrfs_ioctl_vol_args { char name[BTRFS_VOL_NAME_MAX]; };
  static inline int ioctl(int fd, int define, void *arg) { return 0; }
  #endif

If those "name" buffers are not deliberately one byte shorter
than the real ones in ioctl.h, let me know and I'll adjust them
in a precursor patch.

>>  		res = ioctl(fdmnt, BTRFS_IOC_ADD_DEV, &ioctl_args);
>>  		e = errno;
>>  		if(res<0){
>> @@ -161,6 +162,7 @@ static int cmd_rm_dev(int argc, char **argv)
>>  		int	res;
>>
>>  		strncpy(arg.name, argv[i], BTRFS_PATH_NAME_MAX);
>> +		arg.name[BTRFS_PATH_NAME_MAX-1] = 0;
>
> ... and here...
>
>>  		res = ioctl(fdmnt, BTRFS_IOC_RM_DEV, &arg);
>>  		e = errno;
>>  		if(res<0){
>> @@ -226,6 +228,7 @@ static int cmd_scan_dev(int argc, char **argv)
>>  		printf("Scanning for Btrfs filesystems in '%s'\n", argv[i]);
>>
>>  		strncpy(args.name, argv[i], BTRFS_PATH_NAME_MAX);
>> +		args.name[BTRFS_PATH_NAME_MAX-1] = 0;
>
> ... and here, and the next 3 hunks as well.
...
>> +	args.name[BTRFS_PATH_NAME_MAX-1] = 0;
>
>    This, however, is wrong. args here is a struct
> btrfs_ioctl_vol_args_v2, and the name field is BTRFS_SUBVOL_NAME_MAX+1
> long, so it should be:
>
> -  	strncpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX);
> +  	strncpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX+1);
> +	args.name[BTRFS_SUBVOL_NAME_MAX] = 0;

As you noted, later, that was fixed in a subsequent round.

Here's a complete version of that adjusted patch:

>From c2026ca5fe3ffe90f2ddc0f4f6332922c4f33cb0 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Mon, 16 Apr 2012 22:03:38 +0200
Subject: [PATCHv4 3/4] avoid several strncpy-induced buffer overruns

* btrfs-list.c (add_root): Use calloc in place of malloc+memset-0
* restore.c (main): Ensure strncpy-copied dir_name is NUL-terminated.
* btrfsctl.c (main): Likewise, for a command-line argument.
* utils.c (multiple functions): Likewise.
* btrfslabel.c (change_label_unmounted): Likewise.
* cmds-device.c (cmd_add_dev, cmd_rm_dev, cmd_scan_dev): Likewise.
* cmds-filesystem.c (cmd_resize): Likewise.
* cmds-subvolume.c (cmd_subvol_create, cmd_subvol_delete, cmd_snapshot):
Likewise.

Reviewed-by: Josef Bacik <josef@redhat.com>

.
---
 btrfs-list.c      |  3 +--
 btrfsctl.c        |  5 +++--
 btrfslabel.c      |  1 +
 cmds-device.c     |  3 +++
 cmds-filesystem.c |  1 +
 cmds-subvolume.c  |  3 +++
 restore.c         |  3 ++-
 utils.c           | 13 ++++++++++---
 8 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/btrfs-list.c b/btrfs-list.c
index 5f4a9be..da43eff 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -172,12 +172,11 @@ static int add_root(struct root_lookup *root_lookup,
 {
 	struct root_info *ri;
 	struct rb_node *ret;
-	ri = malloc(sizeof(*ri) + name_len + 1);
+	ri = calloc(sizeof(*ri) + name_len + 1);
 	if (!ri) {
 		printf("memory allocation failed\n");
 		exit(1);
 	}
-	memset(ri, 0, sizeof(*ri) + name_len + 1);
 	ri->path = NULL;
 	ri->dir_id = dir_id;
 	ri->root_id = root_id;
diff --git a/btrfsctl.c b/btrfsctl.c
index d45e2a7..518684c 100644
--- a/btrfsctl.c
+++ b/btrfsctl.c
@@ -241,9 +241,10 @@ int main(int ac, char **av)
 		fd = open_file_or_dir(fname);
 	 }

-	if (name)
+	if (name) {
                 strncpy(args.name, name, BTRFS_PATH_NAME_MAX + 1);
-	else
+                args.name[BTRFS_PATH_NAME_MAX] = 0;
+	} else
 		args.name[0] = '\0';

 	if (command == BTRFS_IOC_SNAP_CREATE) {
diff --git a/btrfslabel.c b/btrfslabel.c
index c9f4684..da694e1 100644
--- a/btrfslabel.c
+++ b/btrfslabel.c
@@ -58,6 +58,7 @@ static void change_label_unmounted(char *dev, char *nLabel)

        trans = btrfs_start_transaction(root, 1);
        strncpy(root->fs_info->super_copy.label, nLabel, BTRFS_LABEL_SIZE);
+       root->fs_info->super_copy.label[BTRFS_LABEL_SIZE-1] = 0;
        btrfs_commit_transaction(trans, root);

        /* Now we close it since we are done. */
diff --git a/cmds-device.c b/cmds-device.c
index db625a6..0e3e38c 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -117,6 +117,7 @@ static int cmd_add_dev(int argc, char **argv)
 		close(devfd);

 		strncpy(ioctl_args.name, argv[i], BTRFS_PATH_NAME_MAX);
+		ioctl_args.name[BTRFS_PATH_NAME_MAX] = 0;
 		res = ioctl(fdmnt, BTRFS_IOC_ADD_DEV, &ioctl_args);
 		e = errno;
 		if(res<0){
@@ -161,6 +162,7 @@ static int cmd_rm_dev(int argc, char **argv)
 		int	res;

 		strncpy(arg.name, argv[i], BTRFS_PATH_NAME_MAX);
+		arg.name[BTRFS_PATH_NAME_MAX] = 0;
 		res = ioctl(fdmnt, BTRFS_IOC_RM_DEV, &arg);
 		e = errno;
 		if(res<0){
@@ -226,6 +228,7 @@ static int cmd_scan_dev(int argc, char **argv)
 		printf("Scanning for Btrfs filesystems in '%s'\n", argv[i]);

 		strncpy(args.name, argv[i], BTRFS_PATH_NAME_MAX);
+		args.name[BTRFS_PATH_NAME_MAX] = 0;
 		/*
 		 * FIXME: which are the error code returned by this ioctl ?
 		 * it seems that is impossible to understand if there no is
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 1f53d1c..b6ab2b3 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -489,6 +489,7 @@ static int cmd_resize(int argc, char **argv)

 	printf("Resize '%s' of '%s'\n", path, amount);
 	strncpy(args.name, amount, BTRFS_PATH_NAME_MAX);
+	args.name[BTRFS_PATH_NAME_MAX] = 0;
 	res = ioctl(fd, BTRFS_IOC_RESIZE, &args);
 	e = errno;
 	close(fd);
diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 950fa8f..47a69c3 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -111,6 +111,7 @@ static int cmd_subvol_create(int argc, char **argv)

 	printf("Create subvolume '%s/%s'\n", dstdir, newname);
 	strncpy(args.name, newname, BTRFS_PATH_NAME_MAX);
+	args.name[BTRFS_PATH_NAME_MAX] = 0;
 	res = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE, &args);
 	e = errno;

@@ -202,6 +203,7 @@ static int cmd_subvol_delete(int argc, char **argv)

 	printf("Delete subvolume '%s/%s'\n", dname, vname);
 	strncpy(args.name, vname, BTRFS_PATH_NAME_MAX);
+	args.name[BTRFS_PATH_NAME_MAX] = 0;
 	res = ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &args);
 	e = errno;

@@ -378,6 +380,7 @@ static int cmd_snapshot(int argc, char **argv)

 	args.fd = fd;
 	strncpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX);
+	args.name[BTRFS_SUBVOL_NAME_MAX] = 0;
 	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
 	e = errno;

diff --git a/restore.c b/restore.c
index 2674832..d1ac542 100644
--- a/restore.c
+++ b/restore.c
@@ -846,7 +846,8 @@ int main(int argc, char **argv)

 	memset(path_name, 0, 4096);

-	strncpy(dir_name, argv[optind + 1], 128);
+	strncpy(dir_name, argv[optind + 1], sizeof dir_name);
+	dir_name[sizeof dir_name - 1] = 0;

 	/* Strip the trailing / on the dir name */
 	len = strlen(dir_name);
diff --git a/utils.c b/utils.c
index ee7fa1b..5908715 100644
--- a/utils.c
+++ b/utils.c
@@ -657,9 +657,11 @@ int resolve_loop_device(const char* loop_dev, char* loop_file, int max_len)
 	ret_ioctl = ioctl(loop_fd, LOOP_GET_STATUS, &loopinfo);
 	close(loop_fd);

-	if (ret_ioctl == 0)
+	if (ret_ioctl == 0) {
 		strncpy(loop_file, loopinfo.lo_name, max_len);
-	else
+		if (max_len > 0)
+			loop_file[max_len-1] = 0;
+	} else
 		return -errno;

 	return 0;
@@ -860,8 +862,10 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 	}

 	/* Did we find an entry in mnt table? */
-	if (mnt && size && where)
+	if (mnt && size && where) {
 		strncpy(where, mnt->mnt_dir, size);
+		where[size-1] = 0;
+	}
 	if (fs_dev_ret)
 		*fs_dev_ret = fs_devices_mnt;

@@ -893,6 +897,8 @@ int get_mountpt(char *dev, char *mntpt, size_t size)
                if (strcmp(dev, mnt->mnt_fsname) == 0)
                {
                        strncpy(mntpt, mnt->mnt_dir, size);
+                       if (size)
+                                mntpt[size-1] = 0;
                        break;
                }
        }
@@ -925,6 +931,7 @@ void btrfs_register_one_device(char *fname)
 		return;
 	}
 	strncpy(args.name, fname, BTRFS_PATH_NAME_MAX);
+	args.name[BTRFS_PATH_NAME_MAX] = 0;
 	ret = ioctl(fd, BTRFS_IOC_SCAN_DEV, &args);
 	e = errno;
 	if(ret<0){
--
1.7.11.rc2

[PATCHv2 4/4] mkfs: avoid heap-buffer-read-underrun for zero-length "size" arg

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

* mkfs.c (parse_size): ./mkfs.btrfs -A '' would read and possibly
write the byte before beginning of strdup'd heap buffer.  All other
size-accepting options were similarly affected.

Reviewed-by: Josef Bacik <josef@redhat.com>
---
 cmds-subvolume.c |    2 +-
 mkfs.c           |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index fc749f1..a01c830 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -380,7 +380,7 @@ static int cmd_snapshot(int argc, char **argv)

 	args.fd = fd;
 	strncpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX);
-	args.name[BTRFS_PATH_NAME_MAX-1] = 0;
+	args.name[BTRFS_SUBVOL_NAME_MAX-1] = 0;
 	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
 	e = errno;

diff --git a/mkfs.c b/mkfs.c
index 03239fb..4aff2fd 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -63,7 +63,7 @@ static u64 parse_size(char *s)

 	s = strdup(s);

-	if (!isdigit(s[len - 1])) {
+	if (len && !isdigit(s[len - 1])) {
 		c = tolower(s[len - 1]);
 		switch (c) {
 		case 'g':
-- 
1.7.10.208.gb4267

Re: [PATCHv2 4/4] mkfs: avoid heap-buffer-read-underrun for zero-length "size" arg

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 1718 bytes --]

On Fri, Apr 20, 2012 at 09:27:26PM +0200, Jim Meyering wrote:
> From: Jim Meyering <meyering@redhat.com>
> 
> * mkfs.c (parse_size): ./mkfs.btrfs -A '' would read and possibly
> write the byte before beginning of strdup'd heap buffer.  All other
> size-accepting options were similarly affected.
> 
> Reviewed-by: Josef Bacik <josef@redhat.com>
> ---
>  cmds-subvolume.c |    2 +-
>  mkfs.c           |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> index fc749f1..a01c830 100644
> --- a/cmds-subvolume.c
> +++ b/cmds-subvolume.c
> @@ -380,7 +380,7 @@ static int cmd_snapshot(int argc, char **argv)
> 
>  	args.fd = fd;
>  	strncpy(args.name, newname, BTRFS_SUBVOL_NAME_MAX);
                                                     ^ +1

> -	args.name[BTRFS_PATH_NAME_MAX-1] = 0;
> +	args.name[BTRFS_SUBVOL_NAME_MAX-1] = 0;
 
   args.name[BTRFS_SUBVOL_NAME_MAX] = 0;

>  	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
>  	e = errno;
> 
> diff --git a/mkfs.c b/mkfs.c
> index 03239fb..4aff2fd 100644
> --- a/mkfs.c
> +++ b/mkfs.c
> @@ -63,7 +63,7 @@ static u64 parse_size(char *s)
> 
>  	s = strdup(s);
> 
> -	if (!isdigit(s[len - 1])) {
> +	if (len && !isdigit(s[len - 1])) {

   I think I'd prefer that len is a size_t, not an int here. (Or that
len is tested to be >0).

>  		c = tolower(s[len - 1]);
>  		switch (c) {
>  		case 'g':

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
      --- Your problem is that you've got too much taste to be ---       
                            a web developer.                             

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCHv2 4/4] mkfs: avoid heap-buffer-read-underrun for zero-length "size" arg

[Jim Meyering]

Hugo Mills wrote:
>> diff --git a/mkfs.c b/mkfs.c
>> index 03239fb..4aff2fd 100644
>> --- a/mkfs.c
>> +++ b/mkfs.c
>> @@ -63,7 +63,7 @@ static u64 parse_size(char *s)
>>
>>  	s = strdup(s);
>>
>> -	if (!isdigit(s[len - 1])) {
>> +	if (len && !isdigit(s[len - 1])) {
>
>    I think I'd prefer that len is a size_t, not an int here. (Or that
> len is tested to be >0).
>
>>  		c = tolower(s[len - 1]);
>>  		switch (c) {
>>  		case 'g':

IMHO, there is no contest.  The correct type is size_t.
If you'd like to include that type change along with the bug fix,
here's the patch:

(However, note that even a quick git grep 'int.*strlen' shows five
more instances of this technically-subject-to-overflow use of "int",
so it might be better to fix them all at once -- auditing to ensure
that no use requires the signed type, of course.

    btrfs-list.c:           int add_len = strlen(found->path);
    btrfs.c:        int len = strlen(argv0_buf);
    cmds-filesystem.c:      int len = strlen(s);
    mkfs.c: int len = strlen(input);
    utils.c:       int len = strlen(input);

)

>From 666892466e85c228f897104e5bced2c99d798302 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Tue, 17 Apr 2012 14:56:28 +0200
Subject: [PATCH] mkfs: avoid heap-buffer-read-underrun for zero-length "size" arg

* mkfs.c (parse_size): ./mkfs.btrfs -A '' would read and possibly
write the byte before beginning of strdup'd heap buffer.  All other
size-accepting options were similarly affected.

Reviewed-by: Josef Bacik <josef@redhat.com>
---
 mkfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mkfs.c b/mkfs.c
index 03239fb..017e317 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -56,14 +56,14 @@ struct directory_name_entry {

 static u64 parse_size(char *s)
 {
-	int len = strlen(s);
+	size_t len = strlen(s);
 	char c;
 	u64 mult = 1;
 	u64 ret;

 	s = strdup(s);

-	if (!isdigit(s[len - 1])) {
+	if (len && !isdigit(s[len - 1])) {
 		c = tolower(s[len - 1]);
 		switch (c) {
 		case 'g':
--
1.7.11.rc1