e2fsprogs coverity patch <cid-33.diff>

e2fsprogs coverity patch <cid-33.diff>

[Brian D. Behlendorf]

Lawrence Livermore National Labs recently ran the source code
analysis tool Coverity over the e2fsprogs-1.39 source to see 
if it would identify any significant bugs.  The analysis
turned up 38 mostly minor issues which are enumerated here
with patches.  We went through and resolved these issues
but would love to see these mostly minor changes reviewed
and commited upstream.

Thanks,
Brian Behlendorf <behlendorf1@llnl.gov>, and
Herb Wartens <wartens2@llnl.gov>

-----------------------------------------------------------------------------
Coverity ID: 33: Resource Leak

The memory allocated by buf is not reclaimed.  This patch
addresses the issue.

Index: e2fsprogs+chaos/misc/util.c
===================================================================
--- e2fsprogs+chaos.orig/misc/util.c
+++ e2fsprogs+chaos/misc/util.c
@@ -176,7 +176,7 @@ void check_mount(const char *device, int
 
 void parse_journal_opts(const char *opts)
 {
-	char	*buf, *token, *next, *p, *arg;
+	char	*buf = NULL, *token, *next, *p, *arg;
 	int	len;
 	int	journal_usage = 0;
 
@@ -234,8 +234,11 @@ void parse_journal_opts(const char *opts
 			"\tdevice=<journal device>\n\n"
 			"The journal size must be between "
 			"1024 and 102400 filesystem blocks.\n\n"), stderr);
+		free(buf);
 		exit(1);
 	}
+
+	free(buf);
 }	
 
 /*

Re: e2fsprogs coverity patch <cid-33.diff>

[Eric Sandeen]

Brian D. Behlendorf wrote:
> Lawrence Livermore National Labs recently ran the source code
> analysis tool Coverity over the e2fsprogs-1.39 source to see 
> if it would identify any significant bugs.  The analysis
> turned up 38 mostly minor issues which are enumerated here
> with patches.  We went through and resolved these issues
> but would love to see these mostly minor changes reviewed
> and commited upstream.

Did cid-34.diff get lost?

-Eric

Re: e2fsprogs coverity patch <cid-33.diff>

[Andreas Dilger]

On May 29, 2007  13:49 -0500, Eric Sandeen wrote:
> Brian D. Behlendorf wrote:
> >Lawrence Livermore National Labs recently ran the source code
> >analysis tool Coverity over the e2fsprogs-1.39 source to see 
> >if it would identify any significant bugs.  The analysis
> >turned up 38 mostly minor issues which are enumerated here
> >with patches.  We went through and resolved these issues
> >but would love to see these mostly minor changes reviewed
> >and commited upstream.
> 
> Did cid-34.diff get lost?

I still have it in my "apply atop 1.39-WIP" series, so it appears not
to have made it into Ted's repo.  I'm including the patch again for
posterity.

=========================================================================
Coverity ID: 34: Resource Leak

The memory allocated by buf is not reclaimed.  This patch
addresses the issue.

Index: e2fsprogs+chaos/misc/mke2fs.c
===================================================================
--- e2fsprogs+chaos.orig/misc/mke2fs.c
+++ e2fsprogs+chaos/misc/mke2fs.c
@@ -749,7 +749,7 @@ static int set_os(struct ext2_super_bloc
 static void parse_extended_opts(struct ext2_super_block *param, 
 				const char *opts)
 {
-	char	*buf, *token, *next, *p, *arg;
+	char	*buf = NULL, *token, *next, *p, *arg;
 	int	len;
 	int	r_usage = 0;
 
@@ -834,6 +834,7 @@ static void parse_extended_opts(struct e
 				if (param->s_rev_level == EXT2_GOOD_OLD_REV) {
 					fprintf(stderr, 
 	_("On-line resizing not supported with revision 0 filesystems\n"));
+					free(buf);
 					exit(1);
 				}
 				param->s_feature_compat |=
@@ -852,8 +853,11 @@ static void parse_extended_opts(struct e
 			"Valid extended options are:\n"
 			"\tstride=<stride length in blocks>\n"
 			"\tresize=<resize maximum size in blocks>\n\n"));
+		free(buf);
 		exit(1);
 	}
+
+	free(buf);
 }	
 
 static __u32 ok_features[3] = {
=========================================================================

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Re: e2fsprogs coverity patch <cid-33.diff>

[Eric Sandeen]

Andreas Dilger wrote:
> On May 29, 2007  13:49 -0500, Eric Sandeen wrote:
>> Brian D. Behlendorf wrote:
>>> Lawrence Livermore National Labs recently ran the source code
>>> analysis tool Coverity over the e2fsprogs-1.39 source to see 
>>> if it would identify any significant bugs.  The analysis
>>> turned up 38 mostly minor issues which are enumerated here
>>> with patches.  We went through and resolved these issues
>>> but would love to see these mostly minor changes reviewed
>>> and commited upstream.
>> Did cid-34.diff get lost?
> 
> I still have it in my "apply atop 1.39-WIP" series, so it appears not
> to have made it into Ted's repo.  I'm including the patch again for
> posterity.

Thanks Andreas - near as I can tell, it never made it to the list.

-Eric

Re: e2fsprogs coverity patch <cid-33.diff>

[Theodore Tso]

On Tue, May 29, 2007 at 03:56:44PM -0500, Eric Sandeen wrote:
> >I still have it in my "apply atop 1.39-WIP" series, so it appears not
> >to have made it into Ted's repo.  I'm including the patch again for
> >posterity.
> 
> Thanks Andreas - near as I can tell, it never made it to the list.

Yeah, I wondered about that numbering discontinuity --- but IIRC it
wasn't the only one.  I had assumed that the "missing" cid's were ones
were human examination of the Coverity report lead to the conclusion
that it really wasn't a problem....

					- Ted

Re: e2fsprogs coverity patch <cid-33.diff>

[Andreas Dilger]

On May 29, 2007  18:20 -0400, Theodore Tso wrote:
> On Tue, May 29, 2007 at 03:56:44PM -0500, Eric Sandeen wrote:
> > >I still have it in my "apply atop 1.39-WIP" series, so it appears not
> > >to have made it into Ted's repo.  I'm including the patch again for
> > >posterity.
> > 
> > Thanks Andreas - near as I can tell, it never made it to the list.
> 
> Yeah, I wondered about that numbering discontinuity --- but IIRC it
> wasn't the only one.  I had assumed that the "missing" cid's were ones
> were human examination of the Coverity report lead to the conclusion
> that it really wasn't a problem....

I also have another outstanding patch:

===========================================================================
Coverity ID: 6: Forward Null

At the second conditional iter->file could still be NULL. We need to check for
it again.

Index: e2fsprogs+chaos/e2fsck/profile.c
===================================================================
--- e2fsprogs+chaos.orig/e2fsck/profile.c
+++ e2fsprogs+chaos/e2fsck/profile.c
@@ -1260,7 +1260,7 @@ errcode_t profile_node_iterator(void **i
 	 * If the file has changed, then the node pointer is invalid,
 	 * so we'll have search the file again looking for it.
 	 */
-	if (iter->node && (iter->file->upd_serial != iter->file_serial)) {
+	if (iter->node && (iter->file && iter->file->upd_serial != iter->file_serial)) {
 		iter->flags &= ~PROFILE_ITER_FINAL_SEEN;
 		skip_num = iter->num;
 		iter->node = 0;
===========================================================================

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Re: e2fsprogs coverity patch <cid-33.diff>

[Eric Sandeen]

Theodore Tso wrote:
> On Tue, May 29, 2007 at 03:56:44PM -0500, Eric Sandeen wrote:
>>> I still have it in my "apply atop 1.39-WIP" series, so it appears not
>>> to have made it into Ted's repo.  I'm including the patch again for
>>> posterity.
>> Thanks Andreas - near as I can tell, it never made it to the list.
> 
> Yeah, I wondered about that numbering discontinuity --- but IIRC it
> wasn't the only one.  I had assumed that the "missing" cid's were ones
> were human examination of the Coverity report lead to the conclusion
> that it really wasn't a problem....
> 
> 					- Ted

I only knew about it because I had a RHEL bug with it attached :)

Thanks,
-Eric

Re: e2fsprogs coverity patch <cid-33.diff>

[Theodore Tso]

On Tue, May 29, 2007 at 02:51:41PM -0600, Andreas Dilger wrote:
> > Did cid-34.diff get lost?
> 
> I still have it in my "apply atop 1.39-WIP" series, so it appears not
> to have made it into Ted's repo.  I'm including the patch again for
> posterity.

Fix applied.

						- Ted

Re: e2fsprogs coverity patch <cid-33.diff>

[Theodore Tso]

On Tue, May 29, 2007 at 05:10:25PM -0600, Andreas Dilger wrote:
> I also have another outstanding patch:
> 
> ===========================================================================
> Coverity ID: 6: Forward Null
> 
> At the second conditional iter->file could still be NULL. We need to
> check for it again.

Also applied.

						- Ted