367133 - Possible leaks in nss_Init() when incorrect cipher set.

Status: RESOLVED DUPLICATE of bug 367037

(NSS :: Libraries, defect)

Description

[Slavomir Katuscak]

In nss_Init() there is STAN_LoadDefaultNSS3TrustDomain() call in loser part (called when init is not successfull). From this function there are called many others that allocates some memory, which is later not released.

There are 65 possible leaks like this:

Possible memory leak -- address in block (aib):
Found 2 leaked blocks with total size 4142 bytes
At time of each allocation, the call stack was:
	[1] PR_Malloc() at line 467 in "prmem.c"
	[2] PL_ArenaAllocate() at line 214 in "plarena.c"
	[3] nss_zalloc_arena_locked() at line 832 in "arena.c"
	[4] nss_ZAlloc() at line 928 in "arena.c"
	[5] nssSlot_CreateFromPK11SlotInfo() at line 150 in "dev3hack.c"
	[6] nssToken_CreateFromPK11SlotInfo() at line 202 in "dev3hack.c"
	[7] STAN_InitTokenForSlotInfo() at line 104 in "pki3hack.c"
	[8] STAN_LoadDefaultNSS3TrustDomain() at line 157 in "pki3hack.c"
	[9] nss_Init() at line 493 in "nssinit.c"
	[10] NSS_Initialize() at line 592 in "nssinit.c"
	[11] main() at line 1441 in "strsclnt.c"

I will add complete list as attachement.
This list contains also leaks from bug 291225.
See also bug 267037 which describes other problem in situation when there is incorrect cipher set.

Comment 1

[Slavomir Katuscak]

In description there should be bug 367037 instead of 267037.
And NSS_DISABLE_ARENA_FREE_LIST was set to value 1.

Comment 2

[Slavomir Katuscak]

Attachment: List of memory leaks.

These leaks were found when trying cipher z. Tested on both Sparc and x86 version of Solaris/DBX (machines mace and mandela). They were not found by 64bit version of DBX (machine biarritz).

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Slavo,  what is the exact command that starts the process in which these leak
stacks appear?  

It appears to me that that program simply is not shutting down NSS.

Comment 4

[Slavomir Katuscak]

Hi Nelson,

Steps to reproduce:

Start selfserv:
selfserv -D -p 8443 -d
/share/builds/mccrel3/security/securitytip/builds/20070111.1/wozzeck_Solaris8/mozilla/tests_results/security/mace.5/server_memleak
-n mace.red.iplanet.com -e mace.red.iplanet.com-ec -w nss -c
ABCDEF:C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D:C00E:C00F:C010:C011:C012:C013:C014cdefgijklmnvyz
-t 5 

Test strsclnt with incorrect cipher:
strsclnt -q -p 8443 -d
/share/builds/mccrel3/security/securitytip/builds/20070111.1/wozzeck_Solaris8/mozilla/tests_results/security/mace.5/client_memleak
-w nss -c 1000 -C z mace.red.iplanet.com

See also bug 367037.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Slavo, are these leaks in strsclnt or in selfserv?  
The information in this bug report simply doesn't tell me that essential detail.

Comment 6

[Slavomir Katuscak]

These leaks are in strsclnt.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Does strsclnt output any error messages to stderr when it runs this test?

Comment 8

[Slavomir Katuscak]

Yes, it prints usage page (when cipher z is used) as described in bug 367037:

strsclnt -q -p 8443 -d
/share/builds/mccrel3/security/securitytip/builds/20070111.1/wozzeck_Solaris8/mozilla/tests_results/security/mace.5/client_memleak
-w nss -c 1000 -C z mace.red.iplanet.com
Usage: strsclnt [-n nickname] [-p port] [-d dbdir] [-c connections]
          [-23BDNTovqs] [-f filename] [-N | -P percentage]
          [-w dbpasswd] [-C cipher(s)] [-t threads] hostname
 where -v means verbose
       -o flag is interpreted as follows:
          1 -o   means override the result of server certificate validation.
          2 -o's mean skip server certificate validation altogether.
       -D means no TCP delays
       -q means quit when server gone (timeout rather than retry forever)
       -s means disable SSL socket locking
       -N means no session reuse
       -P means do a specified percentage of full handshakes (0-100)
       -2 means disable SSL2
       -3 means disable SSL3
       -T means disable TLS
       -U means enable throttling up threads
       -B bypasses the PKCS11 layer for SSL encryption and MACing

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

All these leaks are a direct consequence of bug 367037. 
strsclnt should NEVER call the Usage() function after initializing NSS.
The Usage() function calls exit() without shutting down NSS and NSPR first.

The fix for bug 367037 will be to change function client_main so that it
never calls Usage() or exit(), but instead to print a meaningful error 
message (one that identifies the specific bad cipher suite), set the 
global boolean failed_already to true, and returns.  Then the code will
continue to shutdown NSS and NSPR, plugging this leak.

The fix for bug 367037 will also fix this bug.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

I'm marking this bug a duplicate of bug 367037, because there's only
one real flaw in the code, and this bug is just another manifestation 
of that same flaw.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Sorry, I dup'ed this bug to the wrong bug.  I will correct it now.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

I was right the first time.  Sorry for the confusion.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Slavo, I hace committed the fix for bug 367037 on the trunk.  That should
also fix this bug, and bug 373297.  Please run your memory leak tests on 
the NSS trunk, and let me know whether or not all the leaks related to the 
bad 'z' cipher have been fixed.  If this bug and bug 373297 have been fixed,
please mark this bug verified.

Note: I haven't fixed the 'z' cipher yet, I have only fixed the error message 
and the leaks.  I will fix the z cipher suite AFTER these 3 bugs are verified
on the trunk: bug 367037, Bug 367133, bug 373297.  I'd like to do that on
Monday, 3/12.

Comment 15

[Slavomir Katuscak]

Not found in last memleak tests. Seems to be OK now.