135069 - SEC_PKCS12DestroyExportContext() leaks slot reference

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

SEC_PKCS12CreateExportContext() calls PK11_GetInternalSlot()
if the third argument 'slot' is NULL. But
SEC_PKCS12DestroyExportContext() does not call PK11_FreeSlot(),
so this slot reference is leaked.

Comment 1

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 2

[Wan-Teh Chang]

Attachment: Proposed patch

Grab a reference in SEC_PKCS12CreateExportContext and
free the reference in SEC_PKCS12DestroyExportContext.

Please review this patch.  Thanks.

Comment 3

[Robert Relyea]

Comment on attachment 82944 [details] [diff] [review]
Proposed patch

I think if we add this code then we need to free the reference in
SEC_PKCS12ExportContextDestroy()

The other way is to free the PK11_GetInternalSlot() in the Create, but that
could 1) get ugly, and 2) more importantly leave a possible (albeit unlikely)
scenario where the internal slot disappears before we are done with it .

Comment 4

[Wan-Teh Chang]

Comment on attachment 82944 [details] [diff] [review]
Proposed patch

Bob wrote:
> I think if we add this code then we need to free the reference in
> SEC_PKCS12ExportContextDestroy()

That's exactly what my patch does (at @@ -2146,6 +2146,8 @@).

Comment 5

[Ian McGreer]

Comment on attachment 82944 [details] [diff] [review]
Proposed patch


looks like the slot is correctly freed to me.

Comment 6

[Robert Relyea]

Sorry, I thought it was the error case at 201, but it's clear we don't hit this
error case for the case where we set the slot. Ian's correct, the patch is right.

Comment 7

[Wan-Teh Chang]

Thanks for the code review.  I checked in the patch into the
tip of NSS.  Marked the bug fixed.

Comment 8

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) for checkin to the 1.0 branch. Pls check this in
asap. thanks!