336477 - Coverity CERT_UncacheCRL variable named "returned" is not freed if !removed

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

this might not be a particularly reachable case, but the codepath seems to mostly exist to allow it.

Comment 1

[timeless]

Attachment: hopefully i'm patching a leak instead of making a double free

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 221176 [details] [diff] [review]
hopefully i'm patching a leak instead of making a double free

The variable named "returned" has gone out of scope before the point where this patch attempts to free it.

Comment 3

[Julien Pierre]

Thanks for the bug report, timeless. This is a leak. The function to call to destroy a CachedCrl is CachedCrl_Destroy . It should be done unconditionally in CERT_UncacheCRL if CachedCrl_Create succeeded .

Comment 4

[Julien Pierre]

Attachment: proper fix

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 221863 [details] [diff] [review]
proper fix

r=nelson

Comment 6

[Julien Pierre]

Thanks for the review, Nelson.

Checked in to NSS_3_11_BRANCH :

Checking in crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.49.24.1; previous revision: 1.49
done

And tip :

Checking in crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.50; previous revision: 1.49
done

Comment 7

[Wan-Teh Chang]

Julien, a problem with your patch "proper fix" is
that the old value of 'rv' set by a failed
CachedCrl_Compare call inside the for loop will be
lost if the CachedCrl_Destroy call succeeds.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Yeah, I noticed that in review, but figured Julien must have believed 
that's OK.  Reopening to ask Julien about this.

Comment 9

[Julien Pierre]

Attachment: better fix

I wish you had given me this feedback during the review.
The only return value that could be lost was from CachedCrl_Compare, which is not very likely to happen, so the problem was minor.
But I found that the code wasn't checking the return from DPCache_RemoveCRL . This is more serious.

This incremental patch fixes both issues.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222132 [details] [diff] [review]
better fix

This makes sense to me.
r=nelson

Comment 11

[Wan-Teh Chang]

Comment on attachment 222132 [details] [diff] [review]
better fix

r=wtc.

This function has the following near the end:

    if (PR_TRUE != removed)
    {
        rv = SECFailure;
    }
    ...

    if (PR_TRUE != removed)
    {
        PORT_SetError(SEC_ERROR_CRL_NOT_FOUND);
    }

I'm wondering if these two should be combined.  Also,
the PORT_SetError call may overwrite the existing error
code.  So I'm wondering if these should be written as:

    if (SECSuccess == rv && PR_TRUE != removed)
    {
        rv = SECFailure;
        PORT_SetError(SEC_ERROR_CRL_NOT_FOUND);
    }

Comment 12

[Julien Pierre]

Attachment: hopefully the last patch

Wan-Teh,

Your last suggestion makes sense, so I added it to this patch.
Also, we weren't checking the status of SEC_DestroyCrl . This patch does.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 223257 [details] [diff] [review]
hopefully the last patch

r=nelson, with one proviso.  This too-long line needs to be wrapped.
I suggest putting the comment on a separate line inside the basic block.


>+    if (SECSuccess != SEC_DestroyCrl(oldcrl) ) { /* need to do this because object is refcounted */
>+        rv = SECFailure;

Comment 14

[Julien Pierre]

Checked in to the tip :

Checking in crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.52; previous revision: 1.51
done

And to NSS_3_11_BRANCH :

Checking in crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.49.24.3; previous revision: 1.49.24.2
done

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

CId 415