341117 - PK11_DestroyPBEParams doesn't destroy (free) its argument

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

PK11_DestroyPBEParams takes, as its sole argument, a SECItem * which, in turn,
points to a CK_PBE_PARAMS.  It destroys the CK_PBE_PARAMS, but not the SECItem.

PK11_DestroyPBEParams complements PK11_CreatePBEParams, which returns the 
address of a newly allocated SECItem, containing the created CK_PBE_PARAMS.

So, a function that calls PK11_CreatePBEParams to get the SECItem, and then 
calls  PK11_DestroyPBEParams to destroy it, leaks the SECItem.  

This seems wrong.  PK11_DestroyPBEParams should destroy the SECItem along 
with the CK_PBE_PARAMS.  If there is code that now depends on it NOT 
destroying the SECItem, then IMO we need another function that does.

This problem was detected in coverity CID 544

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

Would the correct course of action here be too call |SECITEM_ZfreeItem(pItem, PR_TRUE)| after |PORT_ZFree(pItem, sizeof(SECItem))| in |PK11_DestroyPBEParams|?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #1)
> Would the correct course of action here be too call |SECITEM_ZfreeItem(pItem,
> PR_TRUE)| after |PORT_ZFree(pItem, sizeof(SECItem))| in
> |PK11_DestroyPBEParams|?

Ryan, the suggested code would call         SECITEM_ZfreeItem(pItem, PR_TRUE)
*INSTEAD OF*                                PORT_ZFree(pItem, sizeof(SECItem))
in PK11_DestroyPBEParams

Comment 3

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Patch v1.

Call |SECITEM_ZfreeItem(pItem, PR_TRUE)| instead off |PORT_ZFree(pItem, sizeof(SECItem))| in |PK11_DestroyPBEParams|.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 242650 [details] [diff] [review]
Patch v1

r=nelson.  
I can't do both r and sr on this patch, so asking Bob relyea for sr

Comment 5

[Robert Relyea]

Comment on attachment 242650 [details] [diff] [review]
Patch v1

This patch is not correct, and will cause a double free. 

pk11_destroy_ck_pbe_params() will free the params, which is the same as pItem->data.
SECITEM_ZfreeItem(pItem,PR_TRUE);
will again free pItem->data.

I do not believe the original evaluation of the bug is correct. If we are leaking PBEParams, we must be leaking elsewhere (or it is possible the tool is mistaken).

I would approve a patch that rearranged the allocation (rename pk11_destroy_ck_pbe_params to pk11_destroy_ck_pbe_params_data, does not free params, and uses SECITEM_ZfreeItem as in this patch. Be sure to change the loser side of Create if you do this.

bob

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

It appears to me that this bug has already been fixed, but no notation was
ever made in this bug showing that.  

Alexei Volkov's revision  1.14 to pk11pbe.c seems to have fixed the problem
in PK11_DestroyPBEParams.  SO I think all that's left to do is mark this bug
fixed.  

Bob, Alexei, do you agree?

Comment 7

[Robert Relyea]

Absolutely. Before 1.14 there was a secitem leak. I say close it.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Reassigning to Alexei before resolving fixed.

Comment 9

[Alexei Volkov]

Didn't know that there is a separate bug for PK11_DestroyPBEParams. It is closed as it should be.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Thanks for fixing it, Alexei!