351872 - Memory leak in FIPS power up self test

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Christophe Ravel]

Memory Leak (mel):
Found leaked block of size 2071 bytes at address 0x120418
At time of allocation, the call stack was:
	[1] PR_Malloc() at line 467 in "prmem.c"
	[2] PL_ArenaAllocate() at line 214 in "plarena.c"
	[3] PORT_ArenaAlloc() at line 243 in "secport.c"
	[4] PORT_ArenaZAlloc() at line 265 in "secport.c"
	[5] EC_DecodeParams() at line 675 in "ecdecode.c"
	[6] sftk_fips_ECDSA_PowerUpSelfTest() at line 1541 in "fipstest.c"
	[7] sftk_fipsPowerUpSelfTest() at line 1911 in "fipstest.c"
	[8] FC_Initialize() at line 447 in "fipstokn.c"
	[9] secmod_ModuleInit() at line 150 in "pk11load.c"
	[10] SECMOD_LoadPKCS11Module() at line 322 in "pk11load.c"
	[11] SECMOD_LoadModule() at line 323 in "pk11pars.c"
	[12] SECMOD_LoadModule() at line 338 in "pk11pars.c"
	[13] 0xe9517040
	[14] NSS_Initialize() at line 592 in "nssinit.c"
	[15] main() at line 1902 in "selfserv.c"

Comment 1

[Julien Pierre]

This was introduced in bug 349965 . This is a one-time leak during softoken initialization, so marking P2.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

A "Major" bug is one that causes a major feature to be missing or not working,
or that creates a significant usability problem.  A small one-time leak doesn't
do those things.  So, this bug isn't major.

Comment 3

[glen beasley]

Attachment: free ecparams arena

Comment 4

[glen beasley]

Attachment: fix mem leak for ecparams

Comment 5

[Wan-Teh Chang]

Comment on attachment 237405 [details] [diff] [review]
free ecparams arena


>+    /* free the memory for the ecparams */
>+    if (ecparams->arena != NULL) {
>+        PORT_FreeArena(ecparams->arena, PR_FALSE);
>+    }

The test should be if (ecparams != NULL).

You also need to free ecparams->arena before returning here:

1550     if (EC_NewKeyFromSeed(ecparams, &ecdsa_private_key, ecdsa_Known_Seed, 
1551         sizeof(ecdsa_Known_Seed)) != SECSuccess) {
1552         return( CKR_DEVICE_ERROR );
1553     }

In fact, since ecparams won't be used after the
EC_NewKeyFromSeed call, you can free its arena like this:

    ecdsastatus = EC_NewKeyFromSeed(ecparams, &ecdsa_private_key, ecdsa_Known_Seed, 
        sizeof(ecdsa_Known_Seed));
    PORT_FreeArena(ecparams->arena, PR_FALSE);
    ecparams = NULL;
    if (ecdsastatus != SECSuccess) {
        return( CKR_DEVICE_ERROR );
    }

By the way, in

1617     if (ecdsa_private_key->ecParams.arena != NULL) {
1618         PORT_FreeArena(ecdsa_private_key->ecParams.arena, PR_FALSE);
1619     }

The test should be if (ecdsa_private_key != NULL).

Comment 6

[glen beasley]

Attachment: fix mem leak for ecparams

Comment 7

[Julien Pierre]

Comment on attachment 237412 [details] [diff] [review]
fix mem leak for ecparams 

Sorry, I meant to r+ this patch. Looks like you attached twice the same patch.

Comment 8

[Julien Pierre]

Comment on attachment 237413 [details] [diff] [review]
fix mem leak for ecparams 

This is a duplicate of the second patch. And you can't review your own patch ;)

Comment 9

[glen beasley]

Attachment: memfix2 per wan-teh

Comment 10

[Wan-Teh Chang]

Comment on attachment 237420 [details] [diff] [review]
memfix2 per wan-teh

r=wtc.

>+    /* free the ecparams they are no longer needed */
>+    if (ecparams != NULL) {
>+        PORT_FreeArena(ecparams->arena, PR_FALSE);
>+        ecparams = NULL;
>+    }

At this point, ecparams cannot be NULL, so the check
should be removed.

Comment 11

[glen beasley]

removed the check for ecparams != NULL since not needed. 
checked in on the NSS_3_11 branch and the tip.
/cvsroot/mozilla/security/nss/lib/softoken/fipstest.c,v  <--  fipstest.c
new revision: 1.13.2.7; previous revision: 1.13.2.6
done
Checking in fipstest.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstest.c,v  <--  fipstest.c
new revision: 1.20; previous revision: 1.19
done