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 <[email protected]>, and
Herb Wartens <[email protected]>
-----------------------------------------------------------------------------
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);
}
/*
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
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.
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
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
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.
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
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
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