306785 - memory leaks in PQG_ParamGenSeedLen

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

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 .

Comment 1

[Julien Pierre]

Attachment: 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.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 194622 [details] [diff] [review]
fix for memory leaks

r=nelson

Comment 3

[Julien Pierre]

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

Comment 4

[Robert Relyea]

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