* btrfs-utils: minor buffer-overrun fixes
@ 2012-04-20 17:36 Jim Meyering
2012-04-20 17:36 ` [PATCH 1/4] mkfs: use strdup in place of strlen,malloc,strcpy sequence Jim Meyering
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Jim Meyering @ 2012-04-20 17:36 UTC (permalink / raw)
To: linux-btrfs
[PATCH 1/4] mkfs: use strdup in place of strlen,malloc,strcpy
[PATCH 2/4] restore: don't corrupt stack for a zero-length
[PATCH 3/4] avoid strncpy-induced buffer overrun
[PATCH 4/4] mkfs: avoid heap-buffer-read-underrun for zero-length
The first is just clean-up.
The other three fix one sort of corruption or another,
usually induced by an invalid input.
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/4] mkfs: use strdup in place of strlen,malloc,strcpy sequence 2012-04-20 17:36 btrfs-utils: minor buffer-overrun fixes Jim Meyering @ 2012-04-20 17:36 ` Jim Meyering 2012-04-20 18:36 ` Josef Bacik 2012-04-20 17:36 ` [PATCH 2/4] restore: don't corrupt stack for a zero-length command-line argument Jim Meyering ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Jim Meyering @ 2012-04-20 17:36 UTC (permalink / raw) To: linux-btrfs From: Jim Meyering <meyering@redhat.com> * mkfs.c (traverse_directory): No semantic change. --- 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] mkfs: use strdup in place of strlen,malloc,strcpy sequence 2012-04-20 17:36 ` [PATCH 1/4] mkfs: use strdup in place of strlen,malloc,strcpy sequence Jim Meyering @ 2012-04-20 18:36 ` Josef Bacik 0 siblings, 0 replies; 11+ messages in thread From: Josef Bacik @ 2012-04-20 18:36 UTC (permalink / raw) To: Jim Meyering; +Cc: linux-btrfs On Fri, Apr 20, 2012 at 07:36:45PM +0200, Jim Meyering wrote: > From: Jim Meyering <meyering@redhat.com> > > * mkfs.c (traverse_directory): No semantic change. > --- > 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 > Reviewed-by: Josef Bacik <josef@redhat.com> Thanks, Josef ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/4] restore: don't corrupt stack for a zero-length command-line argument 2012-04-20 17:36 btrfs-utils: minor buffer-overrun fixes Jim Meyering 2012-04-20 17:36 ` [PATCH 1/4] mkfs: use strdup in place of strlen,malloc,strcpy sequence Jim Meyering @ 2012-04-20 17:36 ` Jim Meyering 2012-04-20 18:37 ` Josef Bacik 2012-04-20 18:40 ` Josef Bacik 2012-04-20 17:36 ` [PATCH 3/4] avoid strncpy-induced buffer overrun Jim Meyering 2012-04-20 17:36 ` [PATCH 4/4] mkfs: avoid heap-buffer-read-underrun for zero-length "size" arg Jim Meyering 3 siblings, 2 replies; 11+ messages in thread From: Jim Meyering @ 2012-04-20 17:36 UTC (permalink / raw) To: linux-btrfs 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'. --- restore.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/restore.c b/restore.c index 250c9d3..f049105 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] restore: don't corrupt stack for a zero-length command-line argument 2012-04-20 17:36 ` [PATCH 2/4] restore: don't corrupt stack for a zero-length command-line argument Jim Meyering @ 2012-04-20 18:37 ` Josef Bacik 2012-04-20 18:40 ` Josef Bacik 1 sibling, 0 replies; 11+ messages in thread From: Josef Bacik @ 2012-04-20 18:37 UTC (permalink / raw) To: Jim Meyering; +Cc: linux-btrfs On Fri, Apr 20, 2012 at 07:36:46PM +0200, Jim Meyering wrote: > 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'. > --- > restore.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/restore.c b/restore.c > index 250c9d3..f049105 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) { Reviewed-by: Josef Bacik <josef@redhat.com> Thanks, Josef ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] restore: don't corrupt stack for a zero-length command-line argument 2012-04-20 17:36 ` [PATCH 2/4] restore: don't corrupt stack for a zero-length command-line argument Jim Meyering 2012-04-20 18:37 ` Josef Bacik @ 2012-04-20 18:40 ` Josef Bacik 1 sibling, 0 replies; 11+ messages in thread From: Josef Bacik @ 2012-04-20 18:40 UTC (permalink / raw) To: Jim Meyering; +Cc: linux-btrfs On Fri, Apr 20, 2012 at 07:36:46PM +0200, Jim Meyering wrote: > 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'. > --- > restore.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/restore.c b/restore.c > index 250c9d3..f049105 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'; > } Oops I didn't notice this until I was looking at patch 3, can you take out that extra ) here so the patch is bisectable. Thanks, Josef ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] avoid strncpy-induced buffer overrun 2012-04-20 17:36 btrfs-utils: minor buffer-overrun fixes Jim Meyering 2012-04-20 17:36 ` [PATCH 1/4] mkfs: use strdup in place of strlen,malloc,strcpy sequence Jim Meyering 2012-04-20 17:36 ` [PATCH 2/4] restore: don't corrupt stack for a zero-length command-line argument Jim Meyering @ 2012-04-20 17:36 ` Jim Meyering 2012-04-20 18:42 ` Josef Bacik 2012-04-20 17:36 ` [PATCH 4/4] mkfs: avoid heap-buffer-read-underrun for zero-length "size" arg Jim Meyering 3 siblings, 1 reply; 11+ messages in thread From: Jim Meyering @ 2012-04-20 17:36 UTC (permalink / raw) To: linux-btrfs 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. --- btrfs-list.c | 2 ++ btrfsctl.c | 5 +++-- btrfslabel.c | 1 + cmds-device.c | 3 +++ cmds-filesystem.c | 1 + cmds-subvolume.c | 3 +++ restore.c | 5 +++-- utils.c | 13 ++++++++++--- 8 files changed, 26 insertions(+), 7 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 f049105..d1ac542 100644 --- a/restore.c +++ b/restore.c @@ -846,11 +846,12 @@ 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); - while (len && dir_name[--len] == '/')) { + while (len && dir_name[--len] == '/') { dir_name[len] = '\0'; } 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] avoid strncpy-induced buffer overrun 2012-04-20 17:36 ` [PATCH 3/4] avoid strncpy-induced buffer overrun Jim Meyering @ 2012-04-20 18:42 ` Josef Bacik 2012-04-20 19:26 ` Jim Meyering 0 siblings, 1 reply; 11+ messages in thread From: Josef Bacik @ 2012-04-20 18:42 UTC (permalink / raw) To: Jim Meyering; +Cc: linux-btrfs On Fri, Apr 20, 2012 at 07:36:47PM +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. > --- > btrfs-list.c | 2 ++ > btrfsctl.c | 5 +++-- > btrfslabel.c | 1 + > cmds-device.c | 3 +++ > cmds-filesystem.c | 1 + > cmds-subvolume.c | 3 +++ > restore.c | 5 +++-- > utils.c | 13 ++++++++++--- > 8 files changed, 26 insertions(+), 7 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 f049105..d1ac542 100644 > --- a/restore.c > +++ b/restore.c > @@ -846,11 +846,12 @@ 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); > - while (len && dir_name[--len] == '/')) { > + while (len && dir_name[--len] == '/') { > dir_name[len] = '\0'; > } > > 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){ Once you fix that extra ) in the other patch you can add Reviewed-by: Josef Bacik <josef@redhat.com> Thanks, Josef ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] avoid strncpy-induced buffer overrun 2012-04-20 18:42 ` Josef Bacik @ 2012-04-20 19:26 ` Jim Meyering 0 siblings, 0 replies; 11+ messages in thread From: Jim Meyering @ 2012-04-20 19:26 UTC (permalink / raw) To: Josef Bacik; +Cc: linux-btrfs Josef Bacik wrote: > On Fri, Apr 20, 2012 at 07:36:47PM +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. ... >> 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; Hi Josef, Thanks for the reviews. I've moved the parenthesis-fix you noticed, and have just noticed that I used the wrong symbol name above. The following change is folded into the PATCHv2 I'm about to post: 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; ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/4] mkfs: avoid heap-buffer-read-underrun for zero-length "size" arg 2012-04-20 17:36 btrfs-utils: minor buffer-overrun fixes Jim Meyering ` (2 preceding siblings ...) 2012-04-20 17:36 ` [PATCH 3/4] avoid strncpy-induced buffer overrun Jim Meyering @ 2012-04-20 17:36 ` Jim Meyering 2012-04-20 18:41 ` Josef Bacik 3 siblings, 1 reply; 11+ messages in thread From: Jim Meyering @ 2012-04-20 17:36 UTC (permalink / raw) To: linux-btrfs 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. --- mkfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] mkfs: avoid heap-buffer-read-underrun for zero-length "size" arg 2012-04-20 17:36 ` [PATCH 4/4] mkfs: avoid heap-buffer-read-underrun for zero-length "size" arg Jim Meyering @ 2012-04-20 18:41 ` Josef Bacik 0 siblings, 0 replies; 11+ messages in thread From: Josef Bacik @ 2012-04-20 18:41 UTC (permalink / raw) To: Jim Meyering; +Cc: linux-btrfs On Fri, Apr 20, 2012 at 07:36:48PM +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. > --- > mkfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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': Reviewed-by: Josef Bacik <josef@redhat.com> Thanks, Josef ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-04-20 19:26 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-04-20 17:36 btrfs-utils: minor buffer-overrun fixes Jim Meyering 2012-04-20 17:36 ` [PATCH 1/4] mkfs: use strdup in place of strlen,malloc,strcpy sequence Jim Meyering 2012-04-20 18:36 ` Josef Bacik 2012-04-20 17:36 ` [PATCH 2/4] restore: don't corrupt stack for a zero-length command-line argument Jim Meyering 2012-04-20 18:37 ` Josef Bacik 2012-04-20 18:40 ` Josef Bacik 2012-04-20 17:36 ` [PATCH 3/4] avoid strncpy-induced buffer overrun Jim Meyering 2012-04-20 18:42 ` Josef Bacik 2012-04-20 19:26 ` Jim Meyering 2012-04-20 17:36 ` [PATCH 4/4] mkfs: avoid heap-buffer-read-underrun for zero-length "size" arg Jim Meyering 2012-04-20 18:41 ` Josef Bacik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).