[PATCH 01/10] btrfs-progs: fix leak of "path" in btrfs_find_item() error paths

[PATCH 01/10] btrfs-progs: fix leak of "path" in btrfs_find_item() error paths

[Eryu Guan]

path needs to be freed before return.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 ctree.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/ctree.c b/ctree.c
index e6e5689..1434007 100644
--- a/ctree.c
+++ b/ctree.c
@@ -1058,26 +1058,28 @@ int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *found_path,
 		path = found_path;
 
 	ret = btrfs_search_slot(NULL, fs_root, &key, path, 0, 0);
-	if ((ret < 0) || (found_key == NULL)) {
-		if (path != found_path)
-			btrfs_free_path(path);
-		return ret;
-	}
+	if ((ret < 0) || (found_key == NULL))
+		goto out;
 
 	eb = path->nodes[0];
 	if (ret && path->slots[0] >= btrfs_header_nritems(eb)) {
 		ret = btrfs_next_leaf(fs_root, path);
 		if (ret)
-			return ret;
+			goto out;
 		eb = path->nodes[0];
 	}
 
 	btrfs_item_key_to_cpu(eb, found_key, path->slots[0]);
 	if (found_key->type != key.type ||
-			found_key->objectid != key.objectid)
-		return 1;
+			found_key->objectid != key.objectid) {
+		ret = 1;
+		goto out;
+	}
 
-	return 0;
+out:
+	if (path != found_path)
+		btrfs_free_path(path);
+	return ret;
 }
 
 /*
-- 
2.4.3

[PATCH 02/10] btrfs-progs: save and return error number correctly in check_chunks_and_extents

[Eryu Guan]

"err" is assigned to "ret" then "ret" gets overwritten by
check_extent_refs() before "ret" can be used.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 cmds-check.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index 4225b21..80a7c29 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -8066,13 +8066,12 @@ again:
 		goto out;
 	}
 
-	err = check_chunks(&chunk_cache, &block_group_cache,
+	ret = check_chunks(&chunk_cache, &block_group_cache,
 			   &dev_extent_cache, NULL, NULL, NULL, 0);
-	if (err) {
-		if (err == -EAGAIN)
+	if (ret) {
+		if (ret == -EAGAIN)
 			goto loop;
-		if (!ret)
-			ret = err;
+		err = ret;
 	}
 
 	ret = check_extent_refs(root, &extent_cache);
@@ -8082,8 +8081,8 @@ again:
 		goto out;
 	}
 
-	err = check_devices(&dev_cache, &dev_extent_cache);
-	if (err && !ret)
+	ret = check_devices(&dev_cache, &dev_extent_cache);
+	if (ret && err)
 		ret = err;
 
 out:
-- 
2.4.3

[PATCH 02/10] btrfs-progs: save error number correctly in check_chunks_and_extents

[Eryu Guan]

Coverity reports assigning value from "err" to "ret", but that stored
value is overwritten by check_extent_refs() before it can be used.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 cmds-check.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index 4225b21..80a7c29 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -8066,13 +8066,12 @@ again:
 		goto out;
 	}
 
-	err = check_chunks(&chunk_cache, &block_group_cache,
+	ret = check_chunks(&chunk_cache, &block_group_cache,
 			   &dev_extent_cache, NULL, NULL, NULL, 0);
-	if (err) {
-		if (err == -EAGAIN)
+	if (ret) {
+		if (ret == -EAGAIN)
 			goto loop;
-		if (!ret)
-			ret = err;
+		err = ret;
 	}
 
 	ret = check_extent_refs(root, &extent_cache);
@@ -8082,8 +8081,8 @@ again:
 		goto out;
 	}
 
-	err = check_devices(&dev_cache, &dev_extent_cache);
-	if (err && !ret)
+	ret = check_devices(&dev_cache, &dev_extent_cache);
+	if (ret && err)
 		ret = err;
 
 out:
-- 
2.4.3

Re: [PATCH 02/10] btrfs-progs: save error number correctly in check_chunks_and_extents

[David Sterba]

On Mon, Oct 19, 2015 at 07:37:52PM +0800, Eryu Guan wrote:
> Coverity reports assigning value from "err" to "ret", but that stored
> value is overwritten by check_extent_refs() before it can be used.

If you fix a coverity issue, please add a tag and the id, like

Resolves-coverity-id: 1234

Re: [PATCH 02/10] btrfs-progs: save error number correctly in check_chunks_and_extents

[Eryu Guan]

On Mon, Oct 19, 2015 at 03:41:04PM +0200, David Sterba wrote:
> On Mon, Oct 19, 2015 at 07:37:52PM +0800, Eryu Guan wrote:
> > Coverity reports assigning value from "err" to "ret", but that stored
> > value is overwritten by check_extent_refs() before it can be used.
> 
> If you fix a coverity issue, please add a tag and the id, like
> 
> Resolves-coverity-id: 1234

I was looking hard for CID but without luck.. I will ask around and see
where I can find the CIDs.

Thanks,
Eryu

Re: [PATCH 02/10] btrfs-progs: save error number correctly in check_chunks_and_extents

[David Sterba]

On Tue, Oct 20, 2015 at 06:28:00PM +0800, Eryu Guan wrote:
> On Mon, Oct 19, 2015 at 03:41:04PM +0200, David Sterba wrote:
> > On Mon, Oct 19, 2015 at 07:37:52PM +0800, Eryu Guan wrote:
> > > Coverity reports assigning value from "err" to "ret", but that stored
> > > value is overwritten by check_extent_refs() before it can be used.
> > 
> > If you fix a coverity issue, please add a tag and the id, like
> > 
> > Resolves-coverity-id: 1234
> 
> I was looking hard for CID but without luck.. I will ask around and see
> where I can find the CIDs.

Ok never mind then, I'll pick the patch as-is.

[PATCH 03/10] btrfs-progs: remove deadcode around metadump_v2 in check_chunk_refs

[Eryu Guan]

metadump_v2 is initialized to 0 and never updated again. So remove the
deadcode.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---

I'm not sure about this one, seems metadump_v2 should be initialized depending
on whether superblock has BTRFS_SUPER_FLAG_METADUMP_V2 flag set.

 cmds-check.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index 80a7c29..118a274 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -7597,7 +7597,6 @@ static int check_chunk_refs(struct chunk_record *chunk_rec,
 	u64 devid;
 	u64 offset;
 	u64 length;
-	int metadump_v2 = 0;
 	int i;
 	int ret = 0;
 
@@ -7610,8 +7609,7 @@ static int check_chunk_refs(struct chunk_record *chunk_rec,
 					       cache);
 		if (chunk_rec->length != block_group_rec->offset ||
 		    chunk_rec->offset != block_group_rec->objectid ||
-		    (!metadump_v2 &&
-		     chunk_rec->type_flags != block_group_rec->flags)) {
+		    chunk_rec->type_flags != block_group_rec->flags) {
 			if (!silent)
 				fprintf(stderr,
 					"Chunk[%llu, %u, %llu]: length(%llu), offset(%llu), type(%llu) mismatch with block group[%llu, %u, %llu]: offset(%llu), objectid(%llu), flags(%llu)\n",
@@ -7645,9 +7643,6 @@ static int check_chunk_refs(struct chunk_record *chunk_rec,
 		ret = 1;
 	}
 
-	if (metadump_v2)
-		return ret;
-
 	length = calc_stripe_length(chunk_rec->type_flags, chunk_rec->length,
 				    chunk_rec->num_stripes);
 	for (i = 0; i < chunk_rec->num_stripes; ++i) {
-- 
2.4.3

Re: [PATCH 03/10] btrfs-progs: remove deadcode around metadump_v2 in check_chunk_refs

[David Sterba]

On Mon, Oct 19, 2015 at 07:37:53PM +0800, Eryu Guan wrote:
> metadump_v2 is initialized to 0 and never updated again. So remove the
> deadcode.

Josef introduced the v2 metadump to resolve some issues with the v1
format. Even if the code appears dead, I think it's "not yet used", but
I don't know what were the intentions with it.

[PATCH 04/10] btrfs-progs: return -EIO properly in restore_metadump()

[Eryu Guan]

Error number -EIO is assigned to ret but later ret is overwritten by
wait_for_worker().

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 btrfs-image.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index 82eed05..7d3a2f8 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -2465,6 +2465,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 	u64 bytenr = 0;
 	FILE *in = NULL;
 	int ret = 0;
+	int err = 0;
 
 	if (!strcmp(input, "-")) {
 		in = stdin;
@@ -2526,7 +2527,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 		if (le64_to_cpu(header->magic) != HEADER_MAGIC ||
 		    le64_to_cpu(header->bytenr) != bytenr) {
 			fprintf(stderr, "bad header in metadump image\n");
-			ret = -EIO;
+			err = -EIO;
 			break;
 		}
 		ret = add_cluster(cluster, &mdrestore, &bytenr);
@@ -2536,6 +2537,8 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 		}
 	}
 	ret = wait_for_worker(&mdrestore);
+	if (!ret)
+		ret = err;
 
 	if (!ret && !multi_devices && !old_restore) {
 		struct btrfs_root *root;
-- 
2.4.3

Re: [PATCH 04/10] btrfs-progs: return -EIO properly in restore_metadump()

[David Sterba]

On Mon, Oct 19, 2015 at 07:37:54PM +0800, Eryu Guan wrote:
> Error number -EIO is assigned to ret but later ret is overwritten by
> wait_for_worker().
> 
> Signed-off-by: Eryu Guan <guaneryu@gmail.com>
> ---
>  btrfs-image.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/btrfs-image.c b/btrfs-image.c
> index 82eed05..7d3a2f8 100644
> --- a/btrfs-image.c
> +++ b/btrfs-image.c
> @@ -2465,6 +2465,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
>  	u64 bytenr = 0;
>  	FILE *in = NULL;
>  	int ret = 0;
> +	int err = 0;
>  
>  	if (!strcmp(input, "-")) {
>  		in = stdin;
> @@ -2526,7 +2527,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
>  		if (le64_to_cpu(header->magic) != HEADER_MAGIC ||
>  		    le64_to_cpu(header->bytenr) != bytenr) {
>  			fprintf(stderr, "bad header in metadump image\n");
> -			ret = -EIO;
> +			err = -EIO;

Shouldn't we do 'goto out' instead? As this looks likes the metadump is
broken anyway so there's no point in continuing.

[PATCH 05/10] btrfs-progs: mute coverity warnings about deadcode

[Eryu Guan]

Coverity reports execution cannot reach this statements. So put WARN_ON
in if-else conditions.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 backref.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/backref.c b/backref.c
index 9a2efca..8f41f82 100644
--- a/backref.c
+++ b/backref.c
@@ -323,9 +323,9 @@ static int __resolve_indirect_ref(struct btrfs_fs_info *fs_info,
 
 	eb = path->nodes[level];
 	while (!eb) {
-		WARN_ON(!level);
 		if (!level) {
 			ret = 1;
+			WARN_ON(1);
 			goto out;
 		}
 		level--;
@@ -1178,7 +1178,6 @@ int extent_from_logical(struct btrfs_fs_info *fs_info, u64 logical,
 		 logical, logical - found_key->objectid, found_key->objectid,
 		 found_key->offset, flags, item_size);
 
-	WARN_ON(!flags_ret);
 	if (flags_ret) {
 		if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK)
 			*flags_ret = BTRFS_EXTENT_FLAG_TREE_BLOCK;
@@ -1187,9 +1186,10 @@ int extent_from_logical(struct btrfs_fs_info *fs_info, u64 logical,
 		else
 			BUG_ON(1);
 		return 0;
+	} else {
+		WARN_ON(1);
+		return -EIO;
 	}
-
-	return -EIO;
 }
 
 /*
-- 
2.4.3

Re: [PATCH 05/10] btrfs-progs: mute coverity warnings about deadcode

[David Sterba]

On Mon, Oct 19, 2015 at 07:37:55PM +0800, Eryu Guan wrote:
> Coverity reports execution cannot reach this statements. So put WARN_ON
> in if-else conditions.
> 
> Signed-off-by: Eryu Guan <guaneryu@gmail.com>

Applied, thanks.

[PATCH 06/10] btrfs-progs: vailidate pointer before dereferencing it in btrfs_cow_block()

[Eryu Guan]

Check trans before dereferencing it.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 ctree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ctree.c b/ctree.c
index 1434007..f9d972a 100644
--- a/ctree.c
+++ b/ctree.c
@@ -356,7 +356,7 @@ int btrfs_cow_block(struct btrfs_trans_handle *trans,
 		WARN_ON(1);
 	}
 	*/
-	if (trans->transid != root->fs_info->generation) {
+	if (trans && trans->transid != root->fs_info->generation) {
 		printk(KERN_CRIT "trans %llu running %llu\n",
 			(unsigned long long)trans->transid,
 			(unsigned long long)root->fs_info->generation);
-- 
2.4.3

Re: [PATCH 06/10] btrfs-progs: vailidate pointer before dereferencing it in btrfs_cow_block()

[David Sterba]

On Mon, Oct 19, 2015 at 07:37:56PM +0800, Eryu Guan wrote:
> Check trans before dereferencing it.

Have you found a code path where 'trans' can be NULL? A quick search did
not reveal anything.

[PATCH 07/10] btrfs-progs: fix memory leak on error path

[Eryu Guan]

dev_scans and t_scans should be freed on malloc error.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 chunk-recover.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/chunk-recover.c b/chunk-recover.c
index 1fb04f7..c727f0f 100644
--- a/chunk-recover.c
+++ b/chunk-recover.c
@@ -847,11 +847,16 @@ static int scan_devices(struct recover_control *rc)
 	if (!dev_scans)
 		return -ENOMEM;
 	t_scans = (pthread_t *)malloc(sizeof(pthread_t) * devnr);
-	if (!t_scans)
+	if (!t_scans) {
+		free(dev_scans);
 		return -ENOMEM;
+	}
 	t_rets = (long *)malloc(sizeof(long) * devnr);
-	if (!t_rets)
+	if (!t_rets) {
+		free(dev_scans);
+		free(t_scans);
 		return -ENOMEM;
+	}
 
 	list_for_each_entry(dev, &rc->fs_devices->devices, dev_list) {
 		fd = open(dev->name, O_RDONLY);
-- 
2.4.3

[PATCH 08/10] btrfs-progs: remove identical branch in record_extent()

[Eryu Guan]

The same code is executed when the condition "ret" is true or false,
because the code in the if-then branch and after the if statement is
identical.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 cmds-check.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index 118a274..a459893 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -6310,8 +6310,6 @@ static int record_extent(struct btrfs_trans_handle *trans,
 			"start %llu len %llu parent %llu root %llu\n",
 			rec->start, rec->max_size, parent, tback->root);
 	}
-	if (ret)
-		goto fail;
 fail:
 	btrfs_release_path(path);
 	return ret;
-- 
2.4.3

[PATCH 09/10] btrfs-progs: fix memory leak in cmd_qgroup_show()

[Eryu Guan]

filter_set and comparer_set should be freed on return.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 cmds-qgroup.c | 2 ++
 qgroup.c      | 1 +
 2 files changed, 3 insertions(+)

diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index 48c1733..82bd2e2 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -354,6 +354,8 @@ static int cmd_qgroup_show(int argc, char **argv)
 	fd = open_file_or_dir(path, &dirstream);
 	if (fd < 0) {
 		fprintf(stderr, "ERROR: can't access '%s'\n", path);
+		btrfs_qgroup_free_filter_set(filter_set);
+		btrfs_qgroup_free_comparer_set(comparer_set);
 		return 1;
 	}
 
diff --git a/qgroup.c b/qgroup.c
index ec9a3ac..0272aa8 100644
--- a/qgroup.c
+++ b/qgroup.c
@@ -1211,6 +1211,7 @@ int btrfs_show_qgroups(int fd,
 
 	__free_all_qgroups(&qgroup_lookup);
 	btrfs_qgroup_free_filter_set(filter_set);
+	btrfs_qgroup_free_comparer_set(comp_set);
 	return ret;
 }
 
-- 
2.4.3

[PATCH 10/10] btrfs-progs: return -ENOMEM properly in btrfs_read_block_groups()

[Eryu Guan]

Breaking from the while loop makes ret overwritten to zero, goto error
label directly and return -ENOMEM.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 extent-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extent-tree.c b/extent-tree.c
index 0c8152a..97cf961 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -3255,7 +3255,7 @@ int btrfs_read_block_groups(struct btrfs_root *root)
 		cache = kzalloc(sizeof(*cache), GFP_NOFS);
 		if (!cache) {
 			ret = -ENOMEM;
-			break;
+			goto error;
 		}
 
 		read_extent_buffer(leaf, &cache->item,
-- 
2.4.3

Re: [PATCH 01/10] btrfs-progs: fix leak of "path" in btrfs_find_item() error paths

[David Sterba]

I've merged 1, 2 (the first one), 7, 8, 9 and 10. Please add the missing
coverity references and resend. The remaining patches seem unnecessary
but please check whether I haven't missed something.