507376 - useless null check of global_salt in nsslowkey_GetPWCheckEntry

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P5)

Description

[timeless]

1340 nsslowkey_GetPWCheckEntry(NSSLOWKEYDBHandle *handle,NSSLOWKEYPasswordEntry *entry)
1347     SECItem   none = { siBuffer, NULL, 0 };
1356 global_salt = GetKeyDBGlobalSalt(handle);
1357 if (!global_salt) {
1358 global_salt = &none;
1360     if (global_salt->len > sizeof(entry->data)) {
1404     if (global_salt && global_salt != &none) {

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I wish we could stop coverity from complaining about "useless" checks.
When a pointer is checked for NULL after it has been dereferenced, 
there are two possibilities.  Either 
a) the pointer could be NULL at the point that it is dereferenced, which 
is a potentially serious bug, or 
b) the pointer CANNOT be NULL at that point, in which case the following 
NULL check, while superfluous, hurts nothing.  
I think Coverity is smart enough to tell the difference.  It should be.

In this case, someone (timeless?  Coverity) has determined that this is a
harmless case.

We must prioritize all the substantive issues higher than these trivial
matters.  Trying to "fix" these trivial matters is mere "apple polishing".
I wish we could just stop Coverity from wasting our time with this stuff.

Comment 2

[timeless]

yeah, i'm the one making the determination. i'll do that with the summary 'useless' and the severity 'enhancement'.

for all of these bugs which are simple, i'll also post the patches (but i do that in a distinct pass, i.e. now).

coverity just says "you checked for null after dereferencing", it's up to the person reading the code to figure out whether it's useless or a bug.

currently i'm running through a section which is mostly going to be this class.

Comment 3

[timeless]

Attachment: patch

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 391588 [details] [diff] [review]
patch

r+
no objection since SECITEM_FreeItem also checks.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

This patch affects FIPS validated code, so must wait.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

softoken/legacydb/keydb.c; new revision: 1.11.8.3; previous revision: 1.11.8.2