354313 - STAN_GetCERTCertificateName leaks "instance" struct

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[kmaraas]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; nb-NO; rv:1.8.0.7) Gecko/20060918 Fedora/1.5.0.7-3.fc6 Firefox/1.5.0.7 pango-text
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; nb-NO; rv:1.8.0.7) Gecko/20060918 Fedora/1.5.0.7-3.fc6 Firefox/1.5.0.7 pango-text

I see these reported by valgrind when running evolution under it. Could be a problem on the evolution side, but I don't know NSS well enough to be certain. Please give directions on how to describe the leakage if I need to file a bug against evolution :-)

==26015== 18,541 (6,868 direct, 11,673 indirect) bytes in 287 blocks are definitely lost in loss record 253 of 281
==26015==    at 0x400473F: calloc (vg_replace_malloc.c:279)
==26015==    by 0x22DC53B: PR_Calloc (prmem.c:474)
==26015==    by 0x262B5EF: nss_ZAlloc (arena.c:891)
==26015==    by 0x2629A40: nssCryptokiObject_Clone (devutil.c:103)
==26015==    by 0x2624A82: get_cert_instance (pki3hack.c:650)
==26015==    by 0x2625A30: STAN_GetCERTCertificateName (pki3hack.c:709)
==26015==    by 0x25E875F: pk11ListCertCallback (pk11cert.c:2300)
==26015==    by 0x2622883: nssPKIObjectCollection_Traverse (pkibase.c:978)
==26015==    by 0x261F9A9: NSSTrustDomain_TraverseCertificates (trustdomain.c:1085)
==26015==    by 0x25E848C: PK11_ListCerts (pk11cert.c:2350)
==26015==    by 0x6054510: load_certs (certificate-manager.c:937)
==26015==    by 0x60554E5: certificate_manager_config_control_new (certificate-manager.c:953)
==26015==    by 0x5F8CF36: factory (component-factory.c:70)
==26015==    by 0x461F1CD: bonobo_marshal_OBJECT__STRING (bonobo-marshal.c:203)
==26015==    by 0x4E1EE7A: g_closure_invoke (gclosure.c:490)
==26015==    by 0x461DCD3: bonobo_closure_invoke_va_list (bonobo-types.c:404)
==26015==    by 0x461DF3B: bonobo_closure_invoke (bonobo-types.c:467)
==26015==    by 0x46094E7: bonobo_generic_factory_new_generic (bonobo-generic-factory.c:277)
==26015==    by 0x4617713: bonobo_shlib_factory_new_generic (bonobo-shlib-factory.c:155)
==26015==    by 0x46097B8: impl_Bonobo_ObjectFactory_createObject (bonobo-generic-factory.c:82)
==26015==    by 0x46A6120: _ORBIT_skel_small_Bonobo_GenericFactory_createObject (Bonobo_GenericFactory-common.c:16)
==26015==    by 0x47155BF: ORBit_c_stub_invoke (poa.c:2630)
==26015==    by 0x46A7CE2: Bonobo_GenericFactory_createObject (Bonobo_GenericFactory-stubs.c:13)
==26015==    by 0x46A9D82: bonobo_activation_activate_shlib_server (bonobo-activation-shlib.c:203)
==26015==    by 0x46AA6FD: handle_activation_result (bonobo-activation-activate.c:315)
==26015==    by 0x46AAE49: bonobo_activation_activate (bonobo-activation-activate.c:395)
==26015==    by 0x46AB047: bonobo_activation_activate_from_id (bonobo-activation-activate.c:437)
==26015==    by 0x8056A9A: e_shell_settings_dialog_init (e-shell-settings-dialog.c:215)
==26015==    by 0x4E3EEA9: g_type_create_instance (gtype.c:1567)
==26015==    by 0x4E26031: g_object_constructor (gobject.c:1038)

And

==26015== 8,408 (2,071 direct, 6,337 indirect) bytes in 1 blocks are definitely lost in loss record 256 of 281
==26015==    at 0x4005400: malloc (vg_replace_malloc.c:149)
==26015==    by 0x22DC307: PR_Malloc (prmem.c:467)
==26015==    by 0x229CEAF: PL_ArenaAllocate (plarena.c:214)
==26015==    by 0x261AFF0: PORT_ArenaAlloc (secport.c:243)
==26015==    by 0x25F4622: PK11_GetAttributes (pk11obj.c:241)
==26015==    by 0x25F48DF: PK11_MatchItem (pk11obj.c:1474)
==26015==    by 0x26264AB: nssToken_IsPrivateKeyAvailable (devtoken.c:1717)
==26015==    by 0x261D159: NSSCertificate_IsPrivateKeyAvailable (certificate.c:769)
==26015==    by 0x2624B16: nssTrust_GetCERTCertTrustForCert (pki3hack.c:622)
==26015==    by 0x2625CDF: stan_GetCERTCertificate (pki3hack.c:788)
==26015==    by 0x25E85FA: pk11ListCertCallback (pk11cert.c:2289)
==26015==    by 0x2622883: nssPKIObjectCollection_Traverse (pkibase.c:978)
==26015==    by 0x261F9A9: NSSTrustDomain_TraverseCertificates (trustdomain.c:1085)
==26015==    by 0x25E848C: PK11_ListCerts (pk11cert.c:2350)
==26015==    by 0x6054510: load_certs (certificate-manager.c:937)
==26015==    by 0x60554E5: certificate_manager_config_control_new (certificate-manager.c:953)
==26015==    by 0x5F8CF36: factory (component-factory.c:70)
==26015==    by 0x461F1CD: bonobo_marshal_OBJECT__STRING (bonobo-marshal.c:203)
==26015==    by 0x4E1EE7A: g_closure_invoke (gclosure.c:490)
==26015==    by 0x461DCD3: bonobo_closure_invoke_va_list (bonobo-types.c:404)
==26015==    by 0x461DF3B: bonobo_closure_invoke (bonobo-types.c:467)
==26015==    by 0x46094E7: bonobo_generic_factory_new_generic (bonobo-generic-factory.c:277)
==26015==    by 0x4617713: bonobo_shlib_factory_new_generic (bonobo-shlib-factory.c:155)
==26015==    by 0x46097B8: impl_Bonobo_ObjectFactory_createObject (bonobo-generic-factory.c:82)
==26015==    by 0x46A6120: _ORBIT_skel_small_Bonobo_GenericFactory_createObject (Bonobo_GenericFactory-common.c:16)
==26015==    by 0x47155BF: ORBit_c_stub_invoke (poa.c:2630)
==26015==    by 0x46A7CE2: Bonobo_GenericFactory_createObject (Bonobo_GenericFactory-stubs.c:13)
==26015==    by 0x46A9D82: bonobo_activation_activate_shlib_server (bonobo-activation-shlib.c:203)
==26015==    by 0x46AA6FD: handle_activation_result (bonobo-activation-activate.c:315)
==26015==    by 0x46AAE49: bonobo_activation_activate (bonobo-activation-activate.c:395)


Reproducible: Always

Steps to Reproduce:
1. Run Evolution in valgrind
2. 
3.

Actual Results:  
Memory leaked 

Expected Results:  
No memory leaked

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

The second of these two reports is undoubtedly a false positive.

NSS/NSPR have an "Arena free list" on which all allocated arenas are put, 
and from which new ones are taken.  We do not consider arenas on the free 
list to have been leaked, but they will appear to be leaked unless the 
application calls a function to empty the free arena list.  

The arena free list is more efficient that reallocating from the heap
every time an arena is allocated, but it has a number of undesirable
effects on leak testing.  
(a) it makes arenas on the free list appear to have been leaked, and 
(b) for arenas that really are leaked, leak testers will show the stack
of the code that first allocated the arena (and put it on the arena 
free list) rather than the stack of the code that took it off the free 
list and then really leaked it.  

There is also an environment variable that disables the arena free list.  
We recommend that people doing leak testing use that environment variable
to overcome the above two issues.  Set NSS_DISABLE_ARENA_FREE_LIST to "1"
in the program's environment before testing/running it.  The program may 
run slightly slower but will not have any false positive arena leaks.

I'll look at the first or the two reports next.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

It appears to me that STAN_GetCERTCertificateName leaks its "instance".
This is called from PK11_ListCerts.  I'll bet our leak testing never 
calls that functions.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

taking

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

Will request review after I test it.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 240192 [details] [diff] [review]
patch v1

tested now.  seems to work OK.

Was a bear to test.  This function only gets called from clients that do client authentication, in their callbacks for cert selection.  No wonder this bug went undetected for so long.

Comment 6

[Robert Relyea]

Comment on attachment 240192 [details] [diff] [review]
patch v1

It's certainly a leak, and this code certainly fixes it.

r+ = rrelyea

Comment 7

[Julien Pierre]

Our memory leak testing should have found this. We are testing both client and server sides. Christophe, are we not doing any client auth tests during the leak tests ?

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

> are we not doing any client auth tests during the leak tests ?

It happens that, as a coincidence, some NSS test programs that do SSL
client auth use a function that calls the leaking function.  But I
do not think we should rely on SSL client auth to leak test this function.

Some parts of NSS need to be tested as a set, rather than tested individually.
One cannot test certain SSL-related functions without a connected SSL socket,
for example, and so it makes sense to test such functions as part of a 
"system test" that tests all of SSL.  

But not all of NSS should be tested using only "system tests".  Many of 
NSS's functions can be tested with a minimum of other NSS functions being
needed for the test.  Such functions should be tested as "unit tests".

The functions that were at fault here are, IMO, excellent candidates for
unit testing.  We need unit test programs for them.

In this case, we need a program that will call PK11_ListCerts on a cert DB
with user certs (certs with private keys).  certutil doesn't do it.  
Some programs that do client auth call it, but we shouldn't need to setup
SSL servers to test this function.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Checking in pki3hack.c; new revision: 1.91;      previous revision: 1.90
Checking in pki3hack.c; new revision: 1.86.28.5; previous revision: 1.86.28.4