Closed Bug 1028764 Opened 6 years ago Closed 6 years ago

Remove some useless declarations

Categories

(NSS :: Libraries, defect, P2)

Tracking

(Not tracked)

RESOLVED FIXED
3.17.3

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

(Blocks 1 open bug)

Details

(Keywords: clang-analyzer)

Attachments

(1 file, 1 obsolete file)

Attached patch remove-useless-declaration.diff (obsolete) — Splinter Review
The attached patches remove some useless/dead declarations.
AFAIK, there is no side effect.
Attachment #8444198 - Flags: review?(wtc)
Assignee: nobody → sledru
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 3.16.3
Target Milestone: 3.16.3 → 3.17
Attachment #8444198 - Flags: review?(kaie)
Can you please explain WHY exactly you think they are useless declarations? You are removing functional code. It's not immediately clear why this code is useless.

You probably already went through the code to understand why it's unnecessary.

Instead of requiring us to try to repeat your work, and draw the same conclusions as you - could you please save us time, by explaining why these are unnecessary, so that all we have to do is checking that your arguments are reasonable?
Flags: needinfo?(sledru)
In particular I'm referring to the variable assignments.

(You don't need to talk about the removed variables.)
Flags: needinfo?(sledru)
Restrict Comments: true
Comment on attachment 8444198 [details] [diff] [review]
remove-useless-declaration.diff

>-    if (!doPriv && !doPub) doPriv = PR_TRUE;
>-

ok, because doPriv isn't used after this line.


>-        doKeyGen = PR_TRUE; /* Always do a keygen for session keys.
>-                               Import of hardcoded key is not supported */

ok, only used in the else path, variable not used in remaining code.


>-    maxout -= fullblocks;

ok


>     if (holdingLock) {
> 	PZ_Unlock(blindingParamsList.lock);
>-	holdingLock = PR_FALSE;
>     }

You're right that this isn't really necessary, because it's so close to the end of the function. But for consistency with the remainder of the function, it might be worth it to keep it. Inside this function, the variable is always consistent with the lock, by removing this assignment, you're breaking the rule.

It's a matter of taste. My taste is consistency.


>-    unsigned long controlMask = mod->evControlMask;
>+    unsigned long controlMask;
...
>     mod->evControlMask |= SECMOD_END_WAIT;
>     controlMask = mod->evControlMask;

ok...




> NSSLOWCERTCertificate *
> nsslowcert_FindCertByIssuerAndSN(NSSLOWCERTCertDBHandle *handle, NSSLOWCERTIssuerAndSN *issuerAndSN)
> {
>     SECItem certKey;
>     SECItem *sn = &issuerAndSN->serialNumber;
>     SECItem *issuer = &issuerAndSN->derIssuer;
>     NSSLOWCERTCertificate *cert;
>-    int data_left = sn->len-1;
>+    int data_left;

If this variable is unnecessary outside the following if block, then please remove this line altogether, and declare the variable locally inside the if block.




> nsslowcert_FindTrustByIssuerAndSN(NSSLOWCERTCertDBHandle *handle, 
> 					NSSLOWCERTIssuerAndSN *issuerAndSN)
> {
>     SECItem certKey;
>     SECItem *sn = &issuerAndSN->serialNumber;
>     SECItem *issuer = &issuerAndSN->derIssuer;
>     NSSLOWCERTTrust *trust;
>     unsigned char keyBuf[512];
>-    int data_left = sn->len-1;
>+    int data_left;

same here, please move declaration inside the if block


>diff --git a/lib/softoken/pkcs11c.c b/lib/softoken/pkcs11c.c
>--- a/lib/softoken/pkcs11c.c
>+++ b/lib/softoken/pkcs11c.c
>@@ -6318,17 +6318,16 @@ CK_RV NSC_DeriveKey( CK_SESSION_HANDLE h
> 		** server_write_key[CipherSpec.key_material]
> 		** final_server_write_key = MD5(server_write_key +
> 		**                    ServerHello.random + ClientHello.random);
> 		*/
> 		MD5_Begin(md5);
> 		MD5_Update(md5, &key_block[i], effKeySize);
>             	MD5_Update(md5, srcrdata, sizeof srcrdata);
> 		MD5_End(md5, key_block2, &outLen, MD5_LENGTH);
>-		i += effKeySize;

I'd prefer to keep it for consistency with the surrounding code. It's only unnecessary because there's no other operation involving i following. But removing it could easily introduce an error when future developers do copy/paste and miss this line when adding a following operation.

Matter of taste.


> 	secret.len  = effKeySize;
>-	i          += effKeySize;
> 	keyblk.data = key_block2;
> 	keyblk.len  = keySize;

same consistency argument. I'd keep it.


> 		    PORT_SetError(SEC_ERROR_KEYGEN_FAIL);
>-		    rv = SECFailure;
> 		    break;

You're right this assignment doesn't have an effect, because the break leaves the "for" loop, and line 867 sets it unconditionally to success. However, could you have found a real bug? Do you think the original programmer intended to return an error? If yes, this should be fixed. If no, a comment should be added, why setting an error code with returning success is intended...


> 	    return SECFailure;
> 	}
>-	maxBytes        -= extLen;

ok


>     const PRInt32      limit     = sslCopyLimit;
>     PRBool             blocking;
>-    PRIOVec            myIov     = { 0, 0 };
>+    PRIOVec            myIov;

This function is slightly tricky to read, because of its use of macros. I'd prefer to keep this simple initialization to an apparent safe default, it shouldn't hurt.
Comment on attachment 8444198 [details] [diff] [review]
remove-useless-declaration.diff

r- primarily because of the need to clarify the removal of the rv=SECFailure and potential bug here.

Your other changes seem functional correct, but I'd prefer to keep some of them unchanged.
Attachment #8444198 - Flags: review?(kaie) → review-
Restrict Comments: false
Attached patch patch v2Splinter Review
I agree with all your comments.
I will report a new bug for the rv=SECFailure issue.
Attachment #8444198 - Attachment is obsolete: true
Attachment #8444198 - Flags: review?(wtc)
Attachment #8490850 - Flags: review?(kaie)
See Also: → 1068753
Attachment #8490850 - Attachment description: attachment.cgi?id=8444198 → patch v2
Attachment #8490850 - Attachment filename: attachment.cgi?id=8444198 → patchv2
Comment on attachment 8490850 [details] [diff] [review]
patch v2

r=kaie

I'll remove the unnecessary whitespace change in derive.c

Your changes to file rsa.c are no longer necessary, it seems this has already been cleaned up in the meantime.
Attachment #8490850 - Flags: review?(kaie) → review+
https://hg.mozilla.org/projects/nss/rev/9f358fbaae22
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: 3.17 → 3.17.3
Target Milestone: 3.17.3 → 3.18
Target Milestone: 3.18 → 3.17.3
Keywords: clang-analyzer
You need to log in before you can comment on or make changes to this bug.