Closed Bug 341117 Opened 15 years ago Closed 15 years ago

PK11_DestroyPBEParams doesn't destroy (free) its argument

Categories

(NSS :: Libraries, defect, P2)

3.11.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.3

People

(Reporter: nelson, Assigned: alvolkov.bgs)

References

Details

(Keywords: coverity, Whiteboard: CID 544)

Attachments

(1 file)

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
Priority: -- → P2
Target Milestone: --- → 3.11.3
Blocks: 341118
Whiteboard: CID 544
Would the correct course of action here be too call |SECITEM_ZfreeItem(pItem, PR_TRUE)| after |PORT_ZFree(pItem, sizeof(SECItem))| in |PK11_DestroyPBEParams|?
(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
Attached patch Patch v1Splinter Review
Patch v1.

Call |SECITEM_ZfreeItem(pItem, PR_TRUE)| instead off |PORT_ZFree(pItem, sizeof(SECItem))| in |PK11_DestroyPBEParams|.
Assignee: rrelyea → sciguyryan+bugzilla
Status: NEW → ASSIGNED
Attachment #242650 - Flags: superreview?(nelson)
Attachment #242650 - Flags: review?(nelson)
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
Attachment #242650 - Flags: superreview?(rrelyea)
Attachment #242650 - Flags: superreview?(nelson)
Attachment #242650 - Flags: review?(nelson)
Attachment #242650 - Flags: review+
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
Attachment #242650 - Flags: superreview?(rrelyea) → superreview-
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?
Absolutely. Before 1.14 there was a secitem leak. I say close it.
Reassigning to Alexei before resolving fixed.
Assignee: sciguyryan+bugzilla → alexei.volkov.bugs
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Didn't know that there is a separate bug for PK11_DestroyPBEParams. It is closed as it should be.
Thanks for fixing it, Alexei!
You need to log in before you can comment on or make changes to this bug.