Closed
Bug 1028764
Opened 10 years ago
Closed 10 years ago
Remove some useless declarations
Categories
(NSS :: Libraries, defect, P2)
NSS
Libraries
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)
9.17 KB,
patch
|
KaiE
:
review+
|
Details | Diff | Splinter Review |
The attached patches remove some useless/dead declarations.
AFAIK, there is no side effect.
Attachment #8444198 -
Flags: review?(wtc)
Updated•10 years ago
|
Assignee: nobody → sledru
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 3.16.3
Updated•10 years ago
|
Target Milestone: 3.16.3 → 3.17
Assignee | ||
Updated•10 years ago
|
Attachment #8444198 -
Flags: review?(kaie)
Comment 1•10 years ago
|
||
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)
Comment 2•10 years ago
|
||
In particular I'm referring to the variable assignments.
(You don't need to talk about the removed variables.)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(sledru)
Restrict Comments: true
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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-
Assignee | ||
Updated•10 years ago
|
Restrict Comments: false
Assignee | ||
Comment 6•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8490850 -
Attachment description: attachment.cgi?id=8444198 → patch v2
Attachment #8490850 -
Attachment filename: attachment.cgi?id=8444198 → patchv2
Comment 7•10 years ago
|
||
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+
Comment 8•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: 3.17 → 3.17.3
Updated•10 years ago
|
Target Milestone: 3.17.3 → 3.18
Updated•10 years ago
|
Target Milestone: 3.18 → 3.17.3
Assignee | ||
Updated•10 years ago
|
Keywords: clang-analyzer
You need to log in
before you can comment on or make changes to this bug.
Description
•