1216514 - [CID 222177] leaked_storage: Variable rvCRL going out of scope leaks the storage it points to

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

rvCRL is leaked when returning early

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: CID222177.patch

Comment 2

[Franziskus Kiefer [:franziskus]]

Attachment: CID222177.patch

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8676227 [details] [diff] [review]
CID222177.patch

Review of attachment 8676227 [details] [diff] [review]:
-----------------------------------------------------------------

I couldn't satisfy myself that this was correct.  Asking a second opinion.

::: lib/pki/certificate.c
@@ +1121,5 @@
>                                            NULL, /* class */
>                                            &rvCRL->url,
>                                            &rvCRL->isKRL);
>      if (status != PR_SUCCESS) {
> +	nssPKIObject_Destroy((nssPKIObject *)rvCRL);

I think that there is a better option here.  Before allocating, optionally call mark = nssArena_Mark().  At the end, if there is a mark, call Unmark on success and Release on failure.  If there is no mark, then call nss_ZFreeIf().  The problem here is that you will decrement the reference count on the parent object when you free here, but I can't see any evidence that the reference count is incremented anywhere.  That could produce a worse situation.

BTW, there is no leak if you accept the precondition that the arena is always non-NULL, but I couldn't figure out how to guarantee that (other than adding an explicit test for non-NULL, that is).

Comment 4

[Franziskus Kiefer [:franziskus]]

(In reply to Martin Thomson [:mt:] from comment #3)
> Comment on attachment 8676227 [details] [diff] [review]
> CID222177.patch
> I think that there is a better option here.  Before allocating, optionally
> call mark = nssArena_Mark().  At the end, if there is a mark, call Unmark on
> success and Release on failure.  If there is no mark, then call
> nss_ZFreeIf().  The problem here is that you will decrement the reference
> count on the parent object when you free here, but I can't see any evidence
> that the reference count is incremented anywhere.  That could produce a
> worse situation.
> 
> BTW, there is no leak if you accept the precondition that the arena is
> always non-NULL, but I couldn't figure out how to guarantee that (other than
> adding an explicit test for non-NULL, that is).

yeah I see... the best (easiest) solutions seems to explicitly null-check the arena and only destroy the object if the arena is actually null.

Comment 5

[Eric Rescorla (:ekr)]

I agree with MT that it's not clear that the free here is safe.

In general, nssPKIObject_Destroy seems pretty scary. Franziskus's proposed solution seems reasonable.

Comment 6

[Franziskus Kiefer [:franziskus]]

Attachment: CID222177.patch

destroying this now only if we don't have an arena

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8684845 [details] [diff] [review]
CID222177.patch

Review of attachment 8684845 [details] [diff] [review]:
-----------------------------------------------------------------

I agree that this is probably the best we can hope to do here.

https://hg.mozilla.org/projects/nss/rev/040828d246d8