thr3ads.net - Btrfs devel - [PATCH 00/16] btrfs-progs: Several more static analysis defect fixes [Nov 2013]

If this information is useful, please help other people find it:
Share via:

Eric Sandeen

2013-Nov-06 23:15 UTC

[PATCH 00/16] btrfs-progs: Several more static analysis defect fixes

These all apply to the integration branch in Chris''s current git tree.
(Which saw the defects rise by from 55 to 65 in the last l6 commits :()

This beats back the defect count again.  Compile tested only, FWIW.

Thanks,
-Eric


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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 01/16] btrfs-progs: fix potential double-frees in cmd_subvol_delete()

If we "goto again" in cmd_subvol_delete(), and error out to out:
before re-allocating the dupdname and dupvname pointers, we''ll
double-free them.

Set them to NULL after freeing to avoid this.

Resolves-Coverity-CID: 1125944
Resolves-Coverity-CID: 1125945
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-subvolume.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 63c708e..89b90cf 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -288,6 +288,8 @@ again:
 out:
 	free(dupdname);
 	free(dupvname);
+	dupdname = NULL;
+	dupvname = NULL;
 	cnt++;
 	if (cnt < argc)
 		goto again;
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 02/16] btrfs-progs: fix error returns in get_df()

get_df returns -ERRNO, or maybe (+)errno, or even 0 in
the case where we inexplicably got 0 total_spaces from
the BTRFS_IOC_SPACE_INFO.

Consistently return a negative error number, and return
-ENOENT rather than 0 for total_spaces == 0, so that the
caller will know that **sargs_ret hasn''t been set up.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-filesystem.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 0bfd710..e6642ef 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -106,11 +106,12 @@ static int get_df(int fd, struct btrfs_ioctl_space_args
**sargs_ret)
 		fprintf(stderr, "ERROR: couldn''t get space info - %s\n",
 			strerror(e));
 		free(sargs);
-		return ret;
+		return -e;
 	}
+	/* This really should never happen */
 	if (!sargs->total_spaces) {
 		free(sargs);
-		return 0;
+		return -ENOENT;
 	}
 	count = sargs->total_spaces;
 	free(sargs);
@@ -128,7 +129,7 @@ static int get_df(int fd, struct btrfs_ioctl_space_args
**sargs_ret)
 		fprintf(stderr, "ERROR: get space info count %llu - %s\n",
 				count, strerror(e));
 		free(sargs);
-		return ret;
+		return -e;
 	}
 	*sargs_ret = sargs;
 	return 0;
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 03/16] btrfs-progs: use strncpy in btrfs_scan_lblkid()

Use strncpy(... ,PATH_MAX) to be sure we don''t overflow
the path[PATH_MAX] array.

Resolves-Coverity-CID: 1125941
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 utils.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/utils.c b/utils.c
index a7441c1..c11a7c2 100644
--- a/utils.c
+++ b/utils.c
@@ -1952,7 +1952,7 @@ int btrfs_scan_lblkid(int update_kernel)
 		if (!dev)
 			continue;
 		/* if we are here its definitly a btrfs disk*/
-		strcpy(path, blkid_dev_devname(dev));
+		strncpy(path, blkid_dev_devname(dev), PATH_MAX);
 		if (test_skip_this_disk(path))
 			continue;
 
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 04/16] btrfs-progs: fix test for return of realpath in find_mount_root()

find_mount_root() tries to test for realpath() failure, but
tests the wrong value.  Fix it.

Resolves-Coverity-CID: 1125940
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-send.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cmds-send.c b/cmds-send.c
index 39110e7..53e9a53 100644
--- a/cmds-send.c
+++ b/cmds-send.c
@@ -98,7 +98,7 @@ int find_mount_root(const char *path, char **mount_root)
 
 	ret = 0;
 	*mount_root = realpath(longest_match, NULL);
-	if (!mount_root)
+	if (!*mount_root)
 		ret = -errno;
 
 	free(longest_match);
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 05/16] btrfs-progs: don''t leak fd in test_dev_for_mkfs() error paths

Close fd before we return on error paths.

Resolves-Coverity-CID: 1125939
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 utils.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/utils.c b/utils.c
index c11a7c2..c784345 100644
--- a/utils.c
+++ b/utils.c
@@ -1905,10 +1905,12 @@ int test_dev_for_mkfs(char *file, int force_overwrite,
char *estr)
 	if (fstat(fd, &st)) {
 		snprintf(estr, sz, "unable to stat %s: %s\n", file,
 			strerror(errno));
+		close(fd);
 		return 1;
 	}
 	if (!S_ISBLK(st.st_mode)) {
 		fprintf(stderr, "''%s'' is not a block device\n",
file);
+		close(fd);
 		return 1;
 	}
 	close(fd);
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 06/16] btrfs-progs: fix leak of "buf" in make_btrfs() error paths

If any pwrite failed we leaked the allocated "buf" on
return from the function.  "goto out" takes care of
those paths.

Resolves-Coverity-CID: 1125938
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 utils.c |   62 ++++++++++++++++++++++++++++++++------------------------------
 1 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/utils.c b/utils.c
index c784345..d8ce153 100644
--- a/utils.c
+++ b/utils.c
@@ -210,10 +210,10 @@ int make_btrfs(int fd, const char *device, const char
*label,
 
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[1]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != leafsize)
-		return -EIO;
+	if (ret != leafsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
 
 	/* create the items for the extent tree */
 	memset(buf->data+sizeof(struct btrfs_header), 0,
@@ -269,10 +269,10 @@ int make_btrfs(int fd, const char *device, const char
*label,
 	btrfs_set_header_nritems(buf, nritems);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[2]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != leafsize)
-		return -EIO;
+	if (ret != leafsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
 
 	/* create the chunk tree */
 	memset(buf->data+sizeof(struct btrfs_header), 0,
@@ -356,10 +356,10 @@ int make_btrfs(int fd, const char *device, const char
*label,
 	btrfs_set_header_nritems(buf, nritems);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[3]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != leafsize)
-		return -EIO;
+	if (ret != leafsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
 
 	/* create the device tree */
 	memset(buf->data+sizeof(struct btrfs_header), 0,
@@ -395,10 +395,10 @@ int make_btrfs(int fd, const char *device, const char
*label,
 	btrfs_set_header_nritems(buf, nritems);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[4]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != leafsize)
-		return -EIO;
+	if (ret != leafsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
 
 	/* create the FS root */
 	memset(buf->data+sizeof(struct btrfs_header), 0,
@@ -408,11 +408,10 @@ int make_btrfs(int fd, const char *device, const char
*label,
 	btrfs_set_header_nritems(buf, 0);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[5]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != leafsize)
-		return -EIO;
-
+	if (ret != leafsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
 	/* finally create the csum root */
 	memset(buf->data+sizeof(struct btrfs_header), 0,
 		leafsize-sizeof(struct btrfs_header));
@@ -421,10 +420,10 @@ int make_btrfs(int fd, const char *device, const char
*label,
 	btrfs_set_header_nritems(buf, 0);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[6]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != leafsize)
-		return -EIO;
+	if (ret != leafsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
 
 	/* and write out the super block */
 	BUG_ON(sizeof(super) > sectorsize);
@@ -433,13 +432,16 @@ int make_btrfs(int fd, const char *device, const char
*label,
 	buf->len = sectorsize;
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, sectorsize, blocks[0]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != sectorsize)
-		return -EIO;
+	if (ret != sectorsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
+
+	ret = 0;
 
+out:
 	free(buf);
-	return 0;
+	return ret;
 }
 
 u64 btrfs_device_size(int fd, struct stat *st)
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 07/16] btrfs-progs: don''t leak buffer on add_file_items() error

add_file_items() leaked "buffer" on this error return.
Free it first.

Resolves-Coverity-CID: 1125937
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 mkfs.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/mkfs.c b/mkfs.c
index d576797..f29f5cd 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -629,6 +629,7 @@ static int add_file_items(struct btrfs_trans_handle *trans,
 		ret_read = pread64(fd, buffer, st->st_size, bytes_read);
 		if (ret_read == -1) {
 			fprintf(stderr, "%s read failed\n", path_name);
+			free(buffer);
 			goto end;
 		}
 
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

In the "nothing to resume" case we return directly and leak
several bits of memory; goto out to free them properly.

Resolves-Coverity-CID: 1125934
Resolves-Coverity-CID: 1125935
Resolves-Coverity-CID: 1125936
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-scrub.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/cmds-scrub.c b/cmds-scrub.c
index 605af45..5f3eade 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -1261,7 +1261,8 @@ static int scrub_start(int argc, char **argv, int resume)
 		if (!do_quiet)
 			printf("scrub: nothing to resume for %s, fsid %s\n",
 			       path, fsid);
-		return 2;
+		err = 2;
+		goto out;
 	}
 
 	ret = prg_fd = socket(AF_UNIX, SOCK_STREAM, 0);
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 09/16] btrfs-progs: btrfs_scan_kernel(): fd==0 is not an error

The error return from open is -1, so test that, not 0,
for success/failure.

Resolves-Coverity-CID: 1125931
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-filesystem.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index e6642ef..df9d1ff 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -362,13 +362,13 @@ static int btrfs_scan_kernel(void *search)
 		}
 
 		fd = open(mnt->mnt_dir, O_RDONLY);
-		if (fd > 0 && !get_df(fd, &space_info_arg)) {
+		if ((fd != -1) && !get_df(fd, &space_info_arg)) {
 			get_label_mounted(mnt->mnt_dir, label);
 			print_one_fs(&fs_info_arg, dev_info_arg,
 					space_info_arg, label, mnt->mnt_dir);
 			free(space_info_arg);
 		}
-		if (fd > 0)
+		if (fd != -1)
 			close(fd);
 		free(dev_info_arg);
 	}
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 10/16] btrfs-progs: Check for open failure in btrfs_scan_lblkid()

open can fail, of course.

Resolves-Coverity-CID: 1125925
Resolves-Coverity-CID: 1125930
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 utils.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/utils.c b/utils.c
index d8ce153..8471148 100644
--- a/utils.c
+++ b/utils.c
@@ -1955,12 +1955,16 @@ int btrfs_scan_lblkid(int update_kernel)
 		dev = blkid_verify(cache, dev);
 		if (!dev)
 			continue;
-		/* if we are here its definitly a btrfs disk*/
+		/* if we are here its definitely a btrfs disk*/
 		strncpy(path, blkid_dev_devname(dev), PATH_MAX);
 		if (test_skip_this_disk(path))
 			continue;
 
 		fd = open(path, O_RDONLY);
+		if (fd < 0) {
+			printf("ERROR: could not open %s\n", path);
+			continue;
+		}
 		btrfs_scan_one_device(fd, path, &tmp_devices,
 				&num_devices, BTRFS_SUPER_INFO_OFFSET);
 		close(fd);
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 11/16] btrfs-progs: pass positive errno to strerror in cmd_df()

get_df returns a negative error number, but then
we pass it to strerror, which wants a positive value...

Resolves-Coverity-CID: 1125929
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-filesystem.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index df9d1ff..b1291d6 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -173,7 +173,7 @@ static int cmd_df(int argc, char **argv)
 		print_df(sargs);
 		free(sargs);
 	} else {
-		fprintf(stderr, "ERROR: get_df failed %s\n", strerror(ret));
+		fprintf(stderr, "ERROR: get_df failed %s\n", strerror(-ret));
 	}
 
 	close_file_or_dir(fd, dirstream);
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 12/16] btrfs-progs: remove more dead code from check_extent_refs

e0a04278 removed a bunch of dead code but left one little
bit; reinit is always 0, so btrfs_read_block_groups is
never called from here.

Resolves-Coverity-CID: 1125926
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-check.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index 668af15..263b4ef 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -5040,7 +5040,6 @@ static int check_extent_refs(struct btrfs_trans_handle
*trans,
 	int err = 0;
 	int ret = 0;
 	int fixed = 0;
-	int reinit = 0;
 	int had_dups = 0;
 
 	if (repair) {
@@ -5066,8 +5065,6 @@ static int check_extent_refs(struct btrfs_trans_handle
*trans,
 			cache = next_cache_extent(cache);
 		}
 		prune_corrupt_blocks(trans, root->fs_info);
-		if (reinit)
-			btrfs_read_block_groups(root->fs_info->extent_root);
 		reset_cached_block_groups(root->fs_info);
 	}
 
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 13/16] btrfs-progs: check btrfs_scan_one_device in btrfs_scan_lblkid()

Even if it''s "definitely" btrfs at this point,
btrfs_scan_one_device could fail for other reasons.

Check the return value, warn if it fails, and skip
the device register.

Resolves-Coverity-CID: 1125925
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 utils.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/utils.c b/utils.c
index 8471148..ecacc29 100644
--- a/utils.c
+++ b/utils.c
@@ -1937,6 +1937,7 @@ int test_skip_this_disk(char *path)
 int btrfs_scan_lblkid(int update_kernel)
 {
 	int fd = -1;
+	int ret;
 	u64 num_devices;
 	struct btrfs_fs_devices *tmp_devices;
 	blkid_dev_iterate iter = NULL;
@@ -1965,8 +1966,14 @@ int btrfs_scan_lblkid(int update_kernel)
 			printf("ERROR: could not open %s\n", path);
 			continue;
 		}
-		btrfs_scan_one_device(fd, path, &tmp_devices,
+		ret = btrfs_scan_one_device(fd, path, &tmp_devices,
 				&num_devices, BTRFS_SUPER_INFO_OFFSET);
+		if (ret) {
+			printf("ERROR: could not scan %s\n", path);
+			close (fd);
+			continue;
+		}
+
 		close(fd);
 		if (update_kernel)
 			btrfs_register_one_device(path);
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 14/16] btrfs-progs: check for fstat failure in cmd_defrag

Resolves-Coverity-CID: 1125924
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-filesystem.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index b1291d6..2cb067d 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -661,7 +661,13 @@ static int cmd_defrag(int argc, char **argv)
 		if (recursive) {
 			struct stat st;
 
-			fstat(fd, &st);
+			if (fstat(fd, &st)) {
+				fprintf(stderr, "ERROR: failed to stat %s - %s\n",
+						argv[i], strerror(errno));
+				defrag_global_errors++;
+				close_file_or_dir(fd, dirstream);
+				continue;
+			}
 			if (S_ISDIR(st.st_mode)) {
 				ret = nftw(argv[i], defrag_callback, 10,
 						FTW_MOUNT | FTW_PHYS);
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 15/16] btrfs-progs: annotate fallthroughs in parse_size

We intentionally fall through these case statements;
just annotate it to be clear.

Resolves-Coverity-CID: 1054887
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 utils.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/utils.c b/utils.c
index ecacc29..9aeb5f8 100644
--- a/utils.c
+++ b/utils.c
@@ -1499,16 +1499,22 @@ u64 parse_size(char *s)
 		switch (c) {
 		case ''e'':
 			mult *= 1024;
+			/* fallthrough */
 		case ''p'':
 			mult *= 1024;
+			/* fallthrough */
 		case ''t'':
 			mult *= 1024;
+			/* fallthrough */
 		case ''g'':
 			mult *= 1024;
+			/* fallthrough */
 		case ''m'':
 			mult *= 1024;
+			/* fallthrough */
 		case ''k'':
 			mult *= 1024;
+			/* fallthrough */
 		case ''b'':
 			break;
 		default:
-- 
1.7.1

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

Eric Sandeen

2013-Nov-06 23:15 UTC

head link

[PATCH 16/16] btrfs-progs: annotate fallthroughs in parse_limit

We intentionally fall through these case statements;
just annotate it to be clear.

Resolves-Coverity-CID: 1054884
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-qgroup.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index 4fe776c..5a393bd 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -120,12 +120,15 @@ static int parse_limit(const char *p, unsigned long long
*s)
 	case ''T'':
 	case ''t'':
 		size *= 1024;
+		/* fallthrough */
 	case ''G'':
 	case ''g'':
 		size *= 1024;
+		/* fallthrough */
 	case ''M'':
 	case ''m'':
 		size *= 1024;
+		/* fallthrough */
 	case ''K'':
 	case ''k'':
 		size *= 1024;
-- 
1.7.1

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

Wang Shilong

2013-Nov-07 01:48 UTC

head link

Re: [PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

Hi Eric,

On 11/07/2013 07:15 AM, Eric Sandeen wrote:> In the "nothing to resume" case we return directly and leak
> several bits of memory; goto out to free them properly.
>
> Resolves-Coverity-CID: 1125934
> Resolves-Coverity-CID: 1125935
> Resolves-Coverity-CID: 1125936
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   cmds-scrub.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/cmds-scrub.c b/cmds-scrub.c
> index 605af45..5f3eade 100644
> --- a/cmds-scrub.c
> +++ b/cmds-scrub.c
> @@ -1261,7 +1261,8 @@ static int scrub_start(int argc, char **argv, int
resume)
>   		if (!do_quiet)
>   			printf("scrub: nothing to resume for %s, fsid %s\n",
>   			       path, fsid);
> -		return 2;
> +		err = 2;
> +		goto out;Thanks for tracking this problem, but
i intend to return 2 in such case originally.
return ''!err'' will revert to return 1 rather than 2.

Thanks,
Wang>   	}
>   
>   	ret = prg_fd = socket(AF_UNIX, SOCK_STREAM, 0);
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wang Shilong

2013-Nov-07 01:50 UTC

head link

Re: [PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

On 11/07/2013 09:48 AM, Wang Shilong wrote:> Hi Eric,
>
> On 11/07/2013 07:15 AM, Eric Sandeen wrote:
>> In the "nothing to resume" case we return directly and leak
>> several bits of memory; goto out to free them properly.
>>
>> Resolves-Coverity-CID: 1125934
>> Resolves-Coverity-CID: 1125935
>> Resolves-Coverity-CID: 1125936
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>   cmds-scrub.c |    3 ++-
>>   1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/cmds-scrub.c b/cmds-scrub.c
>> index 605af45..5f3eade 100644
>> --- a/cmds-scrub.c
>> +++ b/cmds-scrub.c
>> @@ -1261,7 +1261,8 @@ static int scrub_start(int argc, char **argv, 
>> int resume)
>>           if (!do_quiet)
>>               printf("scrub: nothing to resume for %s, fsid
%s\n",
>>                      path, fsid);
>> -        return 2;
>> +        err = 2;
>> +        goto out;
> Thanks for tracking this problem, but
> i intend to return 2 in such case originally.
> return ''!err'' will revert to return 1 rather than 2.see label out:

if (err)
     return 1
>
> Thanks,
> Wang
>>       }
>>         ret = prg_fd = socket(AF_UNIX, SOCK_STREAM, 0);
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe
linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Anand Jain

2013-Nov-07 02:33 UTC

head link

Re: [PATCH 02/16] btrfs-progs: fix error returns in get_df()

I had just retained whats in the original. But this is
  good change. Thanks Eric.

Reviewed-by: Anand Jain <anand.jain@oracle.com>


On 11/07/2013 07:15 AM, Eric Sandeen wrote:> get_df returns -ERRNO, or maybe (+)errno, or even 0 in
> the case where we inexplicably got 0 total_spaces from
> the BTRFS_IOC_SPACE_INFO.
>
> Consistently return a negative error number, and return
> -ENOENT rather than 0 for total_spaces == 0, so that the
> caller will know that **sargs_ret hasn''t been set up.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   cmds-filesystem.c |    7 ++++---
>   1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 0bfd710..e6642ef 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -106,11 +106,12 @@ static int get_df(int fd, struct
btrfs_ioctl_space_args **sargs_ret)
>   		fprintf(stderr, "ERROR: couldn''t get space info -
%s\n",
>   			strerror(e));
>   		free(sargs);
> -		return ret;
> +		return -e;
>   	}
> +	/* This really should never happen */
>   	if (!sargs->total_spaces) {
>   		free(sargs);
> -		return 0;
> +		return -ENOENT;
>   	}
>   	count = sargs->total_spaces;
>   	free(sargs);
> @@ -128,7 +129,7 @@ static int get_df(int fd, struct btrfs_ioctl_space_args
**sargs_ret)
>   		fprintf(stderr, "ERROR: get space info count %llu - %s\n",
>   				count, strerror(e));
>   		free(sargs);
> -		return ret;
> +		return -e;
>   	}
>   	*sargs_ret = sargs;
>   	return 0;
>--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Anand Jain

2013-Nov-07 02:43 UTC

head link

[PATCH] btrfs-progs: use strncpy in btrfs_scan_lblkid()

From: Eric Sandeen <sandeen@redhat.com>

Use strncpy(... ,PATH_MAX) to be sure we don''t overflow
the path[PATH_MAX] array.

Resolves-Coverity-CID: 1125941
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 utils.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/utils.c b/utils.c
index 753169b..ebd10e5 100644
--- a/utils.c
+++ b/utils.c
@@ -1981,8 +1981,8 @@ int btrfs_scan_lblkid(int update_kernel)
 		dev = blkid_verify(cache, dev);
 		if (!dev)
 			continue;
-		/* if we are here its definitly a btrfs disk*/
-		strcpy(path, blkid_dev_devname(dev));
+		/* if we are here its definitely a btrfs disk*/
+		strncpy(path, blkid_dev_devname(dev), PATH_MAX);
 		if (test_skip_this_disk(path))
 			continue;
 
-- 
1.7.1

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

Anand Jain

2013-Nov-07 02:55 UTC

head link

Re: [PATCH 04/16] btrfs-progs: fix test for return of realpath in find_mount_root()

Reviewed-by: Anand Jain <anand.jain@oracle.com>


On 11/07/2013 07:15 AM, Eric Sandeen wrote:> find_mount_root() tries to test for realpath() failure, but
> tests the wrong value.  Fix it.
>
> Resolves-Coverity-CID: 1125940
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   cmds-send.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/cmds-send.c b/cmds-send.c
> index 39110e7..53e9a53 100644
> --- a/cmds-send.c
> +++ b/cmds-send.c
> @@ -98,7 +98,7 @@ int find_mount_root(const char *path, char **mount_root)
>
>   	ret = 0;
>   	*mount_root = realpath(longest_match, NULL);
> -	if (!mount_root)
> +	if (!*mount_root)
>   		ret = -errno;
>
>   	free(longest_match);
>--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Anand Jain

2013-Nov-07 03:05 UTC

head link

Re: [PATCH 05/16] btrfs-progs: don''t leak fd in test_dev_for_mkfs() error paths

Reviewed-by: Anand Jain <anand.jain@oracle.com>


On 11/07/2013 07:15 AM, Eric Sandeen wrote:> Close fd before we return on error paths.
>
> Resolves-Coverity-CID: 1125939
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   utils.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/utils.c b/utils.c
> index c11a7c2..c784345 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1905,10 +1905,12 @@ int test_dev_for_mkfs(char *file, int
force_overwrite, char *estr)
>   	if (fstat(fd, &st)) {
>   		snprintf(estr, sz, "unable to stat %s: %s\n", file,
>   			strerror(errno));
> +		close(fd);
>   		return 1;
>   	}
>   	if (!S_ISBLK(st.st_mode)) {
>   		fprintf(stderr, "''%s'' is not a block
device\n", file);
> +		close(fd);
>   		return 1;
>   	}
>   	close(fd);
>--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eric Sandeen

2013-Nov-07 03:46 UTC

head link

Re: [PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

On 11/6/13, 7:50 PM, Wang Shilong wrote:> On 11/07/2013 09:48 AM, Wang Shilong wrote:
>> Hi Eric,
>>
>> On 11/07/2013 07:15 AM, Eric Sandeen wrote:
>>> In the "nothing to resume" case we return directly and
leak
>>> several bits of memory; goto out to free them properly.
>>>
>>> Resolves-Coverity-CID: 1125934
>>> Resolves-Coverity-CID: 1125935
>>> Resolves-Coverity-CID: 1125936
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>> ---
>>>   cmds-scrub.c |    3 ++-
>>>   1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/cmds-scrub.c b/cmds-scrub.c
>>> index 605af45..5f3eade 100644
>>> --- a/cmds-scrub.c
>>> +++ b/cmds-scrub.c
>>> @@ -1261,7 +1261,8 @@ static int scrub_start(int argc, char **argv,
int resume)
>>>           if (!do_quiet)
>>>               printf("scrub: nothing to resume for %s, fsid
%s\n",
>>>                      path, fsid);
>>> -        return 2;
>>> +        err = 2;
>>> +        goto out;
>> Thanks for tracking this problem, but
>> i intend to return 2 in such case originally.
>> return ''!err'' will revert to return 1 rather than 2.
> see label out:
> 
> if (err)
>     return 1
> 
Ah, whoops.  Ok, well we still need to fix the leak.  

I just expected that setting err & going to out would return err ;)

So probably:

if (err)
   return err;

will work.

I''ll send a V2.

Thanks for catching it on review!

-Eric

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

Anand Jain

2013-Nov-07 04:29 UTC

head link

Re: [PATCH 09/16] btrfs-progs: btrfs_scan_kernel(): fd==0 is not an error

Reviewed-by: Anand Jain <anand.jain@oracle.com>


On 11/07/2013 07:15 AM, Eric Sandeen wrote:> The error return from open is -1, so test that, not 0,
> for success/failure.
>
> Resolves-Coverity-CID: 1125931
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   cmds-filesystem.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index e6642ef..df9d1ff 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -362,13 +362,13 @@ static int btrfs_scan_kernel(void *search)
>   		}
>
>   		fd = open(mnt->mnt_dir, O_RDONLY);
> -		if (fd > 0 && !get_df(fd, &space_info_arg)) {
> +		if ((fd != -1) && !get_df(fd, &space_info_arg)) {
>   			get_label_mounted(mnt->mnt_dir, label);
>   			print_one_fs(&fs_info_arg, dev_info_arg,
>   					space_info_arg, label, mnt->mnt_dir);
>   			free(space_info_arg);
>   		}
> -		if (fd > 0)
> +		if (fd != -1)
>   			close(fd);
>   		free(dev_info_arg);
>   	}
>--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Anand Jain

2013-Nov-07 04:43 UTC

head link

Re: [PATCH 11/16] btrfs-progs: pass positive errno to strerror in cmd_df()

Reviewed-by Anand Jain <anand.jain@oracle.com>

On 11/07/2013 07:15 AM, Eric Sandeen wrote:> get_df returns a negative error number, but then
> we pass it to strerror, which wants a positive value...
>
> Resolves-Coverity-CID: 1125929
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   cmds-filesystem.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index df9d1ff..b1291d6 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -173,7 +173,7 @@ static int cmd_df(int argc, char **argv)
>   		print_df(sargs);
>   		free(sargs);
>   	} else {
> -		fprintf(stderr, "ERROR: get_df failed %s\n", strerror(ret));
> +		fprintf(stderr, "ERROR: get_df failed %s\n", strerror(-ret));
>   	}
>
>   	close_file_or_dir(fd, dirstream);
>--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wang Shilong

2013-Nov-07 05:06 UTC

head link

Re: [PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

On 11/07/2013 11:46 AM, Eric Sandeen wrote:> On 11/6/13, 7:50 PM, Wang Shilong wrote:
>> On 11/07/2013 09:48 AM, Wang Shilong wrote:
>>> Hi Eric,
>>>
>>> On 11/07/2013 07:15 AM, Eric Sandeen wrote:
>>>> In the "nothing to resume" case we return directly
and leak
>>>> several bits of memory; goto out to free them properly.
>>>>
>>>> Resolves-Coverity-CID: 1125934
>>>> Resolves-Coverity-CID: 1125935
>>>> Resolves-Coverity-CID: 1125936
>>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>>> ---
>>>>    cmds-scrub.c |    3 ++-
>>>>    1 files changed, 2 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/cmds-scrub.c b/cmds-scrub.c
>>>> index 605af45..5f3eade 100644
>>>> --- a/cmds-scrub.c
>>>> +++ b/cmds-scrub.c
>>>> @@ -1261,7 +1261,8 @@ static int scrub_start(int argc, char
**argv, int resume)
>>>>            if (!do_quiet)
>>>>                printf("scrub: nothing to resume for %s,
fsid %s\n",
>>>>                       path, fsid);
>>>> -        return 2;
>>>> +        err = 2;
>>>> +        goto out;
>>> Thanks for tracking this problem, but
>>> i intend to return 2 in such case originally.
>>> return ''!err'' will revert to return 1 rather than
2.
>> see label out:
>>
>> if (err)
>>      return 1
>>
> Ah, whoops.  Ok, well we still need to fix the leak.
>
> I just expected that setting err & going to out would return err ;)
>
> So probably:
>
> if (err)
>     return err;No, we can''t.
maybe add a flag, something like:

flag = 0;

/*  we set flag here for a special case */
if (!do_quient)
     printf....
flag = 2;


then in label
out:
...
/* this happen if nothing resume */
     if (flag)
         return flag
/* syntax or other error happens */
     if (err)
         return 1

Previously, ''err'' can be an casual positive numbers or error
number.
However, we define that we return 1 if syntax error happens.

Thanks,
Wang>
> will work.
>
> I''ll send a V2.
>
> Thanks for catching it on review!
>
> -Eric
>
>

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

Anand Jain

2013-Nov-07 08:34 UTC

head link

Re: [PATCH 13/16] btrfs-progs: check btrfs_scan_one_device in btrfs_scan_lblkid()

reviewed-by: Anand Jain <anand.jain@oracle.com>


On 11/07/2013 07:15 AM, Eric Sandeen wrote:> Even if it''s "definitely" btrfs at this point,
> btrfs_scan_one_device could fail for other reasons.
>
> Check the return value, warn if it fails, and skip
> the device register.
>
> Resolves-Coverity-CID: 1125925
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   utils.c |    9 ++++++++-
>   1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/utils.c b/utils.c
> index 8471148..ecacc29 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1937,6 +1937,7 @@ int test_skip_this_disk(char *path)
>   int btrfs_scan_lblkid(int update_kernel)
>   {
>   	int fd = -1;
> +	int ret;
>   	u64 num_devices;
>   	struct btrfs_fs_devices *tmp_devices;
>   	blkid_dev_iterate iter = NULL;
> @@ -1965,8 +1966,14 @@ int btrfs_scan_lblkid(int update_kernel)
>   			printf("ERROR: could not open %s\n", path);
>   			continue;
>   		}
> -		btrfs_scan_one_device(fd, path, &tmp_devices,
> +		ret = btrfs_scan_one_device(fd, path, &tmp_devices,
>   				&num_devices, BTRFS_SUPER_INFO_OFFSET);
> +		if (ret) {
> +			printf("ERROR: could not scan %s\n", path);
> +			close (fd);
> +			continue;
> +		}
> +
>   		close(fd);
>   		if (update_kernel)
>   			btrfs_register_one_device(path);
>--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Btrfs devel - Nov 2013 - [PATCH 00/16] btrfs-progs: Several more static analysis defect fixes

[PATCH 00/16] btrfs-progs: Several more static analysis defect fixes

[PATCH 01/16] btrfs-progs: fix potential double-frees in cmd_subvol_delete()

[PATCH 02/16] btrfs-progs: fix error returns in get_df()

[PATCH 03/16] btrfs-progs: use strncpy in btrfs_scan_lblkid()

[PATCH 04/16] btrfs-progs: fix test for return of realpath in find_mount_root()

[PATCH 05/16] btrfs-progs: don''t leak fd in test_dev_for_mkfs() error paths

[PATCH 06/16] btrfs-progs: fix leak of "buf" in make_btrfs() error paths

[PATCH 07/16] btrfs-progs: don''t leak buffer on add_file_items() error

[PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

[PATCH 09/16] btrfs-progs: btrfs_scan_kernel(): fd==0 is not an error

[PATCH 10/16] btrfs-progs: Check for open failure in btrfs_scan_lblkid()

[PATCH 11/16] btrfs-progs: pass positive errno to strerror in cmd_df()

[PATCH 12/16] btrfs-progs: remove more dead code from check_extent_refs

[PATCH 13/16] btrfs-progs: check btrfs_scan_one_device in btrfs_scan_lblkid()

[PATCH 14/16] btrfs-progs: check for fstat failure in cmd_defrag

[PATCH 15/16] btrfs-progs: annotate fallthroughs in parse_size

[PATCH 16/16] btrfs-progs: annotate fallthroughs in parse_limit

Re: [PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

Re: [PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

Re: [PATCH 02/16] btrfs-progs: fix error returns in get_df()

[PATCH] btrfs-progs: use strncpy in btrfs_scan_lblkid()

Re: [PATCH 04/16] btrfs-progs: fix test for return of realpath in find_mount_root()

Re: [PATCH 05/16] btrfs-progs: don''t leak fd in test_dev_for_mkfs() error paths

Re: [PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

Re: [PATCH 09/16] btrfs-progs: btrfs_scan_kernel(): fd==0 is not an error

Re: [PATCH 11/16] btrfs-progs: pass positive errno to strerror in cmd_df()

Re: [PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

Re: [PATCH 13/16] btrfs-progs: check btrfs_scan_one_device in btrfs_scan_lblkid()