504136 - pkix_pl_CRL_Hashcode makes coverity sad

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

559 pkix_pl_CRL_Hashcode(
560         PKIX_PL_Object *object,
561         PKIX_UInt32 *pHashcode,
562         void *plContext)
563 {
564         PKIX_PL_CRL *crl = NULL;
565         PKIX_UInt32 certHash;
566         SECItem *crlDer = NULL;
567         
568         PKIX_ENTER(CRL, "pkix_pl_CRL_Hashcode");
569         PKIX_NULLCHECK_TWO(object, pHashcode);
570 
571         PKIX_CHECK(pkix_CheckType(object, PKIX_CRL_TYPE, plContext),
572                     PKIX_OBJECTNOTCRL);
573 
574         crl = (PKIX_PL_CRL *)object;
575         if (crl->adoptedDerCrl) {
576             crlDer = crl->adoptedDerCrl;
577         } else if (crl->nssSignedCrl && crl->nssSignedCrl->derCrl) { 

you're carefully checking nssSignedCrl and derCrl, which means you think it's possible for one or both of them to be null

578             crlDer = crl->nssSignedCrl->derCrl;
579         }

which means there's room for an else clause. Let's assume we reach that else clause then crlDer is null...

and we crash here:
581         if (crlDer->data)
582             PKIX_CHECK(pkix_hash(crlDer->data, crlDer->len,
583                                  &certHash, plContext),
584                        PKIX_ERRORINHASH);

let's assume that we didn't crash there, but that crlDer->data was null, then Coverity complains that certHash is returned unitialized here:
586         *pHashcode = certHash;
587 
588 cleanup:
589 
590         PKIX_RETURN(CRL);
591 }

Comment 1

[timeless]

Attachment: proposal

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 430931 [details] [diff] [review]
proposal

referring review requests for libPKIX to Alexei.

Comment 3

[Alexei Volkov]

Comment on attachment 430931 [details] [diff] [review]
proposal

Patch is fine. Crl, as it used today can not be initialized without der. Der should be available through either crl->nssSignedCrl or clr->adoptedDerCrl.
Returning CANNOTAQUIRECRLDER as an error code is also fine.

Please also change the error description so it does not specify a particular function.
r=alexei

Comment 4

[timeless]

Attachment: updated description

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Alexei, Timeless,
The patch as submitted and reviewed doesn't even compile.

I fixed it and committed it on trunk.

Bug 504136 pkix_pl_CRL_Hashcode makes coverity sad
Patch contributed by Timeless <[e-mail]>, r=alexei