memory leaks in PQG_ParamGenSeedLen

RESOLVED FIXED in 3.11

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Julien Pierre, Assigned: Julien Pierre)

Tracking

({memory-leak})

3.10.1
3.11
memory-leak

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

3.71 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Robert Relyea
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
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, &params, &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

13 years ago
Created attachment 194622 [details] [diff] [review]
fix for memory leaks

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

13 years ago
Priority: -- → P2
Comment on attachment 194622 [details] [diff] [review]
fix for memory leaks

r=nelson
Attachment #194622 - Flags: review?(nelson) → review+
(Assignee)

Comment 3

13 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
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Target Milestone: --- → 3.11

Comment 4

13 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+
Keywords: mlk
You need to log in before you can comment on or make changes to this bug.