[PATCH 00/25] e2fsprogs: Misc cleanups & fixes

[PATCH 00/25] e2fsprogs: Misc cleanups & fixes

[Eric Sandeen]

I ran Coverity over e2fsprogs this week, hoping to find any recent-ish
flaws, but most of what it came up with seems longstanding.  I guess
that's a good sign for the recent work, anyway!

Some of these are just tidyups, some are bugfixes.  It gets the defect
count down to around 25 from 70 to start with, but I'll need to take
a deep breath before tackling the rest.

Thanks,
-Eric

[PATCH 01/25] libext2: Fix EXT2_LIB_SOFTSUPP masking

[Eric Sandeen]

EXT2_LIB_SOFTSUPP_INCOMPAT_* are supposed to be bitmasks
of features which can be opened even though they are
under development.  The intent is that these are masked
out of the features list, so that they will be ignored
on open.

However, the code does a logical not vs. a bitwise not:

	features &= !EXT2_LIB_SOFTSUPP_INCOMPAT;

which will not have the desired effect...

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 lib/ext2fs/openfs.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
index b13db77..0b4df38 100644
--- a/lib/ext2fs/openfs.c
+++ b/lib/ext2fs/openfs.c
@@ -214,7 +214,7 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
 		features = fs->super->s_feature_incompat;
 #ifdef EXT2_LIB_SOFTSUPP_INCOMPAT
 		if (flags & EXT2_FLAG_SOFTSUPP_FEATURES)
-			features &= !EXT2_LIB_SOFTSUPP_INCOMPAT;
+			features &= ~EXT2_LIB_SOFTSUPP_INCOMPAT;
 #endif
 		if (features & ~EXT2_LIB_FEATURE_INCOMPAT_SUPP) {
 			retval = EXT2_ET_UNSUPP_FEATURE;
@@ -224,7 +224,7 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
 		features = fs->super->s_feature_ro_compat;
 #ifdef EXT2_LIB_SOFTSUPP_RO_COMPAT
 		if (flags & EXT2_FLAG_SOFTSUPP_FEATURES)
-			features &= !EXT2_LIB_SOFTSUPP_RO_COMPAT;
+			features &= ~EXT2_LIB_SOFTSUPP_RO_COMPAT;
 #endif
 		if ((flags & EXT2_FLAG_RW) &&
 		    (features & ~EXT2_LIB_FEATURE_RO_COMPAT_SUPP)) {
-- 
1.7.4.1

Re: [PATCH 01/25] libext2: Fix EXT2_LIB_SOFTSUPP masking

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:16PM -0500, Eric Sandeen wrote:
> EXT2_LIB_SOFTSUPP_INCOMPAT_* are supposed to be bitmasks
> of features which can be opened even though they are
> under development.  The intent is that these are masked
> out of the features list, so that they will be ignored
> on open.
> 
> However, the code does a logical not vs. a bitwise not:
> 
> 	features &= !EXT2_LIB_SOFTSUPP_INCOMPAT;
> 
> which will not have the desired effect...
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

						- Ted

[PATCH 02/25] e2fsprogs: Remove impossible name_len tests.

[Eric Sandeen]

The name_len field in ext2_dir_entry is actually comprised of
the name length in the lower 8 bytes, and the filetype in the
high 8 bytes.  So in places, we mask name_len with 0xFF to
get the actual length.

But once we have masked name_len with 0xFF, there is no point
in testing whether it is greater than EXT2_NAME_LEN, which
is 255 - or 0xFF.  So all of these tests are extraneous.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 debugfs/dump.c   |    3 +--
 debugfs/htree.c  |    3 +--
 debugfs/ls.c     |    3 +--
 e2fsck/message.c |    2 --
 e2fsck/pass2.c   |    6 ------
 5 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/debugfs/dump.c b/debugfs/dump.c
index 4cf0752..ec36eca 100644
--- a/debugfs/dump.c
+++ b/debugfs/dump.c
@@ -300,8 +300,7 @@ static int rdump_dirent(struct ext2_dir_entry *dirent,
 	const char *dumproot = private;
 	struct ext2_inode inode;
 
-	thislen = ((dirent->name_len & 0xFF) < EXT2_NAME_LEN
-		   ? (dirent->name_len & 0xFF) : EXT2_NAME_LEN);
+	thislen = dirent->name_len & 0xFF;
 	strncpy(name, dirent->name, thislen);
 	name[thislen] = 0;
 
diff --git a/debugfs/htree.c b/debugfs/htree.c
index b829e25..de36bd4 100644
--- a/debugfs/htree.c
+++ b/debugfs/htree.c
@@ -81,8 +81,7 @@ static void htree_dump_leaf_node(ext2_filsys fs, ext2_ino_t ino,
 				blk);
 			break;
 		}
-		thislen = ((dirent->name_len & 0xFF) < EXT2_NAME_LEN) ?
-			(dirent->name_len & 0xFF) : EXT2_NAME_LEN;
+		thislen = dirent->name_len & 0xFF;
 		strncpy(name, dirent->name, thislen);
 		name[thislen] = '\0';
 		errcode = ext2fs_dirhash(hash_alg, name,
diff --git a/debugfs/ls.c b/debugfs/ls.c
index 8e019d2..e01fd20 100644
--- a/debugfs/ls.c
+++ b/debugfs/ls.c
@@ -60,8 +60,7 @@ static int list_dir_proc(ext2_ino_t dir EXT2FS_ATTR((unused)),
 	int			thislen;
 	struct list_dir_struct *ls = (struct list_dir_struct *) private;
 
-	thislen = ((dirent->name_len & 0xFF) < EXT2_NAME_LEN) ?
-		(dirent->name_len & 0xFF) : EXT2_NAME_LEN;
+	thislen = dirent->name_len & 0xFF;
 	strncpy(name, dirent->name, thislen);
 	name[thislen] = '\0';
 	ino = dirent->inode;
diff --git a/e2fsck/message.c b/e2fsck/message.c
index 49b861d..565c3cd 100644
--- a/e2fsck/message.c
+++ b/e2fsck/message.c
@@ -372,8 +372,6 @@ static _INLINE_ void expand_dirent_expression(ext2_filsys fs, char ch,
 		break;
 	case 'n':
 		len = dirent->name_len & 0xFF;
-		if (len > EXT2_NAME_LEN)
-			len = EXT2_NAME_LEN;
 		if ((ext2fs_get_rec_len(fs, dirent, &rec_len) == 0) &&
 		    (len > rec_len))
 			len = rec_len;
diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c
index e57afb9..9c44aa2 100644
--- a/e2fsck/pass2.c
+++ b/e2fsck/pass2.c
@@ -858,12 +858,6 @@ out_htree:
 			} else
 				goto abort_free_dict;
 		}
-		if ((dirent->name_len & 0xFF) > EXT2_NAME_LEN) {
-			if (fix_problem(ctx, PR_2_FILENAME_LONG, &cd->pctx)) {
-				dirent->name_len = EXT2_NAME_LEN;
-				dir_modified++;
-			}
-		}
 
 		if (dot_state == 0) {
 			if (check_dot(ctx, dirent, ino, &cd->pctx))
-- 
1.7.4.1

Re: [PATCH 02/25] e2fsprogs: Remove impossible name_len tests.

[Andreas Dilger]

On 2011-09-16, at 2:49 PM, Eric Sandeen wrote:
> The name_len field in ext2_dir_entry is actually comprised of
> the name length in the lower 8 bytes, and the filetype in the
> high 8 bytes.  So in places, we mask name_len with 0xFF to
> get the actual length.
> 
> But once we have masked name_len with 0xFF, there is no point
> in testing whether it is greater than EXT2_NAME_LEN, which
> is 255 - or 0xFF.  So all of these tests are extraneous.

This makes me want to remove uses of ext2_dir_entry and instead use
ext2_dir_entry_2 everywhere.  That will avoid a lot of potential
errors in the code, and I think the only possible use for the old
ext2_dir_entry is for ancient big-endian filesystems that were "v1"
format and didn't have EXT2_INCOMPAT_FILETYPE (which appeared in
e2fsprogs commit 89fe431a8 from Linux 2.1.90).

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> debugfs/dump.c   |    3 +--
> debugfs/htree.c  |    3 +--
> debugfs/ls.c     |    3 +--
> e2fsck/message.c |    2 --
> e2fsck/pass2.c   |    6 ------
> 5 files changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/debugfs/dump.c b/debugfs/dump.c
> index 4cf0752..ec36eca 100644
> --- a/debugfs/dump.c
> +++ b/debugfs/dump.c
> @@ -300,8 +300,7 @@ static int rdump_dirent(struct ext2_dir_entry *dirent,
> 	const char *dumproot = private;
> 	struct ext2_inode inode;
> 
> -	thislen = ((dirent->name_len & 0xFF) < EXT2_NAME_LEN
> -		   ? (dirent->name_len & 0xFF) : EXT2_NAME_LEN);
> +	thislen = dirent->name_len & 0xFF;
> 	strncpy(name, dirent->name, thislen);
> 	name[thislen] = 0;
> 
> diff --git a/debugfs/htree.c b/debugfs/htree.c
> index b829e25..de36bd4 100644
> --- a/debugfs/htree.c
> +++ b/debugfs/htree.c
> @@ -81,8 +81,7 @@ static void htree_dump_leaf_node(ext2_filsys fs, ext2_ino_t ino,
> 				blk);
> 			break;
> 		}
> -		thislen = ((dirent->name_len & 0xFF) < EXT2_NAME_LEN) ?
> -			(dirent->name_len & 0xFF) : EXT2_NAME_LEN;
> +		thislen = dirent->name_len & 0xFF;
> 		strncpy(name, dirent->name, thislen);
> 		name[thislen] = '\0';
> 		errcode = ext2fs_dirhash(hash_alg, name,
> diff --git a/debugfs/ls.c b/debugfs/ls.c
> index 8e019d2..e01fd20 100644
> --- a/debugfs/ls.c
> +++ b/debugfs/ls.c
> @@ -60,8 +60,7 @@ static int list_dir_proc(ext2_ino_t dir EXT2FS_ATTR((unused)),
> 	int			thislen;
> 	struct list_dir_struct *ls = (struct list_dir_struct *) private;
> 
> -	thislen = ((dirent->name_len & 0xFF) < EXT2_NAME_LEN) ?
> -		(dirent->name_len & 0xFF) : EXT2_NAME_LEN;
> +	thislen = dirent->name_len & 0xFF;
> 	strncpy(name, dirent->name, thislen);
> 	name[thislen] = '\0';
> 	ino = dirent->inode;
> diff --git a/e2fsck/message.c b/e2fsck/message.c
> index 49b861d..565c3cd 100644
> --- a/e2fsck/message.c
> +++ b/e2fsck/message.c
> @@ -372,8 +372,6 @@ static _INLINE_ void expand_dirent_expression(ext2_filsys fs, char ch,
> 		break;
> 	case 'n':
> 		len = dirent->name_len & 0xFF;
> -		if (len > EXT2_NAME_LEN)
> -			len = EXT2_NAME_LEN;
> 		if ((ext2fs_get_rec_len(fs, dirent, &rec_len) == 0) &&
> 		    (len > rec_len))
> 			len = rec_len;
> diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c
> index e57afb9..9c44aa2 100644
> --- a/e2fsck/pass2.c
> +++ b/e2fsck/pass2.c
> @@ -858,12 +858,6 @@ out_htree:
> 			} else
> 				goto abort_free_dict;
> 		}
> -		if ((dirent->name_len & 0xFF) > EXT2_NAME_LEN) {
> -			if (fix_problem(ctx, PR_2_FILENAME_LONG, &cd->pctx)) {
> -				dirent->name_len = EXT2_NAME_LEN;
> -				dir_modified++;
> -			}
> -		}
> 
> 		if (dot_state == 0) {
> 			if (check_dot(ctx, dirent, ino, &cd->pctx))
> -- 
> 1.7.4.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas

Re: [PATCH 02/25] e2fsprogs: Remove impossible name_len tests.

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:17PM -0500, Eric Sandeen wrote:
> The name_len field in ext2_dir_entry is actually comprised of
> the name length in the lower 8 bytes, and the filetype in the
> high 8 bytes.  So in places, we mask name_len with 0xFF to
> get the actual length.
> 
> But once we have masked name_len with 0xFF, there is no point
> in testing whether it is greater than EXT2_NAME_LEN, which
> is 255 - or 0xFF.  So all of these tests are extraneous.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 03/25] fsck: fix -C option parsing

[Eric Sandeen]

The i++; statement is unreachable; fix same as commit
f1c2eaac535bd9172a35ce39b6d8f392321f274d in util-linux

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 misc/fsck.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/misc/fsck.c b/misc/fsck.c
index fe9426e..9345233 100644
--- a/misc/fsck.c
+++ b/misc/fsck.c
@@ -1179,8 +1179,8 @@ static void PRS(int argc, char *argv[])
 					if (progress_fd < 0)
 						progress_fd = 0;
 					else {
+						++i;
 						goto next_arg;
-						i++;
 					}
 				}
 				break;
-- 
1.7.4.1

Re: [PATCH 03/25] fsck: fix -C option parsing

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:18PM -0500, Eric Sandeen wrote:
> The i++; statement is unreachable; fix same as commit
> f1c2eaac535bd9172a35ce39b6d8f392321f274d in util-linux
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 04/25] mke2fs: remove impossible tests for null usage_types

[Eric Sandeen]

parse_fs_type explicitly sets usage_types if it is null,
so there is no need to test for null later.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 misc/mke2fs.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index abcfbf0..437b495 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1029,15 +1029,12 @@ static char **parse_fs_type(const char *fs_type,
 	if (!usage_types)
 		usage_types = size_type;
 
-	parse_str = malloc(usage_types ? strlen(usage_types)+1 : 1);
+	parse_str = malloc(strlen(usage_types)+1);
 	if (!parse_str) {
 		free(list.list);
 		return 0;
 	}
-	if (usage_types)
-		strcpy(parse_str, usage_types);
-	else
-		*parse_str = '\0';
+	strcpy(parse_str, usage_types);
 
 	if (ext_type)
 		push_string(&list, ext_type);
-- 
1.7.4.1

Re: [PATCH 04/25] mke2fs: remove impossible tests for null usage_types

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:19PM -0500, Eric Sandeen wrote:
> parse_fs_type explicitly sets usage_types if it is null,
> so there is no need to test for null later.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 05/25] libext2: move buf variable completely under ifdef

[Eric Sandeen]

If !WORDS_BIGENDIAN, it is pointless to test whether buf
is NULL, because it is initialized to NULL and never changed.
This makes Coverity complain, so we can just move all handling
of "buf" under the #ifdef.

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

diff --git a/lib/ext2fs/ext_attr.c b/lib/ext2fs/ext_attr.c
index 52664eb..ddcc89f 100644
--- a/lib/ext2fs/ext_attr.c
+++ b/lib/ext2fs/ext_attr.c
@@ -82,9 +82,9 @@ errcode_t ext2fs_write_ext_attr2(ext2_filsys fs, blk64_t block, void *inbuf)
 {
 	errcode_t	retval;
 	char		*write_buf;
+#ifdef WORDS_BIGENDIAN
 	char		*buf = NULL;
 
-#ifdef WORDS_BIGENDIAN
 	retval = ext2fs_get_mem(fs->blocksize, &buf);
 	if (retval)
 		return retval;
@@ -94,8 +94,9 @@ errcode_t ext2fs_write_ext_attr2(ext2_filsys fs, blk64_t block, void *inbuf)
 	write_buf = (char *) inbuf;
 #endif
 	retval = io_channel_write_blk64(fs->io, block, 1, write_buf);
-	if (buf)
-		ext2fs_free_mem(&buf);
+#ifdef WORDS_BIGENDIAN
+	ext2fs_free_mem(&buf);
+#endif
 	if (!retval)
 		ext2fs_mark_changed(fs);
 	return retval;
-- 
1.7.4.1

Re: [PATCH 05/25] libext2: move buf variable completely under ifdef

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:20PM -0500, Eric Sandeen wrote:
> If !WORDS_BIGENDIAN, it is pointless to test whether buf
> is NULL, because it is initialized to NULL and never changed.
> This makes Coverity complain, so we can just move all handling
> of "buf" under the #ifdef.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 06/25] libext2fs: Potential null ptr deref in undo_err_handler_init

[Eric Sandeen]

In the !undo_io_backing_manager case, undo_err_handler_init
will be passed a null data->real, which will be dereferenced.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 lib/ext2fs/undo_io.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/lib/ext2fs/undo_io.c b/lib/ext2fs/undo_io.c
index 454f3b6..da1cf45 100644
--- a/lib/ext2fs/undo_io.c
+++ b/lib/ext2fs/undo_io.c
@@ -400,7 +400,8 @@ static errcode_t undo_open(const char *name, int flags, io_channel *channel)
 	 * setup err handler for read so that we know
 	 * when the backing manager fails do short read
 	 */
-	undo_err_handler_init(data->real);
+	if (data->real)
+		undo_err_handler_init(data->real);
 
 	*channel = io;
 	return 0;
-- 
1.7.4.1

Re: [PATCH 06/25] libext2fs: Potential null ptr deref in undo_err_handler_init

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:21PM -0500, Eric Sandeen wrote:
> In the !undo_io_backing_manager case, undo_err_handler_init
> will be passed a null data->real, which will be dereferenced.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

				- Ted

[PATCH 07/25] e2fsck: handle null fs in print_pathname()

[Eric Sandeen]

testing fs for NULL in expand_percent_expression():

	e2fsck_ctx = fs ? (e2fsck_t) fs->priv_data : NULL;

implies that fs could be NULL, but it's passed to print_pathname()
which defererences it without further testing.

So make this safe by returning "???" for a nul fs.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 e2fsck/message.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/e2fsck/message.c b/e2fsck/message.c
index 565c3cd..f178321 100644
--- a/e2fsck/message.c
+++ b/e2fsck/message.c
@@ -203,8 +203,9 @@ static void print_pathname(ext2_filsys fs, ext2_ino_t dir, ext2_ino_t ino)
 		return;
 	}
 
-	retval = ext2fs_get_pathname(fs, dir, ino, &path);
-	if (retval)
+	if (fs)
+		retval = ext2fs_get_pathname(fs, dir, ino, &path);
+	if (!fs || retval)
 		fputs("???", stdout);
 	else {
 		safe_print(path, -1);
-- 
1.7.4.1

Re: [PATCH 07/25] e2fsck: handle null fs in print_pathname()

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:22PM -0500, Eric Sandeen wrote:
> testing fs for NULL in expand_percent_expression():
> 
> 	e2fsck_ctx = fs ? (e2fsck_t) fs->priv_data : NULL;
> 
> implies that fs could be NULL, but it's passed to print_pathname()
> which defererences it without further testing.
> 
> So make this safe by returning "???" for a nul fs.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 08/25] e2fsprogs: annotate intentional fallthroughs in case statements

[Eric Sandeen]

Using the /* fallthrough */ comment lets Coverity (and humans)
know that we really do want to fall through in these case statements.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 lib/e2p/feature.c    |    1 +
 lib/e2p/mntopts.c    |    1 +
 lib/e2p/parse_num.c  |    3 +++
 lib/ext2fs/dirhash.c |    3 +++
 lib/ext2fs/extent.c  |    2 ++
 5 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/lib/e2p/feature.c b/lib/e2p/feature.c
index 5188483..ef4376a 100644
--- a/lib/e2p/feature.c
+++ b/lib/e2p/feature.c
@@ -307,6 +307,7 @@ int e2p_edit_feature2(const char *str, __u32 *compat_array, __u32 *ok_array,
 		case '-':
 		case '^':
 			neg++;
+			/* fallthrough */
 		case '+':
 			cp++;
 			break;
diff --git a/lib/e2p/mntopts.c b/lib/e2p/mntopts.c
index dfdd0ef..9d3879e 100644
--- a/lib/e2p/mntopts.c
+++ b/lib/e2p/mntopts.c
@@ -122,6 +122,7 @@ int e2p_edit_mntopts(const char *str, __u32 *mntopts, __u32 ok)
 		case '-':
 		case '^':
 			neg++;
+			/* fallthrough */
 		case '+':
 			cp++;
 			break;
diff --git a/lib/e2p/parse_num.c b/lib/e2p/parse_num.c
index 83a329a..9ca6799 100644
--- a/lib/e2p/parse_num.c
+++ b/lib/e2p/parse_num.c
@@ -26,10 +26,13 @@ unsigned long long parse_num_blocks2(const char *arg, int log_block_size)
 	switch (*p) {		/* Using fall-through logic */
 	case 'T': case 't':
 		num <<= 10;
+		/* fallthrough */
 	case 'G': case 'g':
 		num <<= 10;
+		/* fallthrough */
 	case 'M': case 'm':
 		num <<= 10;
+		/* fallthrough */
 	case 'K': case 'k':
 		num >>= log_block_size;
 		break;
diff --git a/lib/ext2fs/dirhash.c b/lib/ext2fs/dirhash.c
index a069706..5477bfb 100644
--- a/lib/ext2fs/dirhash.c
+++ b/lib/ext2fs/dirhash.c
@@ -217,11 +217,13 @@ errcode_t ext2fs_dirhash(int version, const char *name, int len,
 	switch (version) {
 	case EXT2_HASH_LEGACY_UNSIGNED:
 		unsigned_flag++;
+		/* fallthrough */
 	case EXT2_HASH_LEGACY:
 		hash = dx_hack_hash(name, len, unsigned_flag);
 		break;
 	case EXT2_HASH_HALF_MD4_UNSIGNED:
 		unsigned_flag++;
+		/* fallthrough */
 	case EXT2_HASH_HALF_MD4:
 		p = name;
 		while (len > 0) {
@@ -235,6 +237,7 @@ errcode_t ext2fs_dirhash(int version, const char *name, int len,
 		break;
 	case EXT2_HASH_TEA_UNSIGNED:
 		unsigned_flag++;
+		/* fallthrough */
 	case EXT2_HASH_TEA:
 		p = name;
 		while (len > 0) {
diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index abb60dd..6d2afb0 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -373,9 +373,11 @@ retry:
 	case EXT2_EXTENT_ROOT:
 		handle->level = 0;
 		path = handle->path + handle->level;
+		/* fallthrough */
 	case EXT2_EXTENT_FIRST_SIB:
 		path->left = path->entries;
 		path->curr = 0;
+		/* fallthrough */
 	case EXT2_EXTENT_NEXT_SIB:
 		if (path->left <= 0)
 			return EXT2_ET_EXTENT_NO_NEXT;
-- 
1.7.4.1

Re: [PATCH 08/25] e2fsprogs: annotate intentional fallthroughs in case statements

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:23PM -0500, Eric Sandeen wrote:
> Using the /* fallthrough */ comment lets Coverity (and humans)
> know that we really do want to fall through in these case statements.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 09/25] uuidd: Add missing break to option case statement

[Eric Sandeen]

Specifying the "-n" option to uuidd would incorrectly
fall through to the "-p" case, and assign that number to
the pidfile_path.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 misc/uuidd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/misc/uuidd.c b/misc/uuidd.c
index 89bff72..912773a 100644
--- a/misc/uuidd.c
+++ b/misc/uuidd.c
@@ -472,6 +472,7 @@ int main(int argc, char **argv)
 				fprintf(stderr, _("Bad number: %s\n"), optarg);
 				exit(1);
 			}
+			break;
 		case 'p':
 			pidfile_path = optarg;
 			drop_privs = 1;
-- 
1.7.4.1

Re: [PATCH 09/25] uuidd: Add missing break to option case statement

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:24PM -0500, Eric Sandeen wrote:
> Specifying the "-n" option to uuidd would incorrectly
> fall through to the "-p" case, and assign that number to
> the pidfile_path.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

Can you do me a favor and make sure this gets fixed in util-linux-ng
as well?

	 			- Ted

Re: [PATCH 09/25] uuidd: Add missing break to option case statement

[Eric Sandeen]

On 9/16/11 5:54 PM, Ted Ts'o wrote:
> On Fri, Sep 16, 2011 at 03:49:24PM -0500, Eric Sandeen wrote:
>> Specifying the "-n" option to uuidd would incorrectly
>> fall through to the "-p" case, and assign that number to
>> the pidfile_path.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Applied, thanks.
> 
> Can you do me a favor and make sure this gets fixed in util-linux-ng
> as well?

sure thing

-eric

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

[PATCH 10/25] freefrag: fix up getopt case statement

[Eric Sandeen]

There is no need to print out a "bad option" message; getopt
does that for us, and in fact will change "c" to "?" so
it's not even useful.

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

diff --git a/misc/e2freefrag.c b/misc/e2freefrag.c
index e6fd67e..419c6fc 100644
--- a/misc/e2freefrag.c
+++ b/misc/e2freefrag.c
@@ -282,10 +282,8 @@ int main(int argc, char *argv[])
 			}
 			chunk_info.chunkbytes *= 1024;
 			break;
-		default:
-			fprintf(stderr, "%s: bad option '%c'\n",
-				progname, c);
 		case 'h':
+		default:
 			usage(progname);
 			break;
 		}
-- 
1.7.4.1

Re: [PATCH 10/25] freefrag: fix up getopt case statement

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:25PM -0500, Eric Sandeen wrote:
> There is no need to print out a "bad option" message; getopt
> does that for us, and in fact will change "c" to "?" so
> it's not even useful.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 11/25] libe2p: reach unreachable code

[Eric Sandeen]

The EOPNOTSUPP case is unreachable, being outside a set of:
 #if
	...
	return;
 #else
	...
	return;
 #endif

Fix this up so that if neither HAVE_CHFLAGS nor
HAVE_EXT2_IOCTLS applies, we set EOPNOTSUPP.

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

diff --git a/lib/e2p/setflags.c b/lib/e2p/setflags.c
index 72cf441..b203606 100644
--- a/lib/e2p/setflags.c
+++ b/lib/e2p/setflags.c
@@ -55,7 +55,7 @@ int setflags (int fd, unsigned long flags)
 #endif
 
 	return fchflags (fd, bsd_flags);
-#else
+#else /* ! HAVE_CHFLAGS */
 #if HAVE_EXT2_IOCTLS
 	struct stat buf;
 	int	f;
@@ -68,8 +68,9 @@ int setflags (int fd, unsigned long flags)
 	f = (int) flags;
 
 	return ioctl(fd, EXT2_IOC_SETFLAGS, &f);
-#endif /* HAVE_EXT2_IOCTLS */
-#endif
+#else
 	errno = EOPNOTSUPP;
 	return -1;
+#endif /* HAVE_EXT2_IOCTLS */
+#endif /* HAVE_CHFLAGS */
 }
-- 
1.7.4.1

Re: [PATCH 11/25] libe2p: reach unreachable code

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:26PM -0500, Eric Sandeen wrote:
> The EOPNOTSUPP case is unreachable, being outside a set of:
>  #if
> 	...
> 	return;
>  #else
> 	...
> 	return;
>  #endif
> 
> Fix this up so that if neither HAVE_CHFLAGS nor
> HAVE_EXT2_IOCTLS applies, we set EOPNOTSUPP.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 12/25] e2fsck: Don't store old_op from ehandler_operation if we don't restore it.

[Eric Sandeen]

old_op is set but never used, because we restore "0"
not old_op.  So don't bother with it.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 e2fsck/pass1.c |    3 +--
 e2fsck/pass2.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index 16ddcda..4c3fe3f 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -402,7 +402,6 @@ static void check_is_really_dir(e2fsck_t ctx, struct problem_context *pctx,
 {
 	struct ext2_inode *inode = pctx->inode;
 	struct ext2_dir_entry 	*dirent;
-	const char		*old_op;
 	errcode_t		retval;
 	blk64_t			blk;
 	unsigned int		i, rec_len, not_device = 0;
@@ -472,7 +471,7 @@ static void check_is_really_dir(e2fsck_t ctx, struct problem_context *pctx,
 		return;
 
 	/* read the first block */
-	old_op = ehandler_operation(_("reading directory block"));
+	ehandler_operation(_("reading directory block"));
 	retval = ext2fs_read_dir_block3(ctx->fs, blk, buf, 0);
 	ehandler_operation(0);
 	if (retval)
diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c
index 9c44aa2..43ba5b2 100644
--- a/e2fsck/pass2.c
+++ b/e2fsck/pass2.c
@@ -711,7 +711,6 @@ static int check_dir_block(ext2_filsys fs,
 	struct ext2_dir_entry 	*dirent, *prev;
 	ext2_dirhash_t		hash;
 	unsigned int		offset = 0;
-	const char *		old_op;
 	int			dir_modified = 0;
 	int			dot_state;
 	unsigned int		rec_len;
@@ -774,7 +773,7 @@ static int check_dir_block(ext2_filsys fs,
 	       db->blockcnt, ino);
 #endif
 
-	old_op = ehandler_operation(_("reading directory block"));
+	ehandler_operation(_("reading directory block"));
 	cd->pctx.errcode = ext2fs_read_dir_block3(fs, block_nr, buf, 0);
 	ehandler_operation(0);
 	if (cd->pctx.errcode == EXT2_ET_DIR_CORRUPTED)
-- 
1.7.4.1

Re: [PATCH 12/25] e2fsck: Don't store old_op from ehandler_operation if we don't restore it.

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:27PM -0500, Eric Sandeen wrote:
> old_op is set but never used, because we restore "0"
> not old_op.  So don't bother with it.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

				- Ted

[PATCH 13/25] e2fsprogs: Fix some error cleanup path bugs

[Eric Sandeen]

In inode_open(), if the allocation of &io fails, we go to cleanup
and dereference io to test io->name, which is a bug.

Similarly in undo_open()  if allocation of &data fails, we
go to cleanup and dereference data to test data->real.

In the test_open() case we explicitly set retval to the only
possible error return from ext2fs_get_mem(), so remove that
for tidiness.

The other changes just make make earlier returns go through
the error goto for consistency.

In many cases we returned directly from the first error, but
"goto cleanup" etc for every subsequent error.  In some
cases this leads to "impossible" tests such as:

	if (ptr)
		ext2fs_free_mem(&ptr)

on paths where ptr cannot be null because we would have
returned directly earlier, and Coverity flags this.

This isn't really indicative of an error in most cases, but
I think it can be clearer to always exit through the error goto
if it's used later in the function.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 lib/ext2fs/dblist.c   |    2 +-
 lib/ext2fs/inode.c    |    6 ++++--
 lib/ext2fs/inode_io.c |    2 +-
 lib/ext2fs/test_io.c  |    6 ++----
 lib/ext2fs/undo_io.c  |    4 ++--
 lib/ext2fs/unix_io.c  |    2 +-
 6 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/lib/ext2fs/dblist.c b/lib/ext2fs/dblist.c
index c0a3dfe..ead8c7a 100644
--- a/lib/ext2fs/dblist.c
+++ b/lib/ext2fs/dblist.c
@@ -73,7 +73,7 @@ static errcode_t make_dblist(ext2_filsys fs, ext2_ino_t size,
 
 	retval = ext2fs_get_mem(sizeof(struct ext2_struct_dblist), &dblist);
 	if (retval)
-		return retval;
+		goto cleanup;
 	memset(dblist, 0, sizeof(struct ext2_struct_dblist));
 
 	dblist->magic = EXT2_ET_MAGIC_DBLIST;
diff --git a/lib/ext2fs/inode.c b/lib/ext2fs/inode.c
index 829e032..7889a9f 100644
--- a/lib/ext2fs/inode.c
+++ b/lib/ext2fs/inode.c
@@ -671,8 +671,10 @@ errcode_t ext2fs_write_inode_full(ext2_filsys fs, ext2_ino_t ino,
 
 	if (length > (int) sizeof(struct ext2_inode_large)) {
 		w_inode = malloc(length);
-		if (!w_inode)
-			return ENOMEM;
+		if (!w_inode) {
+			retval = ENOMEM;
+			goto errout;
+		}
 	} else
 		w_inode = &temp_inode;
 	memset(w_inode, 0, length);
diff --git a/lib/ext2fs/inode_io.c b/lib/ext2fs/inode_io.c
index b3e7ce5..ced3244 100644
--- a/lib/ext2fs/inode_io.c
+++ b/lib/ext2fs/inode_io.c
@@ -163,7 +163,7 @@ static errcode_t inode_open(const char *name, int flags, io_channel *channel)
 	return 0;
 
 cleanup:
-	if (io->name)
+	if (io && io->name)
 		ext2fs_free_mem(&io->name);
 	if (data)
 		ext2fs_free_mem(&data);
diff --git a/lib/ext2fs/test_io.c b/lib/ext2fs/test_io.c
index 7da1ee6..1a6d6c2 100644
--- a/lib/ext2fs/test_io.c
+++ b/lib/ext2fs/test_io.c
@@ -190,14 +190,12 @@ static errcode_t test_open(const char *name, int flags, io_channel *channel)
 		return EXT2_ET_BAD_DEVICE_NAME;
 	retval = ext2fs_get_mem(sizeof(struct struct_io_channel), &io);
 	if (retval)
-		return retval;
+		goto cleanup;
 	memset(io, 0, sizeof(struct struct_io_channel));
 	io->magic = EXT2_ET_MAGIC_IO_CHANNEL;
 	retval = ext2fs_get_mem(sizeof(struct test_private_data), &data);
-	if (retval) {
-		retval = EXT2_ET_NO_MEMORY;
+	if (retval)
 		goto cleanup;
-	}
 	io->manager = test_io_manager;
 	retval = ext2fs_get_mem(strlen(name)+1, &io->name);
 	if (retval)
diff --git a/lib/ext2fs/undo_io.c b/lib/ext2fs/undo_io.c
index da1cf45..df55abf 100644
--- a/lib/ext2fs/undo_io.c
+++ b/lib/ext2fs/undo_io.c
@@ -357,7 +357,7 @@ static errcode_t undo_open(const char *name, int flags, io_channel *channel)
 		return EXT2_ET_BAD_DEVICE_NAME;
 	retval = ext2fs_get_mem(sizeof(struct struct_io_channel), &io);
 	if (retval)
-		return retval;
+		goto cleanup;
 	memset(io, 0, sizeof(struct struct_io_channel));
 	io->magic = EXT2_ET_MAGIC_IO_CHANNEL;
 	retval = ext2fs_get_mem(sizeof(struct undo_private_data), &data);
@@ -407,7 +407,7 @@ static errcode_t undo_open(const char *name, int flags, io_channel *channel)
 	return 0;
 
 cleanup:
-	if (data->real)
+	if (data && data->real)
 		io_channel_close(data->real);
 	if (data)
 		ext2fs_free_mem(&data);
diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
index ecddfa6..7eb32f4 100644
--- a/lib/ext2fs/unix_io.c
+++ b/lib/ext2fs/unix_io.c
@@ -453,7 +453,7 @@ static errcode_t unix_open(const char *name, int flags, io_channel *channel)
 		return EXT2_ET_BAD_DEVICE_NAME;
 	retval = ext2fs_get_mem(sizeof(struct struct_io_channel), &io);
 	if (retval)
-		return retval;
+		goto cleanup;
 	memset(io, 0, sizeof(struct struct_io_channel));
 	io->magic = EXT2_ET_MAGIC_IO_CHANNEL;
 	retval = ext2fs_get_mem(sizeof(struct unix_private_data), &data);
-- 
1.7.4.1

Re: [PATCH 13/25] e2fsprogs: Fix some error cleanup path bugs

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:28PM -0500, Eric Sandeen wrote:
> In inode_open(), if the allocation of &io fails, we go to cleanup
> and dereference io to test io->name, which is a bug.
> 
> Similarly in undo_open()  if allocation of &data fails, we
> go to cleanup and dereference data to test data->real.
> 
> In the test_open() case we explicitly set retval to the only
> possible error return from ext2fs_get_mem(), so remove that
> for tidiness.
> 
> The other changes just make make earlier returns go through
> the error goto for consistency.
> 
> In many cases we returned directly from the first error, but
> "goto cleanup" etc for every subsequent error.  In some
> cases this leads to "impossible" tests such as:
> 
> 	if (ptr)
> 		ext2fs_free_mem(&ptr)
> 
> on paths where ptr cannot be null because we would have
> returned directly earlier, and Coverity flags this.
> 
> This isn't really indicative of an error in most cases, but
> I think it can be clearer to always exit through the error goto
> if it's used later in the function.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 14/25] subst: Fix free of uninit pointers

[Eric Sandeen]

in add_subst(), if the malloc of ent->name fails, we goto fail;
which will free ent->name (which is null, so OK) but also free
ent->value (which is uninitialized).  There is no case where
we must free ent->value on an error (it is allocated last, and
if it fails it of course doesn't need to be freed) so just
remove it.

Also "retval" is only assigned once to the constant ENOMEM,
so we can just return that explicitly in the failure case.

Signed-off-by: Eric Saneeen <sandeen@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 util/subst.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/util/subst.c b/util/subst.c
index b55f54b..8544b6d 100644
--- a/util/subst.c
+++ b/util/subst.c
@@ -35,9 +35,7 @@ struct subst_entry *subst_table = 0;
 static int add_subst(char *name, char *value)
 {
 	struct subst_entry	*ent = 0;
-	int	retval;
 
-	retval = ENOMEM;
 	ent = (struct subst_entry *) malloc(sizeof(struct subst_entry));
 	if (!ent)
 		goto fail;
@@ -55,10 +53,9 @@ static int add_subst(char *name, char *value)
 fail:
 	if (ent) {
 		free(ent->name);
-		free(ent->value);
 		free(ent);
 	}
-	return retval;
+	return ENOMEM;
 }
 
 static struct subst_entry *fetch_subst_entry(char *name)
-- 
1.7.4.1

Re: [PATCH 14/25] subst: Fix free of uninit pointers

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:29PM -0500, Eric Sandeen wrote:
> in add_subst(), if the malloc of ent->name fails, we goto fail;
> which will free ent->name (which is null, so OK) but also free
> ent->value (which is uninitialized).  There is no case where
> we must free ent->value on an error (it is allocated last, and
> if it fails it of course doesn't need to be freed) so just
> remove it.
> 
> Also "retval" is only assigned once to the constant ENOMEM,
> so we can just return that explicitly in the failure case.
> 
> Signed-off-by: Eric Saneeen <sandeen@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, although I removed the double sign off.  :-)

	 	    	    	- Ted

[PATCH 15/25] filefrag: Fix uninitialized "expected" value

[Eric Sandeen]

The "count" variable is only ever set if FIBMAP is used,
due to the -B switch, or a fiemap failure.  However,
we use it unconditionally to calculate "expected" for
extN files, so we can end up printing garbage.

Initialize count to 0, and unless we go through the FIBMAP
path, expected will be 0 as well, and in that case do not
print the message.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 misc/filefrag.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/misc/filefrag.c b/misc/filefrag.c
index 2795e15..b055c2b 100644
--- a/misc/filefrag.c
+++ b/misc/filefrag.c
@@ -272,7 +272,7 @@ static void frag_report(const char *filename)
 #endif
 	int		bs;
 	long		fd;
-	unsigned long	block, last_block = 0, numblocks, i, count;
+	unsigned long	block, last_block = 0, numblocks, i, count = 0;
 	long		bpib;	/* Blocks per indirect block */
 	long		cylgroups;
 	int		num_extents = 0, expected;
@@ -373,8 +373,9 @@ static void frag_report(const char *filename)
 		printf("%s: 1 extent found", filename);
 	else
 		printf("%s: %d extents found", filename, num_extents);
+	/* count, and thus expected, only set for indirect FIBMAP'd files */
 	expected = (count/((bs*8)-(fsinfo.f_files/8/cylgroups)-3))+1;
-	if (is_ext2 && expected < num_extents)
+	if (is_ext2 && expected && expected < num_extents)
 		printf(", perfection would be %d extent%s\n", expected,
 			(expected>1) ? "s" : "");
 	else
-- 
1.7.4.1

Re: [PATCH 15/25] filefrag: Fix uninitialized "expected" value

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:30PM -0500, Eric Sandeen wrote:
> The "count" variable is only ever set if FIBMAP is used,
> due to the -B switch, or a fiemap failure.  However,
> we use it unconditionally to calculate "expected" for
> extN files, so we can end up printing garbage.
> 
> Initialize count to 0, and unless we go through the FIBMAP
> path, expected will be 0 as well, and in that case do not
> print the message.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 16/25] e2fsck: remove extraneous memset

[Eric Sandeen]

e2fsck_allocate_memory() already sets allocated memory to 0,
so remove the explicit memset.

Especially since it was setting the wrong size (iter not *iter)

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 e2fsck/dirinfo.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/e2fsck/dirinfo.c b/e2fsck/dirinfo.c
index 901235c..ace5b4d 100644
--- a/e2fsck/dirinfo.c
+++ b/e2fsck/dirinfo.c
@@ -318,7 +318,6 @@ extern struct dir_info_iter *e2fsck_dir_info_iter_begin(e2fsck_t ctx)
 
 	iter = e2fsck_allocate_memory(ctx, sizeof(struct dir_info_iter),
 				      "dir_info iterator");
-	memset(iter, 0, sizeof(iter));
 
 	if (db->tdb)
 		iter->tdb_iter = tdb_firstkey(db->tdb);
-- 
1.7.4.1

Re: [PATCH 16/25] e2fsck: remove extraneous memset

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:31PM -0500, Eric Sandeen wrote:
> e2fsck_allocate_memory() already sets allocated memory to 0,
> so remove the explicit memset.
> 
> Especially since it was setting the wrong size (iter not *iter)
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 17/25] mke2fs: Do not let -t or -T be specified more than once

[Eric Sandeen]

In addition to not making since, it causes a memory leak
when fs_type gets overwritten.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 misc/mke2fs.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 437b495..e30c070 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1445,9 +1445,19 @@ profile_error:
 			super_only = 1;
 			break;
 		case 't':
+			if (fs_type) {
+				com_err(program_name, 0, 
+					_("The -t option may only be used once"));
+				exit(1);
+			}
 			fs_type = strdup(optarg);
 			break;
 		case 'T':
+			if (usage_types) {
+				com_err(program_name, 0, 
+					_("The -T option may only be used once"));
+				exit(1);
+			}
 			usage_types = strdup(optarg);
 			break;
 		case 'U':
-- 
1.7.4.1

Re: [PATCH 17/25] mke2fs: Do not let -t or -T be specified more than once

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:32PM -0500, Eric Sandeen wrote:
> In addition to not making since, it causes a memory leak
> when fs_type gets overwritten.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, with whitespace cleanups.

					- Ted

Re: [PATCH 17/25] mke2fs: Do not let -t or -T be specified more than once

[Ted Ts'o]

On Fri, Sep 16, 2011 at 06:57:20PM -0400, Ted Ts'o wrote:
> On Fri, Sep 16, 2011 at 03:49:32PM -0500, Eric Sandeen wrote:
> > In addition to not making since, it causes a memory leak
> > when fs_type gets overwritten.
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Applied, with whitespace cleanups.

Oh, and s/since/sense/

					- Ted

Re: [PATCH 17/25] mke2fs: Do not let -t or -T be specified more than once

[Eric Sandeen]

On 9/16/11 5:57 PM, Ted Ts'o wrote:
> On Fri, Sep 16, 2011 at 06:57:20PM -0400, Ted Ts'o wrote:
>> On Fri, Sep 16, 2011 at 03:49:32PM -0500, Eric Sandeen wrote:
>>> In addition to not making since, it causes a memory leak
>>> when fs_type gets overwritten.
>>>
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>
>> Applied, with whitespace cleanups.
> 
> Oh, and s/since/sense/
> 
> 					- Ted

whoops thanks.

Usually my English is ok, sorry!  ;)

[PATCH 18/25] e2initrd_helper: Fix memory leak on error

[Eric Sandeen]

Some error paths did not properly free "buf"

And the normal exit seemed to close e2_file twice (?)

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 misc/e2initrd_helper.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/misc/e2initrd_helper.c b/misc/e2initrd_helper.c
index eaf9ce6..3d9f5ab 100644
--- a/misc/e2initrd_helper.c
+++ b/misc/e2initrd_helper.c
@@ -73,7 +73,7 @@ static errcode_t get_file(ext2_filsys fs, const char * filename,
 {
 	errcode_t	retval;
 	char 		*buf;
-	ext2_file_t	e2_file;
+	ext2_file_t	e2_file = NULL;
 	unsigned int	got;
 	struct ext2_inode inode;
 	ext2_ino_t	ino;
@@ -101,7 +101,7 @@ static errcode_t get_file(ext2_filsys fs, const char * filename,
 
 	retval = ext2fs_file_open(fs, ino, 0, &e2_file);
 	if (retval)
-		return retval;
+		goto errout;
 
 	retval = ext2fs_file_read(e2_file, buf, inode.i_size, &got);
 	if (retval)
@@ -109,13 +109,16 @@ static errcode_t get_file(ext2_filsys fs, const char * filename,
 
 	retval = ext2fs_file_close(e2_file);
 	if (retval)
-		return retval;
+		goto errout;
 
 	ret_file->buf = buf;
 	ret_file->size = (int) got;
+	return 0;
 
 errout:
-	ext2fs_file_close(e2_file);
+	free(buf);
+	if (e2_file)
+		ext2fs_file_close(e2_file);
 	return retval;
 }
 
-- 
1.7.4.1

Re: [PATCH 18/25] e2initrd_helper: Fix memory leak on error

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:33PM -0500, Eric Sandeen wrote:
> Some error paths did not properly free "buf"
> 
> And the normal exit seemed to close e2_file twice (?)
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 19/25] libext2: Fix leaks in write_bitmaps on error returns

[Eric Sandeen]

block_buf and/or inode_buf may not be properly freed on an error
return.

Create a new errout: target to free them as needed in error conditions.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 lib/ext2fs/rw_bitmaps.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/lib/ext2fs/rw_bitmaps.c b/lib/ext2fs/rw_bitmaps.c
index f8c8a9f..fd8ac10 100644
--- a/lib/ext2fs/rw_bitmaps.c
+++ b/lib/ext2fs/rw_bitmaps.c
@@ -34,7 +34,7 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
 	int		block_nbytes, inode_nbytes;
 	unsigned int	nbits;
 	errcode_t	retval;
-	char 		*block_buf, *inode_buf;
+	char 		*block_buf = NULL, *inode_buf = NULL;
 	int		csum_flag = 0;
 	blk64_t		blk;
 	blk64_t		blk_itr = EXT2FS_B2C(fs, fs->super->s_first_data_block);
@@ -55,7 +55,7 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
 		retval = ext2fs_get_memalign(fs->blocksize, fs->blocksize,
 					     &block_buf);
 		if (retval)
-			return retval;
+			goto errout;
 		memset(block_buf, 0xff, fs->blocksize);
 	}
 	if (do_inode) {
@@ -64,7 +64,7 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
 		retval = ext2fs_get_memalign(fs->blocksize, fs->blocksize,
 					     &inode_buf);
 		if (retval)
-			return retval;
+			goto errout;
 		memset(inode_buf, 0xff, fs->blocksize);
 	}
 
@@ -79,7 +79,7 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
 		retval = ext2fs_get_block_bitmap_range2(fs->block_map,
 				blk_itr, block_nbytes << 3, block_buf);
 		if (retval)
-			return retval;
+			goto errout;
 
 		if (i == fs->group_desc_count - 1) {
 			/* Force bitmap padding for the last group */
@@ -95,8 +95,10 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
 		if (blk) {
 			retval = io_channel_write_blk64(fs->io, blk, 1,
 							block_buf);
-			if (retval)
-				return EXT2_ET_BLOCK_BITMAP_WRITE;
+			if (retval) {
+				retval = EXT2_ET_BLOCK_BITMAP_WRITE;
+				goto errout;
+			}
 		}
 	skip_this_block_bitmap:
 		blk_itr += block_nbytes << 3;
@@ -112,14 +114,16 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
 		retval = ext2fs_get_inode_bitmap_range2(fs->inode_map,
 				ino_itr, inode_nbytes << 3, inode_buf);
 		if (retval)
-			return retval;
+			goto errout;
 
 		blk = ext2fs_inode_bitmap_loc(fs, i);
 		if (blk) {
 			retval = io_channel_write_blk64(fs->io, blk, 1,
 						      inode_buf);
-			if (retval)
-				return EXT2_ET_INODE_BITMAP_WRITE;
+			if (retval) {
+				retval = EXT2_ET_INODE_BITMAP_WRITE;
+				goto errout;
+			}
 		}
 	skip_this_inode_bitmap:
 		ino_itr += inode_nbytes << 3;
@@ -134,6 +138,12 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
 		ext2fs_free_mem(&inode_buf);
 	}
 	return 0;
+errout:
+	if (inode_buf)
+		ext2fs_free_mem(&inode_buf);
+	if (block_buf)
+		ext2fs_free_mem(&block_buf);
+	return retval;
 }
 
 static errcode_t read_bitmaps(ext2_filsys fs, int do_inode, int do_block)
-- 
1.7.4.1

Re: [PATCH 19/25] libext2: Fix leaks in write_bitmaps on error returns

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:34PM -0500, Eric Sandeen wrote:
> block_buf and/or inode_buf may not be properly freed on an error
> return.
> 
> Create a new errout: target to free them as needed in error conditions.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 20/25] test_icount: fclose() before exit

[Eric Sandeen]

Just to be tidy.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 tests/progs/test_icount.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/tests/progs/test_icount.c b/tests/progs/test_icount.c
index 777a812..017cc4d 100644
--- a/tests/progs/test_icount.c
+++ b/tests/progs/test_icount.c
@@ -289,6 +289,8 @@ static int source_file(const char *cmd_file, int sci_idx)
 			exit_status++;
 		}
 	}
+	if (f != stdin)
+		fclose(f);
 	return exit_status;
 }
 
-- 
1.7.4.1

Re: [PATCH 20/25] test_icount: fclose() before exit

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:35PM -0500, Eric Sandeen wrote:
> Just to be tidy.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 21/25] e2fsck: Fix leaks in error paths

[Eric Sandeen]

fn and/or array was not freed in some error paths.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 e2fsck/profile.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/e2fsck/profile.c b/e2fsck/profile.c
index 327bfb4..c790623 100644
--- a/e2fsck/profile.c
+++ b/e2fsck/profile.c
@@ -276,6 +276,7 @@ static errcode_t get_dirlist(const char *dirname, char***ret_array)
 			new_array = realloc(array, sizeof(char *) * (max+1));
 			if (!new_array) {
 				retval = ENOMEM;
+				free(fn);
 				goto errout;
 			}
 			array = new_array;
@@ -345,6 +346,7 @@ profile_init(const char **files, profile_t *ret_profile)
 	     * If all the files were not found, return the appropriate error.
 	     */
 	    if (!profile->first_file) {
+		free_list(array);
 		profile_release(profile);
 		return ENOENT;
 	    }
-- 
1.7.4.1

Re: [PATCH 21/25] e2fsck: Fix leaks in error paths

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:36PM -0500, Eric Sandeen wrote:
> fn and/or array was not freed in some error paths.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, but I did make a two changes.

> @@ -345,6 +346,7 @@ profile_init(const char **files, profile_t *ret_profile)
>  	     * If all the files were not found, return the appropriate error.
>  	     */
>  	    if (!profile->first_file) {
> +		free_list(array);
>  		profile_release(profile);
>  		return ENOENT;
>  	    }

I changed this to be:

 	    if (!profile->first_file) {
 		retval = ENOENT;
		goto errout;
 	    }

Which allows us to unify the error cleanup path and avoid duplicating
code.

Also, I noticed another bug while I was validating your patch.  If the
realloc() in get_dirlist() fails due to a ENOMEM, and we jump to
errout, the array hasn't been null terminated yet.  This could lead to
lead a kernel oops or worse when free_list(array) tries to free the
array.

So I added:

errout:
+	if (array)
+	   array[num] = 0;

to take care of this problem.

				- Ted

commit 04bfa75f42a5adb3510551f4d153526d94be37fb
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Fri Sep 16 15:49:36 2011 -0500

    e2fsck: Fix leaks in error paths
    
    fn and/or array was not freed in some error paths.
    
    [ Also make sure the array is NULL terminated before we free it in
      get_dirlist(). --tytso]
    
    Signed-off-by: Eric Sandeen <sandeen@redhat.com>
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/e2fsck/profile.c b/e2fsck/profile.c
index 327bfb4..f4267b1 100644
--- a/e2fsck/profile.c
+++ b/e2fsck/profile.c
@@ -276,6 +276,7 @@ static errcode_t get_dirlist(const char *dirname, char***ret_array)
 			new_array = realloc(array, sizeof(char *) * (max+1));
 			if (!new_array) {
 				retval = ENOMEM;
+				free(fn);
 				goto errout;
 			}
 			array = new_array;
@@ -290,6 +291,8 @@ static errcode_t get_dirlist(const char *dirname, char***ret_array)
 	closedir(dir);
 	return 0;
 errout:
+	if (array)
+		array[num] = 0;
 	closedir(dir);
 	free_list(array);
 	return retval;
@@ -345,8 +348,8 @@ profile_init(const char **files, profile_t *ret_profile)
 	     * If all the files were not found, return the appropriate error.
 	     */
 	    if (!profile->first_file) {
-		profile_release(profile);
-		return ENOENT;
+		retval = ENOENT;
+		goto errout;
 	    }
 	}
 

Re: [PATCH 21/25] e2fsck: Fix leaks in error paths

[Eric Sandeen]

On 9/16/11 6:41 PM, Ted Ts'o wrote:
> On Fri, Sep 16, 2011 at 03:49:36PM -0500, Eric Sandeen wrote:
>> fn and/or array was not freed in some error paths.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Applied, but I did make a two changes.
> 
>> @@ -345,6 +346,7 @@ profile_init(const char **files, profile_t *ret_profile)
>>  	     * If all the files were not found, return the appropriate error.
>>  	     */
>>  	    if (!profile->first_file) {
>> +		free_list(array);
>>  		profile_release(profile);
>>  		return ENOENT;
>>  	    }
> 
> I changed this to be:
> 
>  	    if (!profile->first_file) {
>  		retval = ENOENT;
> 		goto errout;
>  	    }
> 
> Which allows us to unify the error cleanup path and avoid duplicating
> code.

Makes sense, now why didn't I do that ... was thinking there was some issue
but sure looks right now!

> Also, I noticed another bug while I was validating your patch.  If the
> realloc() in get_dirlist() fails due to a ENOMEM, and we jump to
> errout, the array hasn't been null terminated yet.  This could lead to
> lead a kernel oops or worse when free_list(array) tries to free the
> array.
> 
> So I added:
> 
> errout:
> +	if (array)
> +	   array[num] = 0;
> 
> to take care of this problem.
> 
> 				- Ted
> 
> commit 04bfa75f42a5adb3510551f4d153526d94be37fb
> Author: Eric Sandeen <sandeen@redhat.com>
> Date:   Fri Sep 16 15:49:36 2011 -0500
> 
>     e2fsck: Fix leaks in error paths
>     
>     fn and/or array was not freed in some error paths.
>     
>     [ Also make sure the array is NULL terminated before we free it in
>       get_dirlist(). --tytso]
>     
>     Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>     Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> 
> diff --git a/e2fsck/profile.c b/e2fsck/profile.c
> index 327bfb4..f4267b1 100644
> --- a/e2fsck/profile.c
> +++ b/e2fsck/profile.c
> @@ -276,6 +276,7 @@ static errcode_t get_dirlist(const char *dirname, char***ret_array)
>  			new_array = realloc(array, sizeof(char *) * (max+1));
>  			if (!new_array) {
>  				retval = ENOMEM;
> +				free(fn);
>  				goto errout;
>  			}
>  			array = new_array;
> @@ -290,6 +291,8 @@ static errcode_t get_dirlist(const char *dirname, char***ret_array)
>  	closedir(dir);
>  	return 0;
>  errout:
> +	if (array)
> +		array[num] = 0;
>  	closedir(dir);
>  	free_list(array);
>  	return retval;
> @@ -345,8 +348,8 @@ profile_init(const char **files, profile_t *ret_profile)
>  	     * If all the files were not found, return the appropriate error.
>  	     */
>  	    if (!profile->first_file) {
> -		profile_release(profile);
> -		return ENOENT;
> +		retval = ENOENT;
> +		goto errout;
>  	    }
>  	}
>  

[PATCH 22/25] tune2fs: handle inode and/or block bitmap read failures in resize_inode()

[Eric Sandeen]

Handle these failures in resize_inode, and handle the propagated
error in the caller.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 misc/tune2fs.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index 6d879f3..346e2d1 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -1589,8 +1589,16 @@ static int resize_inode(ext2_filsys fs, unsigned long new_size)
 	int new_ino_blks_per_grp;
 	ext2fs_block_bitmap bmap;
 
-	ext2fs_read_inode_bitmap(fs);
-	ext2fs_read_block_bitmap(fs);
+	retval = ext2fs_read_inode_bitmap(fs);
+	if (retval) {
+		fputs(_("Failed to read inode bitmap\n"), stderr);
+		return retval;
+	}
+	retval = ext2fs_read_block_bitmap(fs);
+	if (retval) {
+		fputs(_("Failed to read blockbitmap\n"), stderr);
+		return retval;
+	}
 	INIT_LIST_HEAD(&blk_move_list);
 
 
@@ -1989,6 +1997,9 @@ retry_open:
 		if (resize_inode(fs, new_inode_size) == 0) {
 			printf(_("Setting inode size %lu\n"),
 							new_inode_size);
+		} else {
+			printf(_("Failed to change inode size\n"));
+			exit(1);
 		}
 	}
 
-- 
1.7.4.1

Re: [PATCH 22/25] tune2fs: handle inode and/or block bitmap read failures in resize_inode()

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:37PM -0500, Eric Sandeen wrote:
> Handle these failures in resize_inode, and handle the propagated
> error in the caller.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 23/25] e2fsprogs: Don't try to close an fd which is negative

[Eric Sandeen]

These reflect either file descriptors which aren't tested
for failure, or closures of fd's which may have failed.

In setup_tdb(), test for failure of mkstemp and return
without trying to open the file (again).

In reserve_stdio_fds, rather than closing the "extra"
fd == 3 due to the way the loop is written, just
don't go that far by using while (fd <= 2).

In logsave, it forks and retries forever if open fails,
but at least make coverity happy by explicitly not
trying to close a negative file descriptor.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 e2fsck/dirinfo.c |    4 ++++
 e2fsck/unix.c    |   11 ++++++-----
 misc/logsave.c   |    3 ++-
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/e2fsck/dirinfo.c b/e2fsck/dirinfo.c
index ace5b4d..ca73c31 100644
--- a/e2fsck/dirinfo.c
+++ b/e2fsck/dirinfo.c
@@ -62,6 +62,10 @@ static void setup_tdb(e2fsck_t ctx, ext2_ino_t num_dirs)
 	uuid_unparse(ctx->fs->super->s_uuid, uuid);
 	sprintf(db->tdb_fn, "%s/%s-dirinfo-XXXXXX", tdb_dir, uuid);
 	fd = mkstemp(db->tdb_fn);
+	if (fd < 0) {
+		db->tdb = NULL;
+		return;
+	}
 	db->tdb = tdb_open(db->tdb_fn, 0, TDB_CLEAR_IF_FIRST,
 			   O_RDWR | O_CREAT | O_TRUNC, 0600);
 	close(fd);
diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index bc18d41..a787d39 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -542,14 +542,16 @@ static int e2fsck_update_progress(e2fsck_t ctx, int pass,
 
 #define PATH_SET "PATH=/sbin"
 
+/*
+ * Make sure 0,1,2 file descriptors are open, so that we don't open
+ * the filesystem using the same file descriptor as stdout or stderr.
+ */
 static void reserve_stdio_fds(void)
 {
-	int	fd;
+	int	fd = 0;
 
-	while (1) {
+	while (fd <= 2) {
 		fd = open("/dev/null", O_RDWR);
-		if (fd > 2)
-			break;
 		if (fd < 0) {
 			fprintf(stderr, _("ERROR: Couldn't open "
 				"/dev/null (%s)\n"),
@@ -557,7 +559,6 @@ static void reserve_stdio_fds(void)
 			break;
 		}
 	}
-	close(fd);
 }
 
 #ifdef HAVE_SIGNAL_H
diff --git a/misc/logsave.c b/misc/logsave.c
index 74e09f7..17457a5 100644
--- a/misc/logsave.c
+++ b/misc/logsave.c
@@ -325,7 +325,8 @@ int main(int argc, char **argv)
 		write_all(outfd, outbuf, outbufsize);
 		free(outbuf);
 	}
-	close(outfd);
+	if (outfd >= 0)
+		close(outfd);
 
 	exit(rc);
 }
-- 
1.7.4.1

Re: [PATCH 23/25] e2fsprogs: Don't try to close an fd which is negative

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:38PM -0500, Eric Sandeen wrote:
> These reflect either file descriptors which aren't tested
> for failure, or closures of fd's which may have failed.
> 
> In setup_tdb(), test for failure of mkstemp and return
> without trying to open the file (again).
> 
> In reserve_stdio_fds, rather than closing the "extra"
> fd == 3 due to the way the loop is written, just
> don't go that far by using while (fd <= 2).
> 
> In logsave, it forks and retries forever if open fails,
> but at least make coverity happy by explicitly not
> trying to close a negative file descriptor.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 24/25] e4defrag: Check error return of sysconf()

[Eric Sandeen]

In theory sysconf() can fail, so check for an error return.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 misc/e4defrag.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/misc/e4defrag.c b/misc/e4defrag.c
index eea3057..4ade180 100644
--- a/misc/e4defrag.c
+++ b/misc/e4defrag.c
@@ -399,13 +399,16 @@ static int calc_entry_counts(const char *file EXT2FS_ATTR((unused)),
 static int page_in_core(int fd, struct move_extent defrag_data,
 			unsigned char **vec, unsigned int *page_num)
 {
-	long	pagesize = sysconf(_SC_PAGESIZE);
+	long	pagesize;
 	void	*page = NULL;
 	loff_t	offset, end_offset, length;
 
 	if (vec == NULL || *vec != NULL)
 		return -1;
 
+ 	pagesize = sysconf(_SC_PAGESIZE);
+	if (pagesize < 0)
+		return -1;
 	/* In mmap, offset should be a multiple of the page size */
 	offset = (loff_t)defrag_data.orig_start * block_size;
 	length = (loff_t)defrag_data.len * block_size;
-- 
1.7.4.1

Re: [PATCH 24/25] e4defrag: Check error return of sysconf()

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:39PM -0500, Eric Sandeen wrote:
> In theory sysconf() can fail, so check for an error return.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks.

					- Ted

[PATCH 25/25] mke2fs: free tdb_dir string if it came from the profile

[Eric Sandeen]

if tdb_dir points to a string allocated from profile_get_string,
it should be freed again before we exit.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 misc/mke2fs.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index e30c070..3076830 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -2024,18 +2024,21 @@ open_err_out:
 static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 {
 	errcode_t retval = ENOMEM;
-	char *tdb_dir, *tdb_file = NULL;
+	char *tdb_dir = NULL, *tdb_file = NULL;
 	char *device_name, *tmp_name;
+	int free_tdb_dir = 0;
 
 	/*
 	 * Configuration via a conf file would be
 	 * nice
 	 */
 	tdb_dir = getenv("E2FSPROGS_UNDO_DIR");
-	if (!tdb_dir)
+	if (!tdb_dir) {
 		profile_get_string(profile, "defaults",
 				   "undo_dir", 0, "/var/lib/e2fsprogs",
 				   &tdb_dir);
+		free_tdb_dir = 1;
+	}
 
 	if (!strcmp(tdb_dir, "none") || (tdb_dir[0] == 0) ||
 	    access(tdb_dir, W_OK))
@@ -2069,10 +2072,14 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 		 "using the command:\n"
 		 "    e2undo %s %s\n\n"), tdb_file, name);
 
+	if (free_tdb_dir)
+		free(tdb_dir);
 	free(tdb_file);
 	return 0;
 
 errout:
+	if (free_tdb_dir)
+		free(tdb_dir);
 	free(tdb_file);
 	com_err(program_name, retval,
 		_("while trying to setup undo file\n"));
-- 
1.7.4.1

Re: [PATCH 25/25] mke2fs: free tdb_dir string if it came from the profile

[Ted Ts'o]

On Fri, Sep 16, 2011 at 03:49:40PM -0500, Eric Sandeen wrote:
> if tdb_dir points to a string allocated from profile_get_string,
> it should be freed again before we exit.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Applied, thanks!

						- Ted

Re: [PATCH 00/25] e2fsprogs: Misc cleanups & fixes

[Eric Sandeen]

On 9/16/11 3:49 PM, Eric Sandeen wrote:
> I ran Coverity over e2fsprogs this week, hoping to find any recent-ish
> flaws, but most of what it came up with seems longstanding.  I guess
> that's a good sign for the recent work, anyway!
> 
> Some of these are just tidyups, some are bugfixes.  It gets the defect
> count down to around 25 from 70 to start with, but I'll need to take
> a deep breath before tackling the rest.

Heh, and I should have pulled from github prior to doing all this.  :(

I take back that comment about recent work, at least slightly.

Is the quota library really full of void functions which cannot return
errors?  My fault for not reviewing while it was on the list, sorry.

-Eric

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

Re: [PATCH 00/25] e2fsprogs: Misc cleanups & fixes

[Ted Ts'o]

On Fri, Sep 16, 2011 at 04:27:46PM -0500, Eric Sandeen wrote:
> 
> Heh, and I should have pulled from github prior to doing all this.  :(
> 
> I take back that comment about recent work, at least slightly.
> 
> Is the quota library really full of void functions which cannot return
> errors?  My fault for not reviewing while it was on the list, sorry.

Yeah, the quota library is going to need a lot of fixups before 1.42
ships.  I don't consider the ABI (or the function naming, since it
doesn't have any kind of namespace discpline) fixed in stone yet.
Perhaps I shouldn't have integrated the patches right away but we'll
get it fixed up before 1.42 goes out the door.

       	     	       	    	     	 - Ted

Re: [PATCH 00/25] e2fsprogs: Misc cleanups & fixes

[Eric Sandeen]

On 9/16/11 5:05 PM, Ted Ts'o wrote:
> On Fri, Sep 16, 2011 at 04:27:46PM -0500, Eric Sandeen wrote:
>>
>> Heh, and I should have pulled from github prior to doing all this.  :(
>>
>> I take back that comment about recent work, at least slightly.
>>
>> Is the quota library really full of void functions which cannot return
>> errors?  My fault for not reviewing while it was on the list, sorry.
> 
> Yeah, the quota library is going to need a lot of fixups before 1.42
> ships.  I don't consider the ABI (or the function naming, since it
> doesn't have any kind of namespace discpline) fixed in stone yet.
> Perhaps I shouldn't have integrated the patches right away but we'll
> get it fixed up before 1.42 goes out the door.

sounds like a plan ... though too bad that's One More Thing which
must be done prior to a 64-bit-capable release :(

by the way, any plans to put a new -WIP-tagged tarball anywhere?

Thanks,
-Eric

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

Re: [PATCH 00/25] e2fsprogs: Misc cleanups & fixes

[Ted Ts'o]

On Fri, Sep 16, 2011 at 05:27:09PM -0500, Eric Sandeen wrote:
> 
> sounds like a plan ... though too bad that's One More Thing which
> must be done prior to a 64-bit-capable release :(
> 
> by the way, any plans to put a new -WIP-tagged tarball anywhere?

Sorry, I hadn't gotten around to sending out an announcement yet.
Since kernel.org is down, I've gone back to using the old sourceforge
distribution method:

http://sourceforge.net/projects/e2fsprogs/files/e2fsprogs-TEST/1.42-WIP-0916/

As always, the tarball is GPG-signed, and the git tag is als
GPG-signed, from these two sites:

	git://github.com/tytso/e2fsprogs.git
	git://e2fsprogs.git.sourceforge.net/gitroot/e2fsprogs/e2fsprogs

BTW, I already know that the 0916 release is broken on big-endian
machines.  A Debian developer filed a FTBFS bug in hours.  That's
fixed on the next branch that I'll be pushing out shortly, along with
your patches.

						- Ted

Re: [PATCH 00/25] e2fsprogs: Misc cleanups & fixes

[Eric Sandeen]

On 9/16/11 6:49 PM, Ted Ts'o wrote:
> On Fri, Sep 16, 2011 at 05:27:09PM -0500, Eric Sandeen wrote:
>>
>> sounds like a plan ... though too bad that's One More Thing which
>> must be done prior to a 64-bit-capable release :(
>>
>> by the way, any plans to put a new -WIP-tagged tarball anywhere?
> 
> Sorry, I hadn't gotten around to sending out an announcement yet.
> Since kernel.org is down, I've gone back to using the old sourceforge
> distribution method:
> 
> http://sourceforge.net/projects/e2fsprogs/files/e2fsprogs-TEST/1.42-WIP-0916/
> 
> As always, the tarball is GPG-signed, and the git tag is als
> GPG-signed, from these two sites:
> 
> 	git://github.com/tytso/e2fsprogs.git
> 	git://e2fsprogs.git.sourceforge.net/gitroot/e2fsprogs/e2fsprogs
> 
> BTW, I already know that the 0916 release is broken on big-endian
> machines.  A Debian developer filed a FTBFS bug in hours.  That's
> fixed on the next branch that I'll be pushing out shortly, along with
> your patches.
> 
> 						- Ted
> 

Thanks, will that be another -WIP then?

As soon as there's a nice -WIP release I'll push a new one to rawhide
as well.

-Eric