337395 - UpdateV7DB should assert that DecodeDBCertEntry(&certEntry,&dbEntry) is changes the value of certEntry.derCert or that (* updatedb->seq)(updatedb, &key, &data, R_NEXT) changes the value of data

Status: RESOLVED INVALID

(NSS :: Libraries, enhancement)

Description

[timeless]

so, this is a coverity false positive if all modules behave correctly, but coverity can't see enough of the world to know this.

Anyway, the coverity report is fairly useless, i had to have someone explain what coverity was thinking to me.

The basic idea is that the function starts, reaches the switch is told to deal with certDBEntryTypeCert, decides that DecodeDBCertEntry isn't changing certEntry.derCert.data's address, watches
pkcs11_freeStaticData(certEntry.derCert.data, certEntry.derCertSpace); delete memory, and then reaches the while condition. at the while condition it probably doesn't think data is being changed or doesn't worry about it, but it decides that the function will allow it toloop again. it goes back to the switch, takes the same path and concludes that with the certEntry.derCert.data address not changing, you've double freed.

that ->seq ... R_NEXT wouldn't change data or that DecodeDBCertEntry for a different dbEntry wouldn't change certEntry are both bad conclusions by coverity, but without some assertion in the code, it can't reasonably reach the conclusion of the human programmer reading this code.

i'm perfectly happy w/ this bug being wontfixed since one can claim any of:
1. code can misbehave in an infinite number of ways
2. code can give bogus pointers
3. code could give back pointers in (void*)x (void*)(2*(int)x) (void*)x instead of (void*)x (void*)x if it decided to be broken [where x and 2*x are both valid creatures...).
4. code to check for broken sequencing is too expensive and messy for the non debug version.
5. something else

anyway, i think it wouldn't be too hard to add local variables to remember the previous values of the two items (certEntry.whatever, data.whatever) and compare those against new values, asserting that they aren't the same.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Timeless, Can you show us what the original "raw" coverity complaint was 
about this code?  Perhaps a URL would be helpful.  

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Coverity CID 674.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

This CID was marked "FALSE" in coverity, and it has subsequently 
disapeared from the runs.  I think this means that coverity has 
agreed that it is a false positive. 
So, accordingly, I am marking this bug invalid.