pkix_pl_CRL_Hashcode makes coverity sad

RESOLVED FIXED in 3.12.7

Status

P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

({coverity})

3.12.3
3.12.7
coverity

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

2.40 KB, patch
timeless
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
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 }
Assignee: nobody → alexei.volkov.bugs
OS: Linux → All
Priority: -- → P2
Hardware: x86 → All
Target Milestone: --- → 3.12.5
Version: unspecified → 3.12.3
(Assignee)

Comment 1

9 years ago
Created attachment 430931 [details] [diff] [review]
proposal
Assignee: alexei.volkov.bugs → timeless
Status: NEW → ASSIGNED
Attachment #430931 - Flags: review?(nelson)
Comment on attachment 430931 [details] [diff] [review]
proposal

referring review requests for libPKIX to Alexei.
Attachment #430931 - Flags: review?(nelson) → review?(alexei.volkov.bugs)

Comment 3

9 years ago
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
Attachment #430931 - Flags: review?(alexei.volkov.bugs) → review+
(Assignee)

Comment 4

9 years ago
Created attachment 431316 [details] [diff] [review]
updated description
Attachment #430931 - Attachment is obsolete: true
Attachment #431316 - Flags: review+

Updated

9 years ago
Keywords: checkin-needed
Target Milestone: 3.12.5 → 3.12.7
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 <timeless@mozdev.org>, r=alexei
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.