Last Comment Bug 354313 - STAN_GetCERTCertificateName leaks "instance" struct
: STAN_GetCERTCertificateName leaks "instance" struct
Status: RESOLVED FIXED
: mlk
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11
: x86 All
: P2 normal (vote)
: 3.11.4
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
Mentors:
: 291228 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-26 06:00 PDT by kmaraas
Modified: 2014-12-07 00:54 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (930 bytes, patch)
2006-09-26 12:21 PDT, Nelson Bolyard (seldom reads bugmail)
julien.pierre: review+
rrelyea: superreview+
Details | Diff | Splinter Review

Description kmaraas 2006-09-26 06:00:55 PDT
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) 2006-09-26 10:42:21 PDT
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) 2006-09-26 10:55:42 PDT
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) 2006-09-26 11:25:51 PDT
taking
Comment 4 Nelson Bolyard (seldom reads bugmail) 2006-09-26 12:21:10 PDT
Created attachment 240192 [details] [diff] [review]
patch v1

Will request review after I test it.
Comment 5 Nelson Bolyard (seldom reads bugmail) 2006-09-26 14:45:02 PDT
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 2006-09-26 14:54:15 PDT
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 2006-09-26 15:01:10 PDT
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) 2006-09-26 18:28:21 PDT
> 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) 2006-09-30 22:43:32 PDT
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
Comment 10 :Cykesiopka 2014-12-07 00:54:39 PST
*** Bug 291228 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.