Closed Bug 469601 Opened 12 years ago Closed 11 years ago

Coverity errors reported for pk11wrap

Categories

(NSS :: Libraries, defect, P2)

3.12
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.7

People

(Reporter: nelson, Assigned: shailen.n.jain)

Details

(Keywords: coverity)

Attachments

(1 file, 5 obsolete files)

The latest full Coverity run for firefox (run 278) reports a few errors 
for pk11wrap.  I will add them as comments to this bug.  Here's the first.

CID 1380 - NULL check after dereference.  

In file nss/lib/pk11wrap/pk11cert.c, function PK11_GetAllSlotsForCert,
the code dereferences the argument "cert" in an initializer of a local 
variable, THEN checks cert for NULL.
CID 1379 - NULL check after dereference

In PK11_PubWrapSymKey, the function passes argument symKey to pk11_ForceSlot,
which dereferences it.  Then later, PK11_PubWrapSymKey checks symKey for NULL.
Priority: -- → P2
Target Milestone: 3.12.3 → 3.12.5
Attached patch Patch V 1 (obsolete) — Splinter Review
Nelson :

  Please review the patch and let me know if I had missed anything.

Regards,
Shailendra
Attachment #430915 - Flags: review?(nelson)
Comment on attachment 430915 [details] [diff] [review]
Patch V 1

The c++ and c99 languages allow variables to be declared anywhere
in the middle of executable code, but older versions of the c 
language only allow variables to be declared at the very beginning
of basic blocks (right after opening braces).  NSS is used on 
platforms whose compilers enforce the old c language rules, and do 
not allow variables to be declared after the first executable statement.
Therefore, it is necessary to keep variable declarations at the 
beginning of the functions (or at the beginning of basic blocks).


>+    NSSCertificate *c = STAN_GetNSSCertificate(cert);
>+    /* add multiple instances to the cert list */

what happens if c is NULL here?

>+    nssCryptokiObject **instances = nssPKIObject_GetInstances(&c->object);
Attachment #430915 - Flags: review?(nelson) → review-
Attached patch Patch Version 2 (obsolete) — Splinter Review
Nelson :

  I made the changes you suggested.

  One question : 

   If pointer c variable happens to be NULL, what should be error code ? In the patch, I have set the error code as 'SEC_ERROR_NO_TOKEN'.

Thanks and Regards,
Shailendra
Assignee: rrelyea → shailen.n.jain
Attachment #430915 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #431055 - Flags: review?(nelson)
Attached patch Patch Version 3 (obsolete) — Splinter Review
Nelson :

   There was a typo in the previous patch due to which I got the below error during compilation.

pk11cert.c:2493: error: redeclaration of âinstancesâ with no linkage
pk11cert.c:2478: note: previous declaration of âinstancesâ was here


Rectified the same with the newer version of the patch.

Thanks and Regards,
Shailendra
Attachment #431055 - Attachment is obsolete: true
Attachment #431058 - Flags: review?(nelson)
Attachment #431055 - Flags: review?(nelson)
But I still have the question.

  If pointer c variable happens to be NULL, what should be error code ? In the
patch, I have set the error code as 'SEC_ERROR_NO_TOKEN'.

Regards,
Shailendra
(In reply to comment #6)
> If pointer c variable happens to be NULL, what should be error code ? 

Rather than setting it to some fixed value, you should call 
CERT_MapStanError() to set it to the appropriate value.

Stan functions have their own error error scheme with their own error codes.
When they return an error indication (e.g. a null pointer) you must translate
their stan error into an NSS error code.  CERT_MapStanError() does that.
Attachment #431058 - Flags: review?(nelson) → review-
Comment on attachment 431058 [details] [diff] [review]
Patch Version 3

For function PK11_PubWrapSymKey, the correct order of tests and 
error codes should be:

1) if symKey is NULL, set SEC_ERROR_INVALID_ARGS and return FAILURE.
2) call ForceSlot.  If it returns non-NULL, replace symkey with the 
return value.
3) check symkey->slot.  If null, set SEC_ERROR_NO_MODULE and 
return failure.
Attached patch Patch Version 4 (obsolete) — Splinter Review
Nelson :

   I am attaching the new patch to include your suggestions.

Thanks,
Shailendra
Attachment #431058 - Attachment is obsolete: true
Attachment #431082 - Flags: review?(nelson)
Attached patch Patch Version 5 (obsolete) — Splinter Review
Nelson :

  Please bare with me as there was some tab/space issue. So attaching the new patch.

Thanks and Regards,
Shailendra
Attachment #431082 - Attachment is obsolete: true
Attachment #431085 - Flags: review?(nelson)
Attachment #431082 - Flags: review?(nelson)
Comment on attachment 431085 [details] [diff] [review]
Patch Version 5

This patch would be OK with me, but I'll bet Coverity will complain
about that symKey cannot be NULL at the test just before the 
NO_MODULE error.  Since the point of this bug is to silence Coverity
we might as well really silence it. 

>+    if (symKey == NULL) {
>+	PORT_SetError( SEC_ERROR_INVALID_ARGS );
>+	return SECFailure;
>+    }
>+
>     /* if this slot doesn't support the mechanism, go to a slot that does */
>     newKey = pk11_ForceSlot(symKey,type,CKA_ENCRYPT);
>     if (newKey != NULL) {
> 	symKey = newKey;
>     }
> 
>     if ((symKey == NULL) || (symKey->slot == NULL)) {
> 	PORT_SetError( SEC_ERROR_NO_MODULE );
Attachment #431085 - Flags: review?(nelson) → review-
Attached patch Patch Version 6Splinter Review
Nelson :

  There are so many little things I came across working on this defect. Thanks for sharing your knowledge on this :)

  I am attaching the newer version of the patch and Please review.
Attachment #431085 - Attachment is obsolete: true
Attachment #431305 - Flags: review?(nelson)
Comment on attachment 431305 [details] [diff] [review]
Patch Version 6

r=nelson
Attachment #431305 - Flags: review?(nelson) → review+
Checking in pk11cert.c; new revision: 1.174; previous revision: 1.173
Checking in pk11skey.c; new revision: 1.118; previous revision: 1.117

Thanks, Shailendra
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: 3.12.5 → 3.12.7
You need to log in before you can comment on or make changes to this bug.