2007-02-10 02:14:35

by Brian Behlendorf

[permalink] [raw]
Subject: e2fsprogs coverity patch <cid-3.diff>

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: 3: Deadcode

blkid_dev_has_tag() will immediately return -1 (an error if value is NULL. Thus
at the test later on value cannot be NULL. There are two possible ways to go
about fixing this. The first would be to remove the first NULL check for value.
The second one would be to remove the second check (and the deadcode). I could
be persuaded either way, but to preserve existing functionality I will opt to
remove the second check.

Index: e2fsprogs+chaos/lib/blkid/tag.c
===================================================================
--- e2fsprogs+chaos.orig/lib/blkid/tag.c
+++ e2fsprogs+chaos/lib/blkid/tag.c
@@ -91,8 +91,6 @@ extern int blkid_dev_has_tag(blkid_dev d
return -1;

tag = blkid_find_tag_dev(dev, type);
- if (!value)
- return(tag != NULL);
if (!tag || strcmp(tag->bit_val, value))
return 0;
return 1;


2007-03-18 14:09:48

by Theodore Ts'o

[permalink] [raw]
Subject: Re: e2fsprogs coverity patch <cid-03.diff>

On Fri, Feb 09, 2007 at 06:11:26PM -0800, Brian D. Behlendorf wrote:
> blkid_dev_has_tag() will immediately return -1 (an error if value is NULL. Thus
> at the test later on value cannot be NULL. There are two possible ways to go
> about fixing this. The first would be to remove the first NULL check for value.
> The second one would be to remove the second check (and the deadcode). I could
> be persuaded either way, but to preserve existing functionality I will opt to
> remove the second check.

Better to do this the other way, I think. It exposes more
functionality, and there shouldn't be any code that depends on
blkid_dev_has_tag returning an error is value is NULL.

- Ted