Bug 548535

Summary: memory leak in attrcrypt
Product: [Retired] 389 Reporter: Noriko Hosoi <nhosoi>
Component: Database - GeneralAssignee: Noriko Hosoi <nhosoi>
Status: CLOSED CURRENTRELEASE QA Contact: Viktor Ashirov <vashirov>
Severity: low Docs Contact:
Priority: medium    
Version: 1.3.0CC: jgalipea, rmeggins
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-12-07 17:03:54 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 434914, 543590    
Attachments:
Description Flags
git patch file
rmeggins: review+
git patch file (revised based upon the comment from Rich)
none
mmraccept valgrind logs none

Description Noriko Hosoi 2009-12-17 18:04:21 UTC 
Description of problem:
Found in TET mmrepl/accept + valgrind
accept.vg
==31192== 1,392 (48 direct, 1,344 indirect) bytes in 2 blocks are definitely lost in loss record 32 of 80
==31192==    at 0x4A0776F: realloc (vg_replace_malloc.c:429)
==31192==    by 0x4C50531: slapi_ch_realloc (ch_malloc.c:199)
==31192==    by 0xB241EA2: attrcrypt_acs_list_add (ldbm_attrcrypt.c:449)
==31192==    by 0xB242047: attrcrypt_init (ldbm_attrcrypt.c:483)
==31192==    by 0xB2184FD: dblayer_instance_start (dblayer.c:1840)
==31192==    by 0xB234E82: bulk_import_start (import-threads.c:1789)
==31192==    by 0xB235693: ldbm_back_wire_import (import-threads.c:2001)
==31192==    by 0x4C4F627: process_bulk_import_op (bulk_import.c:176)
==31192==    by 0x4C4F381: slapi_start_bulk_import (bulk_import.c:76)
==31192==    by 0xB4C6B2D: multimaster_extop_StartNSDS50ReplicationRequest (repl_extop.c:835)
==31192==    by 0x4C99634: plugin_call_exop_plugins (plugin.c:424)
==31192==    by 0x41B28E: do_extended (extendop.c:349)
==31192==    by 0x413417: connection_dispatch_operation (connection.c:617)
==31192==    by 0x4149A1: connection_threadmain (connection.c:2267)
==31192==    by 0x32B7429262: (within /lib64/libnspr4.so)
==31192==    by 0x37A6406869: start_thread (in /lib64/libpthread-2.10.1.so)
==31192==    by 0x37A58DE3BC: clone (in /lib64/libc-2.10.1.so)

Note: attrcrypt_cleanup never be called.
$ egrep attrcrypt_cleanup *.[ch]
ldbm_attrcrypt.c:attrcrypt_cleanup(attrcrypt_cipher_state *acs)
ldbm_attrcrypt.c:    LDAPDebug(LDAP_DEBUG_TRACE,"-> attrcrypt_cleanup\n", 0, 0, 0);
ldbm_attrcrypt.c:    LDAPDebug(LDAP_DEBUG_TRACE,"<- attrcrypt_cleanup\n", 0, 0, 0);
Comment 2 Noriko Hosoi 2010-02-06 02:03:22 UTC 
Created attachment 389229 [details]
git patch file

Files:
	ldap/servers/slapd/back-ldbm/dblayer.c
	ldap/servers/slapd/back-ldbm/ldbm_attrcrypt.c
	ldap/servers/slapd/back-ldbm/proto-back-ldbm.h

Description:
The attrcrypt module maintains the inst_attrcrypt_state_private
field in the instance structure (ldbm_instance) to store the private
keys.  The area and the space for the private keys are allocated in
attrcrypt_init which is called from dblayer_instance_start.
A backend instance could be closed and restarted multiple times
(for instance, in the bulk_import, which is used by the replica
initialization), but the area had no chance to be freed.
This patch is adding the clean-up code.
Comment 3 Rich Megginson 2010-02-08 15:18:04 UTC 
Comment on attachment 389229 [details]
git patch file

The formatting in attrcrypt_cleanup_private() of the LDAPDebug macros is not correct - looks like you need tabs there.

Otherwise, looks good.
Comment 4 Noriko Hosoi 2010-02-08 16:55:01 UTC 
Created attachment 389573 [details]
git patch file (revised based upon the comment from Rich)

(In reply to comment #3)
> (From update of attachment 389229 [details])
> The formatting in attrcrypt_cleanup_private() of the LDAPDebug macros is not
> correct - looks like you need tabs there.
> 
> Otherwise, looks good.    

Rich, thank you for reviewing the patch!

Pushed to master.

$ git merge work
Updating 5c859f5..571f580
Fast forward
 ldap/servers/slapd/back-ldbm/dblayer.c         |    8 ++++++-
 ldap/servers/slapd/back-ldbm/ldbm_attrcrypt.c  |   28 +++++++++++++++++++++++-
 ldap/servers/slapd/back-ldbm/proto-back-ldbm.h |    1 +
 3 files changed, 35 insertions(+), 2 deletions(-)
$ git push
Counting objects: 17, done.
Delta compression using 4 threads.
Compressing objects: 100% (9/9), done.
Writing objects: 100% (9/9), 1.47 KiB, done.
Total 9 (delta 7), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
   5c859f5..571f580  master -> master
Comment 5 Jenny Severance 2010-06-03 17:06:19 UTC 
Created attachment 419447 [details]
mmraccept valgrind logs
Comment 6 Jenny Severance 2010-06-03 17:18:02 UTC 
please review the attached valgrind logs for memory leak fix.  Thanks!
Comment 7 Noriko Hosoi 2010-06-03 17:39:23 UTC 
Thanks, Jenny.  The particular leak does not exist in the valgrind logs.

$ tar xvf ../valgrind_mmraccept.tar 
accept.vg.12390.12396
accept.vg.12697.12703
accept.vg.12994.13000
[...]
accept.vg.7935.7941
accept.vg.8372.8378
accept.vg.8811.8817
$ egrep attrcrypt_acs_list_add *
$
Comment 8 Jenny Severance 2010-06-03 17:59:51 UTC 
verified - RHEL 4 - Thanks Noriko!

version:
redhat-ds-base-8.2.0-2010060204.el4dsrv