366553 - libSSL leaks global array of trusted client auth CA names

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Slavomir Katuscak]

I ran selfserv under DBX on Solaris (both Sparc and X86) and I found these leaks related to CERT_GetSSLCACerts():

Possible memory leak -- address in block (aib):
Found leaked block of size 2071 bytes at address 0x15d6c0
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] CERT_GetSSLCACerts() at line 620 in "certhigh.c"
	[5] SSL_ConfigSecureServer() at line 739 in "sslsecur.c"
	[6] server_main() at line 1437 in "selfserv.c"
	[7] main() at line 2026 in "selfserv.c"

Possible memory leak -- address in block (aib):
Found leaked block of size 88 bytes at address 0x15a9c0
At time of allocation, the call stack was:
	[1] calloc() at 0xe0937488 
	[2] PR_Calloc() at line 475 in "prmem.c"
	[3] PR_NewLock() at line 174 in "ptsynch.c"
	[4] PORT_NewArena() at line 203 in "secport.c"
	[5] CERT_GetSSLCACerts() at line 613 in "certhigh.c"
	[6] SSL_ConfigSecureServer() at line 739 in "sslsecur.c"
	[7] server_main() at line 1437 in "selfserv.c"
	[8] main() at line 2026 in "selfserv.c"

Possible memory leak -- address in block (aib):
Found leaked block of size 44 bytes at address 0x1599b0
At time of allocation, the call stack was:
	[1] calloc() at 0xe0937488 
	[2] PR_Calloc() at line 475 in "prmem.c"
	[3] PORT_ZAlloc() at line 140 in "secport.c"
	[4] PORT_NewArena() at line 198 in "secport.c"
	[5] CERT_GetSSLCACerts() at line 613 in "certhigh.c"
	[6] SSL_ConfigSecureServer() at line 739 in "sslsecur.c"
	[7] server_main() at line 1437 in "selfserv.c"
	[8] main() at line 2026 in "selfserv.c"

It seems that CERT_GetSSLCACerts() is called from SSL_ConfigSecureServer() to get list of CAs (just one time), and this list is never (or incorrectly) released.

See also bug 222707 and bug 308275, it's possible that it's the same problem.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Slavomir, did you have the environment variable NSS_DISABLE_ARENA_FREE_LIST
defined (set to 1) in the environment of the process being tested?
Please answer that question by adding a comment in this bug.
That's a prerequisite to leak testing with NSS.  

Comment 2

[Slavomir Katuscak]

Yes, I set this variable in initialization part of my script. I just performed some experiments and I have the same results with NSS_DISABLE_ARENA_FREE_LIST set to 1 as with undefined. Is it enough to set this variable before testing, or does it affect also build process ?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

taking.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: tested patch, v1

I will not ask for review until I've tested this patch.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 268410 [details] [diff] [review]
tested patch, v1

All.sh passes with this patch.
Wan-Teh, please review.

Comment 6

[Wan-Teh Chang]

Comment on attachment 268410 [details] [diff] [review]
tested patch, v1

>+    if (PR_SUCCESS == PR_CallOnceWithArg(&setupServerCAListOnce, 
>+                                         &serverCAListSetup,
>+                                         (void *)(ss->dbHandle))) {
>+	return SECSuccess;
>+    }

CERT_GetSSLCACerts ignores its argument , so you can call
PR_CallOnce and just pass NULL to CERT_GetSSLCACerts.
In fact, ss->dbHandle is NULL anyway.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Checking in sslsecur.c; new revision: 1.40; previous revision: 1.39

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Want fix in 3.11.8 also

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 268410 [details] [diff] [review]
tested patch, v1

this patch also applies cleanly to the branch, and Slavo desires this fix on the 3.11 branch also.  
Bob, please review for the 3.11 branch.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 268410 [details] [diff] [review]
tested patch, v1

Julien could review this for the branch, too.

Comment 11

[Robert Relyea]

Comment on attachment 268410 [details] [diff] [review]
tested patch, v1

r+ =rrelyea

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Checking in sslsecur.c; new revision: 1.34.2.6; previous revision: 1.34.2.5

Comment 13

[Slavomir Katuscak]

Nelson, please back out this patch in branch, it causes build failure:

cc -o SunOS5.8_OPT.OBJ/sslsecur.o -c -xO4 -KPIC -DSVR4 -DSYSV -D__svr4 -D__svr4__ -DSOLARIS -D_REENTRANT -DSOLARIS2_8 -D_SVID_GETTOD -xarch=v8 -DXP_UNIX -DNSS_ENABLE_ECC -UDEBUG -DNDEBUG -DNSS_ENABLE_ECC -DNSS_ECC_MORE_THAN_SUITE_B -I/usr/dt/include -I/usr/openwin/include -I../../../../dist/SunOS5.8_OPT.OBJ/include  -I../../../../dist/public/nss -I../../../../dist/private/nss  sslsecur.c
"sslsecur.c", line 647: warning: no explicit type given
"sslsecur.c", line 647: syntax error before or at: pristineCallOnce
"sslsecur.c", line 647: warning: old-style declaration or incorrect type for: pristineCallOnce
"sslsecur.c", line 648: warning: no explicit type given
"sslsecur.c", line 648: identifier redeclared: PRCallOnceType
	current : static int
	previous: const int : "sslsecur.c", line 647
"sslsecur.c", line 648: syntax error before or at: setupServerCAListOnce
"sslsecur.c", line 648: warning: old-style declaration or incorrect type for: setupServerCAListOnce
"sslsecur.c", line 664: warning: implicit function declaration: NSS_RegisterShutdown
"sslsecur.c", line 792: warning: implicit function declaration: PR_CallOnceWithArg
cc: acomp failed for sslsecur.c
gmake-3.79[3]: *** [SunOS5.8_OPT.OBJ/sslsecur.o] Error 2
gmake-3.79[3]: Leaving directory `/share/builds/mccrel3/security/securityjes5/builds/20070925.1/wozzeck_Solaris8/mozilla/security/nss/lib/ssl'
gmake-3.79[2]: *** [libs] Error 2
gmake-3.79[2]: Leaving directory `/share/builds/mccrel3/security/securityjes5/builds/20070925.1/wozzeck_Solaris8/mozilla/security/nss/lib'
gmake-3.79[1]: *** [libs] Error 2
gmake-3.79[1]: Leaving directory `/share/builds/mccrel3/security/securityjes5/builds/20070925.1/wozzeck_Solaris8/mozilla/security/nss'
gmake-3.79: *** [nss] Error 2

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

No, I checked in the missing #include.
I need to figure out why prinit.h gets included on Windows but not 
on any other platform.  This same problem has happened in several
other source files.  They build OK on windows, because prinit.h
gets mysteriously included on Windows, but they don't build on 
other platforms until an explicit #include "prinit.h" gets added 
into the file.  

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

OK, got it.  It's not Windows vs Unix.  It's DBG vs. OPT.
All .c files in lib/ssl #include "sslimpl.h".
In DEBUG builds (only), sslimpl.h does #include "private/pprthred.h",
a private NSPR header, in order to get the declaration of PR_InMonitor().
"private/pprthred.h" include's nspr.h, which includes all NSPR public headers, 
including prinit.h, which declares NSPR's callOnce functions and types.

So, essentially, in libSSL, in DEBUG builds, all NSPR header files get 
included whether the SSL source file explicitly includes them or not.  
I think that "private/pprthred.h" should not include nspr.h.  
I will file an NSPR bug about that.