Closed Bug 341117 Opened 15 years ago Closed 15 years ago
_Destroy PBEParams doesn't destroy (free) its argument
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
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
Patch v1. Call |SECITEM_ZfreeItem(pItem, PR_TRUE)| instead off |PORT_ZFree(pItem, sizeof(SECItem))| in |PK11_DestroyPBEParams|.
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 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.