Discussion:
[PATCH 0/4] some btrfs-progs coverity fixes
(too old to reply)
Zach Brown
2014-10-15 23:14:17 UTC
Permalink
Hi gang,

Here's another set of coverity fixes for btrfs-progs against David's
integration-20141007 branch.

I got tired of adding error checking after a few so I moved on to the
other warnings. Maybe we should subscribe linux-btrfs to the reports
that coverity can send out?

- z

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Zach Brown
2014-10-15 23:14:18 UTC
Permalink
coverity warned that the return code from sscanf() assigned to 'i'
wasn't checked before being assigned again. Check it.

Signed-off-by: Zach Brown <***@zabbo.net>
---
utils.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/utils.c b/utils.c
index c2f30d4..51e55be 100644
--- a/utils.c
+++ b/utils.c
@@ -1574,7 +1574,11 @@ scan_again:

strcpy(fullpath,"/dev/");
while(fgets(buf, 1023, proc_partitions)) {
- i = sscanf(buf," %*d %*d %*d %99s", fullpath+5);
+ ret = sscanf(buf," %*d %*d %*d %99s", fullpath+5);
+ if (ret != 1) {
+ fprintf(stderr, "failed to scan device name from /proc/partitions\n");
+ break;
+ }

/*
* multipath and MD devices may register as a btrfs filesystem
--
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Zach Brown
2014-10-15 23:14:19 UTC
Permalink
coverity barked out a warning that btrfs-map-logical was storing but
ignoring errors from read_extent_from_disk(). So don't ignore 'em. I
made extent reading errors fatal to match the fatal errors from mapping
mirrors above.

And while we're at it have read_extent_from_disk() return -errno pread
errors instead of -EIO or -1 (-EPERM). The only other caller who tests
errors clobbers them with -EIO.

Signed-off-by: Zach Brown <***@zabbo.net>
---
btrfs-map-logical.c | 12 +++++++++++-
extent_io.c | 4 +++-
2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/btrfs-map-logical.c b/btrfs-map-logical.c
index 6b475fc..47d1104 100644
--- a/btrfs-map-logical.c
+++ b/btrfs-map-logical.c
@@ -76,8 +76,18 @@ static struct extent_buffer * debug_read_block(struct btrfs_root *root,
(unsigned long long)eb->dev_bytenr, device->name);
kfree(multi);

- if (!copy || mirror_num == copy)
+ if (!copy || mirror_num == copy) {
ret = read_extent_from_disk(eb, 0, eb->len);
+ if (ret) {
+ fprintf(info_file,
+ "Error: failed to read extent: mirror %d logical %llu: %s\n",
+ mirror_num, (unsigned long long)eb->start,
+ strerror(-ret));
+ free_extent_buffer(eb);
+ eb = NULL;
+ break;
+ }
+ }

num_copies = btrfs_num_copies(&root->fs_info->mapping_tree,
eb->start, eb->len);
diff --git a/extent_io.c b/extent_io.c
index 425af8a..5186e89 100644
--- a/extent_io.c
+++ b/extent_io.c
@@ -647,8 +647,10 @@ int read_extent_from_disk(struct extent_buffer *eb,
{
int ret;
ret = pread(eb->fd, eb->data + offset, len, eb->dev_bytenr);
- if (ret < 0)
+ if (ret < 0) {
+ ret = -errno;
goto out;
+ }
if (ret != len) {
ret = -EIO;
goto out;
--
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Zach Brown
2014-10-15 23:14:20 UTC
Permalink
coverity pointed out that unknown flag printing in show super had some
dead code. It turns out that first was reset when the first flag was
tested, not when it was output. We only want to clear it if the first
matching bit is output. If there are no matching bits then we'll want
to output the unknown flag first.

Signed-off-by: Zach Brown <***@zabbo.net>
---
btrfs-show-super.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/btrfs-show-super.c b/btrfs-show-super.c
index 456dbd8..2b48f44 100644
--- a/btrfs-show-super.c
+++ b/btrfs-show-super.c
@@ -324,8 +324,8 @@ static void print_readable_incompat_flag(u64 flag)
printf("%s ", entry->output);
else
printf("|\n\t\t\t %s ", entry->output);
+ first = 0;
}
- first = 0;
}
flag &= ~BTRFS_FEATURE_INCOMPAT_SUPP;
if (flag) {
--
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Zach Brown
2014-10-15 23:14:21 UTC
Permalink
btrfs_setup_all_roots() had some copy and pasted code for trying to
setup a root and then creating a blank node if that failed. The copy
for the csum_root created the blank node in the extent_root.

So we create a function to use a consistent root.

Signed-off-by: Zach Brown <***@zabbo.net>
---
disk-io.c | 66 ++++++++++++++++++++++++++++++++++-----------------------------
1 file changed, 36 insertions(+), 30 deletions(-)

diff --git a/disk-io.c b/disk-io.c
index f9dbc85..b89bb29 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -831,6 +831,34 @@ static int find_best_backup_root(struct btrfs_super_block *super)
return best_index;
}

+static int setup_root_or_create_block(struct btrfs_fs_info *fs_info,
+ enum btrfs_open_ctree_flags flags,
+ struct btrfs_root *info_root,
+ u64 objectid, char *str)
+{
+ struct btrfs_super_block *sb = fs_info->super_copy;
+ struct btrfs_root *root = fs_info->tree_root;
+ u32 leafsize = btrfs_super_leafsize(sb);
+ int ret;
+
+ ret = find_and_setup_root(root, fs_info, objectid, info_root);
+ if (ret) {
+ printk("Couldn't setup %s tree\n", str);
+ if (!(flags & OPEN_CTREE_PARTIAL))
+ return -EIO;
+ /* Need a blank node here just so we don't screw up in the
+ * million of places that assume a root has a valid ->node
+ */
+ info_root->node =
+ btrfs_find_create_tree_block(info_root, 0, leafsize);
+ if (!info_root->node)
+ return -ENOMEM;
+ clear_extent_buffer_uptodate(NULL, info_root->node);
+ }
+
+ return 0;
+}
+
int btrfs_setup_all_roots(struct btrfs_fs_info *fs_info, u64 root_tree_bytenr,
enum btrfs_open_ctree_flags flags)
{
@@ -877,22 +905,10 @@ int btrfs_setup_all_roots(struct btrfs_fs_info *fs_info, u64 root_tree_bytenr,
return -EIO;
}

- ret = find_and_setup_root(root, fs_info, BTRFS_EXTENT_TREE_OBJECTID,
- fs_info->extent_root);
- if (ret) {
- printk("Couldn't setup extent tree\n");
- if (!(flags & OPEN_CTREE_PARTIAL))
- return -EIO;
- /* Need a blank node here just so we don't screw up in the
- * million of places that assume a root has a valid ->node
- */
- fs_info->extent_root->node =
- btrfs_find_create_tree_block(fs_info->extent_root, 0,
- leafsize);
- if (!fs_info->extent_root->node)
- return -ENOMEM;
- clear_extent_buffer_uptodate(NULL, fs_info->extent_root->node);
- }
+ ret = setup_root_or_create_block(fs_info, flags, fs_info->extent_root,
+ BTRFS_EXTENT_TREE_OBJECTID, "extent");
+ if (ret)
+ return ret;
fs_info->extent_root->track_dirty = 1;

ret = find_and_setup_root(root, fs_info, BTRFS_DEV_TREE_OBJECTID,
@@ -903,20 +919,10 @@ int btrfs_setup_all_roots(struct btrfs_fs_info *fs_info, u64 root_tree_bytenr,
}
fs_info->dev_root->track_dirty = 1;

- ret = find_and_setup_root(root, fs_info, BTRFS_CSUM_TREE_OBJECTID,
- fs_info->csum_root);
- if (ret) {
- printk("Couldn't setup csum tree\n");
- if (!(flags & OPEN_CTREE_PARTIAL))
- return -EIO;
- /* do the same thing as extent tree rebuilding */
- fs_info->csum_root->node =
- btrfs_find_create_tree_block(fs_info->extent_root, 0,
- leafsize);
- if (!fs_info->csum_root->node)
- return -ENOMEM;
- clear_extent_buffer_uptodate(NULL, fs_info->csum_root->node);
- }
+ ret = setup_root_or_create_block(fs_info, flags, fs_info->csum_root,
+ BTRFS_CSUM_TREE_OBJECTID, "csum");
+ if (ret)
+ return ret;
fs_info->csum_root->track_dirty = 1;

ret = find_and_setup_root(root, fs_info, BTRFS_QUOTA_TREE_OBJECTID,
--
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
David Sterba
2014-10-16 10:55:30 UTC
Permalink
Post by Zach Brown
btrfs_setup_all_roots() had some copy and pasted code for trying to
setup a root and then creating a blank node if that failed. The copy
for the csum_root created the blank node in the extent_root.
The cleanup is good, though I don't see the bug there. Although the passed root
is extent_root in both cases, it's only used to reach the
fs_info->extent_cache:

131 struct extent_buffer *btrfs_find_create_tree_block(struct btrfs_root *root,
132 u64 bytenr, u32 blocksize)
133 {
134 return alloc_extent_buffer(&root->fs_info->extent_cache, bytenr,
135 blocksize);
136 }
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
David Sterba
2014-10-16 11:46:12 UTC
Permalink
Post by Zach Brown
Here's another set of coverity fixes for btrfs-progs against David's
integration-20141007 branch.
Thanks, I've fished 2 patches for 3.17, the rest is queued.
Post by Zach Brown
I got tired of adding error checking after a few so I moved on to the
other warnings. Maybe we should subscribe linux-btrfs to the reports
that coverity can send out?
I'm not sure if this is allowed by the coverity service and I was not
able to any info about that.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Sandeen
2014-10-16 13:33:39 UTC
Permalink
Post by David Sterba
Post by Zach Brown
Here's another set of coverity fixes for btrfs-progs against David's
integration-20141007 branch.
Thanks, I've fished 2 patches for 3.17, the rest is queued.
Post by Zach Brown
I got tired of adding error checking after a few so I moved on to the
other warnings. Maybe we should subscribe linux-btrfs to the reports
that coverity can send out?
I'm not sure if this is allowed by the coverity service and I was not
able to any info about that.
We could, in theory, "invite" the list, and then I suppose we'd get
a confirmation email that 100 people would click on. ;)

Could maybe just email the scan folks and ask.

I'm a little on the fence about immediately broadcasting all defects, though.
I doubt there should be security implications, but ...

I wish scan were better integrated with git, and then something like
"email the author of the commit that introduced a new defect" would
be pretty cool.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Loading...