* 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
* [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
* [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
* [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 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
* 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
* 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
* 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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.