337034 - Coverity 509, dead code in mozilla/security/nss/lib/softoken/dbmshim.c

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect, P3)

Description

[Jon Smirl]

All paths in dbsopen() return the db if it is opened, there is no way to get to loser: with an open db.

@@ -651,9 +651,6 @@

     return dbs;
 loser:
-    if (db) {
-       (*db->close)(db);
-    }
     if (dbsp && dbsp->blobdir) {
        PORT_Free(dbsp->blobdir);
     }

Comment 1

[Jon Smirl]

Attachment: remove dead code

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 221218 [details] [diff] [review]
remove dead code

IMO, this change is not worth the bother.  Leaving this test in place means that if a new "goto loser" case is ever added, in which db can be non-NULL, loser will already be coded to handle it correctly.  That seems quite desirable, moreso than silencing some automated critic.

Comment 3

[Jon Smirl]

32% (187/588) of all Coverity hits in Mozilla are in mozilla/security (169 in nss). Many noise hits can make Coverity miss real problems. Without eliminating all of this noise it is too hard to locate real problems. Especially since the hits change numbers and have to be reinspected on each run.

Comment 4

[Wan-Teh Chang]

Attachment: Nest two related tests

While reviewing Jon Smirl's patch (attachment 221218 [details] [diff] [review])
I found that these two tests are related and can be
nested.

Software evolves.  Sometimes it makes sense to have
extra code to ensure that the code stays correct with
future changes.  So I am fine with not checking in
Jon Smirl's patch.

Most compilers allow me to turn off a specific warning.
There must be a way to suppress a specific error that
Coverity reports.

Comment 5

[Wan-Teh Chang]

I'll use an example to get this point across.

Some programmers initialize all variables as
a rule.  Most will agree that this practice
defends against common programming errors.  If
Coverity reports unnecessary variable initializations,
should we remove the unnecessary variable
initializations, or should we find out how
to suppress this check in Coverity?

Comment 6

[timeless]

well, most compilers do complain about unused variables... but coverity's design is to give useful complaints. i'd expect a compiler to complain about an unused variable initialized or not, so the coverity design would have no reason to be any more critical of your variable for you having initialized it, and it certainly wouldn't fall into a bucket like this one.

that said, i do sympathize with the concern of having code that scales to meet a future need, if you don't want to actually remove the lines, then #ifndef DEADCODE them until they're needed :).

Comment 7

[Robert Relyea]

I agree with nelson.
removing the code will make future changes more brittle. There's always a tradeoff between what you need to do to silencing diagnostic tool warnings and other factors in the code (safety, speed). In this case I believe safety wins.

Comment 8

[Wan-Teh Chang]

Marked the bug WONTFIX.  We can't fix noncritical bugs in
the softoken on the NSS_3_11_BRANCH now, so this bug cannot
have a 3.11.x target milestone.

I did not check in the "remove dead code" patch.  Note
that after expressing our opinions about such patches
in this bug, we have since reviewed and checked in many
similar patches.  So we may want to reconsider this patch.
In any case, this bug should have the target milestone
3.12 and this patch, if approved, should only be checked
into the NSS trunk.

I checked in my "nested two related tests" patch on the
NSS trunk (3.12).

Checking in dbmshim.c;
/cvsroot/mozilla/security/nss/lib/softoken/dbmshim.c,v  <--  dbmshim.c
new revision: 1.13; previous revision: 1.12
done

Comment 9

[Steffen Wilberg]

Comment on attachment 221218 [details] [diff] [review]
remove dead code

wtc, you marked this bug as WONTFIX, but you didn't make a decision on the review requested from you.
And you said at the same time that "we may want to reconsider this patch".

So what's your final decision here? Should this bug be reopened? Or do you deny the review?

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Steffen, I think that 3 of the NSS module owners have expressed unanimity
about this WONTFIX.

Comment 11

[Steffen Wilberg]

Comment on attachment 221218 [details] [diff] [review]
remove dead code

I see. So the review request on this patch can be cleared, right?
(Part of an effort to reduce obsolete review requests)