604627 - coverity is willing to enforce PORT_Assert(crv != CKR_OK) in sftk_signTemplate

Status: VERIFIED WONTFIX

(NSS :: Libraries, enhancement)

Description

[timeless]

In the NSS which was present in gecko on 20100724,

Coverity proved to itself that by the time loser: is reached, inPeerDBTransaction will only be true if crv != 0.

What follows is its proof from that time.
506  	sftk_signTemplate(PLArenaPool *arena, SFTKDBHandle *handle, 
507  			  PRBool mayBeUpdateDB,
508  			  CK_OBJECT_HANDLE objectID, const CK_ATTRIBUTE *template,
509  			  CK_ULONG count)
510  	{

512  	    CK_RV crv;

516  	    PRBool inPeerDBTransaction = PR_FALSE;

526  	    if (keyHandle == NULL) {
527  		crv = CKR_OK;
528  		goto loser;

539  	    if ((keyTarget->sdb_flags & SDB_HAS_META) == 0) {
540  		crv = CKR_OK;
541  		goto loser;

545  	    if (usingPeerDB) {
546  		crv = (*keyTarget->sdb_Begin)(keyTarget);
547  		if (crv != CKR_OK) {
548  		    goto loser;
549  		}
550  		inPeerDBTransaction = PR_TRUE;
551  	    }
552  	
553  	    for (i=0; i < count; i ++) {
554  		if (sftkdb_isAuthenticatedAttribute(template[i].type)) {

562  		    if (keyHandle->passwordKey.data == NULL) {

Event const: After this line, the value of "crv" is equal to 257
Event assignment: Assigning "257UL" to "crv"
564  			crv = CKR_USER_NOT_LOGGED_IN;
565  			goto loser;
566  		    }

571  		    if (rv != SECSuccess) {
Event const: After this line, the value of "crv" is equal to 5
Event assignment: Assigning "5UL" to "crv"
572  			crv = CKR_GENERAL_ERROR; /* better error code here? */
573  			goto loser;
574  		    }

577  		    if (rv != SECSuccess) {
Event const: After this line, the value of "crv" is equal to 5
Event assignment: Assigning "5UL" to "crv"
578  			crv = CKR_GENERAL_ERROR; /* better error code here? */
579  			goto loser;
580  		    }
581  		}
582  	    }
583  	    crv = CKR_OK;

586  	    if (inPeerDBTransaction) {
587  		crv = (*keyTarget->sdb_Commit)(keyTarget);
Event cannot_single: After this line (or expression), the value of "crv" cannot be 0
588  		if (crv != CKR_OK) {
589  		    goto loser;
590  		}
591  		inPeerDBTransaction = PR_FALSE;
592  	    }
593  	
594  	loser:
595  	    if (inPeerDBTransaction) {

598  		PORT_Assert(crv != CKR_OK);
Event dead_error_condition: On this path, the condition "crv == 0UL" could not be true
Event dead_error_line: Cannot reach dead statement "crv = 5UL;"
599  		if (crv == CKR_OK) crv = CKR_GENERAL_ERROR;
600  	    }
601  	    return crv;
602  	}

I can understand someone not wanting to take away this code. But I think it's reasonable to rely on PORT_Assert and reviewers to ensure the constraint isn't violated instead of including dead code.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

It's a matter of NSS policy that every assert is to be followed by code 
handles the assertion failure case in non-debug builds.  
It's good to know that, at the present time, the assertion cannot fail,
but perhaps at some point in the future, that will change.  This is a 
safety net.  We like our safety nets.  Coverity will just have to live 
with them.

Comment 2

[timeless]

fine w/ me.