Closed
Bug 306785
Opened 19 years ago
Closed 19 years ago
memory leaks in PQG_ParamGenSeedLen
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11
People
(Reporter: julien.pierre, Assigned: julien.pierre)
Details
(Keywords: memory-leak)
Attachments
(1 file)
3.71 KB,
patch
|
nelson
:
review+
rrelyea
:
superreview+
|
Details | Diff | Splinter Review |
I was given a test case today which reproduces a memory leak . I reduced it to the following function : void test() { PQGParams* params = NULL; PQGVerify* vfy = NULL; SECStatus rv; rv = PK11_PQG_ParamGen(0, ¶ms, &vfy); if (rv != SECSuccess) { printf("PQG Param gen failed: %d\n", rv); } if (vfy) { PK11_PQG_DestroyVerify(vfy); } if (params) { PK11_PQG_DestroyParams(params); } } There is some memory left over that isn't freed by the PK11_PQG_DestroyVerify and PK11_PKG_DestroyParams functions . dbx checkleaks reports : Actual leaks report (actual leaks: 2 total size: 128 bytes) Total Num of Leaked Allocation call stack Size Blocks Block Address ========== ====== =========== ======================================= 64 1 0x809fec0 PR_Malloc < PORT_Alloc < SECITEM_AllocItem < PQG_ParamGenSeedLen < PQG_ParamGen < nsc_parameter_gen < NSC_GenerateKey < PK11_PQG_ParamGenSeedLen < PK11_PQG_ParamGen < test < main 64 1 0x809f148 calloc < PR_Calloc < PORT_ZAlloc < getPQseed < PQG_ParamGenSeedLen < PQG_ParamGen < nsc_parameter_gen < NSC_GenerateKey < PK11_PQG_ParamGenSeedLen < PK11_PQG_ParamGen < test < main The first leak is because of a temporary SECItem that isn't freed . The fix is straightforward. The second leak is because the seed bytes are allocated on the main heap, with PORT_ZAlloc, rather than in the arena of the object .
Assignee | ||
Comment 1•19 years ago
|
||
I have tested this and verified the 2 leaks are gone. all.sh still passes. I'm not 100% sure that my change in getPQseed is correct . The code was previously freeing and reallocating/memsetting the seed bytes on the heap each time - eventually leaving the buffer unfreed . I changed it so that the seedbytes only get allocated and zero'ed once. The seed buffer size never changes between multiple attempts, so there is certainly no need to reallocate. I also don't think we need to zero the bytes again, given that we are generating random data into the buffer in the code immediately below . In fact the initial zero'ing may be unnecessary too, unless FIPS186 says we must do that as a precaution.
Attachment #194622 -
Flags: superreview?(rrelyea)
Attachment #194622 -
Flags: review?(nelson)
Assignee | ||
Updated•19 years ago
|
Priority: -- → P2
Comment 2•19 years ago
|
||
Comment on attachment 194622 [details] [diff] [review] fix for memory leaks r=nelson
Attachment #194622 -
Flags: review?(nelson) → review+
Assignee | ||
Comment 3•19 years ago
|
||
Nelson, Thanks for the review . I checked this in to the tip for 3.11 . Checking in pqg.c; /cvsroot/mozilla/security/nss/lib/freebl/pqg.c,v <-- pqg.c new revision: 1.9; previous revision: 1.8 done
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Target Milestone: --- → 3.11
Comment 4•19 years ago
|
||
Comment on attachment 194622 [details] [diff] [review] fix for memory leaks Your code is correct wrt getPQseed(). We know seedBytes doesn't change, so it's clearly a waste to allocate each time through. bob
Attachment #194622 -
Flags: superreview?(rrelyea) → superreview+
You need to log in
before you can comment on or make changes to this bug.
Description
•