[RFC] another round of static analysis fixes

[RFC] another round of static analysis fixes

[Zach Brown]

Hi friends, 

Eric imported a newer git snapshot of btrfs-progs into Red Hat's
universe which kicked off a static analysis run which found a bunch
of problems.  This series is my attempt to fix the warnings that I
agreed were either real bugs or messy code to clean up.

This is against Dave's integration-next as of:

commit 62f6537f8c9149283844c5e93a296e147c266247
Date:   Fri Sep 13 19:32:23 2013 +0800

And totally untested.  Review please?

- z

[PATCH 01/12] btrfs-progs: check path alloc in corrupt block

[Zach Brown]

btrfs-corrupt-block added some untested path allocations.  These showed
up in static analysis when they pass their path to btrfs_search_slot()
which unconditionally dereferences the path.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 btrfs-corrupt-block.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 9e72ca8..018c23d 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -502,6 +502,9 @@ int corrupt_chunk_tree(struct btrfs_trans_handle *trans,
 	struct extent_buffer *leaf;
 
 	path = btrfs_alloc_path();
+	if (!path)
+		return -ENOMEM;
+
 	key.objectid = (u64)-1;
 	key.offset = (u64)-1;
 	key.type = (u8)-1;
@@ -531,7 +534,7 @@ int corrupt_chunk_tree(struct btrfs_trans_handle *trans,
 		if (ret)
 			goto free_out;
 	}
-	btrfs_free_path(path);
+	btrfs_release_path(path);
 
 	/* Here, cow and ins_len must equals 0 for the following reasons:
 	 * 1) chunk recover is based on disk scanning, so COW should be
@@ -540,7 +543,6 @@ int corrupt_chunk_tree(struct btrfs_trans_handle *trans,
 	 * 2) if cow = 0, ins_len must also be set to 0, or BUG_ON will be
 	 *    triggered.
 	 */
-	path = btrfs_alloc_path();
 	ret = btrfs_search_slot(trans, root, &key, path, 0, 0);
 	BUG_ON(ret == 0);
 	if (ret < 0) {
@@ -720,6 +722,10 @@ int main(int ac, char **av)
 			print_usage();
 		del = rand() % 3;
 		path = btrfs_alloc_path();
+		if (!path) {
+			fprintf(stderr, "path allocation failed\n");
+			goto out_close;
+		}
 
 		if (find_chunk_offset(root->fs_info->chunk_root, path,
 				      logical) != 0) {
-- 
1.7.11.7

[PATCH 02/12] btrfs-progs: check fopen failure in cmds-send

[Zach Brown]

Check for fopen() failure.  This shows up in static analysis as a
possible null pointer derference.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 cmds-send.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/cmds-send.c b/cmds-send.c
index 374d040..5f6ff86 100644
--- a/cmds-send.c
+++ b/cmds-send.c
@@ -72,6 +72,11 @@ int find_mount_root(const char *path, char **mount_root)
 	close(fd);
 
 	mnttab = fopen("/proc/mounts", "r");
+	if (!mnttab) {
+		close(fd);
+		return -errno;
+	}
+
 	while ((ent = getmntent(mnttab))) {
 		len = strlen(ent->mnt_dir);
 		if (strncmp(ent->mnt_dir, path, len) == 0) {
-- 
1.7.11.7

[PATCH 03/12] btrfs-progs: don't overrun name in find-collisions

[Zach Brown]

find_collision() allocates name_len bytes for its sub array so the index
must be less than name_len.  This was found by static analysis.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 btrfs-image.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index b05cf07..7474642 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -314,11 +314,11 @@ static char *find_collision(struct metadump_struct *md, char *name,
 		if (val->sub[i] == 127) {
 			do {
 				i++;
-				if (i > name_len)
+				if (i >= name_len)
 					break;
 			} while (val->sub[i] == 127);
 
-			if (i > name_len)
+			if (i >= name_len)
 				break;
 			val->sub[i]++;
 			if (val->sub[i] == '/')
-- 
1.7.11.7

[PATCH 04/12] btrfs-progs: don't overflow read buffer in image

[Zach Brown]

search_for_chunk_blocks() allocates a fixed-size buffer and then reads
arbitrary u32 sized buffers in to it.  Instead let's fail if the item is
bigger than the buffer.  This was found by static analysis.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 btrfs-image.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index 7474642..03ad4e9 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -2011,6 +2011,7 @@ static int search_for_chunk_blocks(struct mdrestore_struct *mdres,
 	u64 current_cluster = cluster_bytenr, bytenr;
 	u64 item_bytenr;
 	u32 bufsize, nritems, i;
+	u32 max_size = MAX_PENDING_SIZE * 2;
 	u8 *buffer, *tmp = NULL;
 	int ret = 0;
 
@@ -2020,7 +2021,7 @@ static int search_for_chunk_blocks(struct mdrestore_struct *mdres,
 		return -ENOMEM;
 	}
 
-	buffer = malloc(MAX_PENDING_SIZE * 2);
+	buffer = malloc(max_size);
 	if (!buffer) {
 		fprintf(stderr, "Error allocing buffer\n");
 		free(cluster);
@@ -2028,7 +2029,7 @@ static int search_for_chunk_blocks(struct mdrestore_struct *mdres,
 	}
 
 	if (mdres->compress_method == COMPRESS_ZLIB) {
-		tmp = malloc(MAX_PENDING_SIZE * 2);
+		tmp = malloc(max_size);
 		if (!tmp) {
 			fprintf(stderr, "Error allocing tmp buffer\n");
 			free(cluster);
@@ -2079,6 +2080,13 @@ static int search_for_chunk_blocks(struct mdrestore_struct *mdres,
 			bufsize = le32_to_cpu(item->size);
 			item_bytenr = le64_to_cpu(item->bytenr);
 
+			if (bufsize > max_size) {
+				fprintf(stderr, "item %u size %u too big\n",
+					i, bufsize);
+				ret = -EIO;
+				break;
+			}
+
 			if (mdres->compress_method == COMPRESS_ZLIB) {
 				ret = fread(tmp, bufsize, 1, mdres->in);
 				if (ret != 1) {
@@ -2088,7 +2096,7 @@ static int search_for_chunk_blocks(struct mdrestore_struct *mdres,
 					break;
 				}
 
-				size = MAX_PENDING_SIZE * 2;
+				size = max_size;
 				ret = uncompress(buffer,
 						 (unsigned long *)&size, tmp,
 						 bufsize);
-- 
1.7.11.7

[PATCH 05/12] btrfs-progs: check link_subvol name base

[Zach Brown]

In principle, link_subvol() can be given an abitrary string as the name
of the saved subvolume.  It copies it into a fixed-size stack buffer and
then uses it as dirent names without testing its length.

This limits its length to BTRFS_NAME_LEN.  This was found by static
analsys.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 btrfs-convert.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/btrfs-convert.c b/btrfs-convert.c
index 221dd45..edef1bd 100644
--- a/btrfs-convert.c
+++ b/btrfs-convert.c
@@ -1428,10 +1428,15 @@ static struct btrfs_root * link_subvol(struct btrfs_root *root,
 	struct btrfs_key key;
 	u64 dirid = btrfs_root_dirid(&root->root_item);
 	u64 index = 2;
-	char buf[64];
+	char buf[BTRFS_NAME_LEN + 1]; /* for snprintf null */
+	int len;
 	int i;
 	int ret;
 
+	len = strlen(base);
+	if (len < 1 || len > BTRFS_NAME_LEN)
+		return NULL;
+
 	path = btrfs_alloc_path();
 	BUG_ON(!path);
 
@@ -1467,18 +1472,22 @@ static struct btrfs_root * link_subvol(struct btrfs_root *root,
 	key.offset = (u64)-1;
 	key.type = BTRFS_ROOT_ITEM_KEY;
 
-	strcpy(buf, base);
+	memcpy(buf, base, len);
 	for (i = 0; i < 1024; i++) {
-		ret = btrfs_insert_dir_item(trans, root, buf, strlen(buf),
+		ret = btrfs_insert_dir_item(trans, root, buf, len,
 					    dirid, &key, BTRFS_FT_DIR, index);
 		if (ret != -EEXIST)
 			break;
-		sprintf(buf, "%s%d", base, i);
+		len = snprintf(buf, ARRAY_SIZE(buf), "%s%d", base, i);
+		if (len < 1 || len > BTRFS_NAME_LEN) {
+			ret = -EINVAL;
+			break;
+		}
 	}
 	if (ret)
 		goto fail;
 
-	btrfs_set_inode_size(leaf, inode_item, strlen(buf) * 2 +
+	btrfs_set_inode_size(leaf, inode_item, len * 2 +
 			     btrfs_inode_size(leaf, inode_item));
 	btrfs_mark_buffer_dirty(leaf);
 	btrfs_release_path(path);
@@ -1487,13 +1496,13 @@ static struct btrfs_root * link_subvol(struct btrfs_root *root,
 	ret = btrfs_add_root_ref(trans, tree_root, root_objectid,
 				 BTRFS_ROOT_BACKREF_KEY,
 				 root->root_key.objectid,
-				 dirid, index, buf, strlen(buf));
+				 dirid, index, buf, len);
 	BUG_ON(ret);
 
 	/* now add the forward ref */
 	ret = btrfs_add_root_ref(trans, tree_root, root->root_key.objectid,
 				 BTRFS_ROOT_REF_KEY, root_objectid,
-				 dirid, index, buf, strlen(buf));
+				 dirid, index, buf, len);
 
 	ret = btrfs_commit_transaction(trans, root);
 	BUG_ON(ret);
-- 
1.7.11.7

[PATCH 06/12] btrfs-progs: remove dead block group checking

[Zach Brown]

Don't carry around dead code.  If its needed again, it's only a few git
commands away.  This was found by static analysis.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 cmds-check.c | 50 --------------------------------------------------
 1 file changed, 50 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index c91cc4a..ebba58e 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -4937,55 +4937,6 @@ static void free_corrupt_block(struct cache_extent *cache)
 
 FREE_EXTENT_CACHE_BASED_TREE(corrupt_blocks, free_corrupt_block);
 
-static int check_block_group(struct btrfs_trans_handle *trans,
-			      struct btrfs_fs_info *info,
-			      struct map_lookup *map,
-			      int *reinit)
-{
-	struct btrfs_key key;
-	struct btrfs_path path;
-	int ret;
-
-	key.objectid = map->ce.start;
-	key.offset = map->ce.size;
-	key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
-
-	btrfs_init_path(&path);
-	ret = btrfs_search_slot(NULL, info->extent_root,
-				&key, &path, 0, 0);
-	btrfs_release_path(&path);
-	if (ret <= 0)
-		goto out;
-
-	ret = btrfs_make_block_group(trans, info->extent_root, 0, map->type,
-			       BTRFS_FIRST_CHUNK_TREE_OBJECTID,
-			       key.objectid, key.offset);
-	*reinit = 1;
-out:
-	return ret;
-}
-
-static int check_block_groups(struct btrfs_trans_handle *trans,
-			      struct btrfs_fs_info *info, int *reinit)
-{
-	struct cache_extent *ce;
-	struct map_lookup *map;
-	struct btrfs_mapping_tree *map_tree = &info->mapping_tree;
-
-	/* this isn't quite working */
-	return 0;
-
-	ce = search_cache_extent(&map_tree->cache_tree, 0);
-	while (1) {
-		if (!ce)
-			break;
-		map = container_of(ce, struct map_lookup, ce);
-		check_block_group(trans, info, map, reinit);
-		ce = next_cache_extent(ce);
-	}
-	return 0;
-}
-
 static void reset_cached_block_groups(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_block_group_cache *cache;
@@ -5047,7 +4998,6 @@ static int check_extent_refs(struct btrfs_trans_handle *trans,
 			cache = next_cache_extent(cache);
 		}
 		prune_corrupt_blocks(trans, root->fs_info);
-		check_block_groups(trans, root->fs_info, &reinit);
 		if (reinit)
 			btrfs_read_block_groups(root->fs_info->extent_root);
 		reset_cached_block_groups(root->fs_info);
-- 
1.7.11.7

[PATCH 07/12] btrfs-progs: free eb in fixup_chunk_tree_block()

[Zach Brown]

This was found by static analysis.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 btrfs-image.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/btrfs-image.c b/btrfs-image.c
index 03ad4e9..d10447f 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -1541,6 +1541,7 @@ next:
 		bytenr += mdres->leafsize;
 	}
 
+	free(eb);
 	return 0;
 }
 
-- 
1.7.11.7

[PATCH 08/12] btrfs-progs: don't leak path in verify_space_cache

[Zach Brown]

This was found by static analysis.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 cmds-check.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index ebba58e..b6035d7 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -3228,13 +3228,13 @@ static int verify_space_cache(struct btrfs_root *root,
 
 	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
 	if (ret < 0)
-		return ret;
+		goto out;
 	ret = 0;
 	while (1) {
 		if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
 			ret = btrfs_next_leaf(root, path);
 			if (ret < 0)
-				return ret;
+				goto out;
 			if (ret > 0) {
 				ret = 0;
 				break;
@@ -3274,6 +3274,8 @@ static int verify_space_cache(struct btrfs_root *root,
 		ret = check_cache_range(root, cache, last,
 					cache->key.objectid +
 					cache->key.offset - last);
+
+out:
 	btrfs_free_path(path);
 
 	if (!ret &&
-- 
1.7.11.7

[PATCH 09/12] btrfs-progs: don't deref pipefd[-1]

[Zach Brown]

commit 4782e8ebdb583dfa3615f7b38dee729d34f62ec1 accidentally replaced
[0] with [-1].  Put it back.  This was found by static analysis.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 send-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/send-test.c b/send-test.c
index 3775f5f..a37b7fd 100644
--- a/send-test.c
+++ b/send-test.c
@@ -354,7 +354,7 @@ static void *process_thread(void *arg_)
 	int ret;
 
 	while (1) {
-		ret = btrfs_read_and_process_send_stream(pipefd[-1],
+		ret = btrfs_read_and_process_send_stream(pipefd[0],
 							 &send_ops_print, arg_, 0);
 		if (ret)
 			break;
-- 
1.7.11.7

[PATCH 10/12] btrfs-progs: don't overflow colors[] in fragments

[Zach Brown]

Stop iteration at the number of elements in the colors[] array when
initializing the elements.  Rather than a magic number.  This was found
by static analysis.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 btrfs-fragments.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/btrfs-fragments.c b/btrfs-fragments.c
index 4dd9470..cedbc57 100644
--- a/btrfs-fragments.c
+++ b/btrfs-fragments.c
@@ -283,7 +283,7 @@ list_fragments(int fd, u64 flags, char *dir)
 				white = gdImageColorAllocate(im, 255, 255, 255);
 				black = gdImageColorAllocate(im, 0, 0, 0);  
 
-				for (j = 0; j < 10; ++j)
+				for (j = 0; j < ARRAY_SIZE(colors); ++j)
 					colors[j] = black;
 
 				init_colors(im, colors);
-- 
1.7.11.7

[PATCH 11/12] btrfs-progs: remove unused variables

[Zach Brown]

Presumably people missed these warnings because btrfs-fragments isn't
built by default.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 btrfs-fragments.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/btrfs-fragments.c b/btrfs-fragments.c
index cedbc57..160429a 100644
--- a/btrfs-fragments.c
+++ b/btrfs-fragments.c
@@ -191,7 +191,6 @@ list_fragments(int fd, u64 flags, char *dir)
 	u64 bgused = 0;
 	u64 saved_extent = 0;
 	u64 saved_len = 0;
-	u64 saved_flags = 0;
 	int saved_color = 0;
 	u64 last_end = 0;
 	u64 areas = 0;
@@ -202,7 +201,6 @@ list_fragments(int fd, u64 flags, char *dir)
 
 	gdImagePtr im = NULL;
 	int black = 0;
-	int white = 0;
 	int width = 800;
 
 	snprintf(name, sizeof(name), "%s/index.html", dir);
@@ -280,7 +278,6 @@ list_fragments(int fd, u64 flags, char *dir)
 				im = gdImageCreate(width,
 					(sh->offset / 4096 + 799) / width);
 
-				white = gdImageColorAllocate(im, 255, 255, 255);
 				black = gdImageColorAllocate(im, 0, 0, 0);  
 
 				for (j = 0; j < ARRAY_SIZE(colors); ++j)
@@ -308,13 +305,11 @@ list_fragments(int fd, u64 flags, char *dir)
 				saved_len = 0;
 			}
 			if (im && sh->type == BTRFS_EXTENT_ITEM_KEY) {
-				u64 e_flags;
 				int c;
 				struct btrfs_extent_item *item;
 
 				item = (struct btrfs_extent_item *)
 						(args.buf + off);
-				e_flags = btrfs_stack_extent_flags(item);
 
 				if (use_color)
 					c = colors[get_color(item, sh->len)];
@@ -328,7 +323,6 @@ list_fragments(int fd, u64 flags, char *dir)
 				if (sh->objectid == bgend) {
 					saved_extent = sh->objectid;
 					saved_len = sh->offset;
-					saved_flags = e_flags;
 					saved_color = c;
 					goto skip;
 				}
-- 
1.7.11.7

[PATCH 12/12] btrfs-progs: free leaked roots in calc-size

[Zach Brown]

This was found by static analysis.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 btrfs-calc-size.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/btrfs-calc-size.c b/btrfs-calc-size.c
index 5aa0b70..38b70d9 100644
--- a/btrfs-calc-size.c
+++ b/btrfs-calc-size.c
@@ -257,5 +257,6 @@ int main(int argc, char **argv)
 		goto out;
 out:
 	close_ctree(root);
+	free(roots);
 	return ret;
 }
-- 
1.7.11.7

Re: [PATCH 09/12] btrfs-progs: don't deref pipefd[-1]

[Eric Sandeen]

On 10/7/13 4:43 PM, Zach Brown wrote:
> commit 4782e8ebdb583dfa3615f7b38dee729d34f62ec1 accidentally replaced
> [0] with [-1].  Put it back.  This was found by static analysis.
> 
> Signed-off-by: Zach Brown <zab@redhat.com>

eeehhhyeah.  Thanks for being charitable. ;)

Really, I have no idea how that happened.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  send-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/send-test.c b/send-test.c
> index 3775f5f..a37b7fd 100644
> --- a/send-test.c
> +++ b/send-test.c
> @@ -354,7 +354,7 @@ static void *process_thread(void *arg_)
>  	int ret;
>  
>  	while (1) {
> -		ret = btrfs_read_and_process_send_stream(pipefd[-1],
> +		ret = btrfs_read_and_process_send_stream(pipefd[0],
>  							 &send_ops_print, arg_, 0);
>  		if (ret)
>  			break;
> 

Re: [PATCH 09/12] btrfs-progs: don't deref pipefd[-1]

[Zach Brown]

On Mon, Oct 07, 2013 at 04:45:01PM -0500, Eric Sandeen wrote:
> On 10/7/13 4:43 PM, Zach Brown wrote:
> > commit 4782e8ebdb583dfa3615f7b38dee729d34f62ec1 accidentally replaced
> > [0] with [-1].  Put it back.  This was found by static analysis.
> > 
> > Signed-off-by: Zach Brown <zab@redhat.com>
> 
> eeehhhyeah.  Thanks for being charitable. ;)
> 
> Really, I have no idea how that happened.

I couldn't figure it out either.  The best I could come up with is that
one of the local XFS guys climbed in your office window and made the
change while you were at lunch.  In a black leotard with an awesome mask
and a black sack over their shoulder.

- z

Re: [PATCH 08/12] btrfs-progs: don't leak path in verify_space_cache

[chandan]

On Monday 07 Oct 2013 2:43:01 PM you wrote:
> This was found by static analysis.
> 
> Signed-off-by: Zach Brown <zab@redhat.com>
> ---
>  cmds-check.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds-check.c b/cmds-check.c
> index ebba58e..b6035d7 100644
> --- a/cmds-check.c
> +++ b/cmds-check.c
> @@ -3228,13 +3228,13 @@ static int verify_space_cache(struct btrfs_root *root,
> 
>  	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
>  	if (ret < 0)
> -		return ret;
> +		goto out;
>  	ret = 0;
>  	while (1) {
>  		if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
>  			ret = btrfs_next_leaf(root, path);
>  			if (ret < 0)
> -				return ret;
> +				goto out;
>  			if (ret > 0) {
>  				ret = 0;
>  				break;
> @@ -3274,6 +3274,8 @@ static int verify_space_cache(struct btrfs_root *root,
>  		ret = check_cache_range(root, cache, last,
>  					cache->key.objectid +
>  					cache->key.offset - last);
> +
> +out:
>  	btrfs_free_path(path);
> 
>  	if (!ret &&
> 
This has been fixed by commit 7ae60bf1. But I feel that your fix is
better since the clean up code is segregated to one place and hence we
have fewer places in the function where we return from.

Reviewed-by: chandan <chandan@linux.vnet.ibm.com>

Re: [PATCH 02/12] btrfs-progs: check fopen failure in cmds-send

[Stefan Behrens]

On Mon,  7 Oct 2013 14:42:55 -0700, Zach Brown wrote:
> Check for fopen() failure.  This shows up in static analysis as a
> possible null pointer derference.
> 
> Signed-off-by: Zach Brown <zab@redhat.com>
> ---
>  cmds-send.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/cmds-send.c b/cmds-send.c
> index 374d040..5f6ff86 100644
> --- a/cmds-send.c
> +++ b/cmds-send.c
> @@ -72,6 +72,11 @@ int find_mount_root(const char *path, char **mount_root)
>  	close(fd);
>  
>  	mnttab = fopen("/proc/mounts", "r");
> +	if (!mnttab) {
> +		close(fd);
> +		return -errno;

close() can modify errno.

And close(fd) is already called 4 lines above. You didn't run the static
code analysis again after applying your patch :)


> +	}
> +
>  	while ((ent = getmntent(mnttab))) {
>  		len = strlen(ent->mnt_dir);
>  		if (strncmp(ent->mnt_dir, path, len) == 0) {
> 

Re: [RFC] another round of static analysis fixes

[David Sterba]

On Mon, Oct 07, 2013 at 02:42:53PM -0700, Zach Brown wrote:
> Eric imported a newer git snapshot of btrfs-progs into Red Hat's
> universe which kicked off a static analysis run which found a bunch
> of problems.  This series is my attempt to fix the warnings that I
> agreed were either real bugs or messy code to clean up.
> 
> This is against Dave's integration-next as of:
> 
> commit 62f6537f8c9149283844c5e93a296e147c266247
> Date:   Fri Sep 13 19:32:23 2013 +0800
> 
> And totally untested.  Review please?

Compile-tested!

[02/12] not merged, [08/12] replaces chandan's commit

thanks

Re: [PATCH 02/12] btrfs-progs: check fopen failure in cmds-send

[Zach Brown]

> And close(fd) is already called 4 lines above. You didn't run the static
> code analysis again after applying your patch :)

Nice, thanks for catching that.  I certainly didn't run it again, no :).

- z

Re: [PATCH 02/12] btrfs-progs: check fopen failure in cmds-send

[Zach Brown]

> > @@ -72,6 +72,11 @@ int find_mount_root(const char *path, char **mount_root)
> >  	close(fd);
> >  
> >  	mnttab = fopen("/proc/mounts", "r");
> > +	if (!mnttab) {
> > +		close(fd);
> > +		return -errno;
> 
> close() can modify errno.
> 
> And close(fd) is already called 4 lines above. You didn't run the static
> code analysis again after applying your patch :)

OK, here's a less dumb attempt:

- z

>From f8a3425c184a55e0c254143e520e60a6856c27da Mon Sep 17 00:00:00 2001
From: Zach Brown <zab@redhat.com>
Date: Fri, 4 Oct 2013 15:38:18 -0700
Subject: [PATCH] btrfs-progs: check fopen failure in cmds-send

Check for fopen() failure.  This shows up in static analysis as a
possible null pointer derference.

Signed-off-by: Zach Brown <zab@redhat.com>
Laughed-at-by: Stefan Behrens <sbehrens@giantdisaster.de>
---
 cmds-send.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/cmds-send.c b/cmds-send.c
index 374d040..81b3e49 100644
--- a/cmds-send.c
+++ b/cmds-send.c
@@ -72,6 +72,9 @@ int find_mount_root(const char *path, char **mount_root)
 	close(fd);
 
 	mnttab = fopen("/proc/mounts", "r");
+	if (!mnttab)
+		return -errno;
+
 	while ((ent = getmntent(mnttab))) {
 		len = strlen(ent->mnt_dir);
 		if (strncmp(ent->mnt_dir, path, len) == 0) {
-- 
1.7.11.7

Re: [RFC] another round of static analysis fixes

[Zach Brown]

> Compile-tested!

Cool, thanks!

> [02/12] not merged, [08/12] replaces chandan's commit

I just sent another attempt at 02/ :).

- z