[Fedora-directory-devel] Re: memberOf plug-in memory leaks and bugs, proposals of patch

[Nathan Kinder]

Andrey Ivanov wrote:
>  Hi,
>
>  I have tried to adapt  the memberOf plugin as it exists in today in
>  the CVS to our production environment - it is a very nice and USEFUL
>  addition to server features, especially if one has to manage direct
>  and indirect groups.
>
>  We have several relatively large groups (~ 3000 entries). It turned
>  out that at each full group members delete and recreation I've had
>  huge memory leaks (~ 100 Mb per each full delete/recreation). I've
>  tried to narrow it down in the plug-in. here is what i have found (i
>  structured it in paragraphs) :
>
>  §1.There  are several slapi_search_internal_get_entry() calls that
>  return a copy of Slapi_Entry *e that is never released by
>  slapi_entry_free(). Here are the proposed changes :
>  --- /tmp/fedora-ds-base-cvs/ldap/servers/plugins/memberof/memberof.c
>   2008-02-19 07:04:56.000000000 +0100
>  +++ /tmp/memberof-patched.c     2008-03-23 22:13:00.000000000 +0100
>  @@ -1019,6 +1019,7 @@
>          }
>
>   bail:
>  +     slapi_entry_free(e);
>          return rc;
>   }
>
>  @@ -1310,6 +1311,7 @@
>          }
>
>          slapi_sdn_free(&sdn);
>  +      slapi_entry_free(group_e);
>          return rc;
>   }
>
>  @@ -1826,6 +1828,8 @@
>
>   bail:
>          slapi_ch_free_string(&filter_str);
>  +      slapi_entry_free(group_e);
>  +      slapi_entry_free(opto_e);
>
>          slapi_log_error( SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM,
>                  "<-- memberof_is_legit_member\n" );
>
>  §2. I don't know whether the arrays pre_array and post_array in
>  memberofd_replace_list() function should be freed. I think they should
>  because they are allocated by slapi_ch_malloc(), but i'm not sure and
>  i don't know how i should free them - i am not a specialist of the
>  plugin API. Anyway, apparently this part of code is never used by our
>  group management applications so i don't have a memory leak caused by
>  this function.
>
>  §3. If we make MODRDN on an INDIRECT group the original indirect group
>  DN is deleted from memberof but the renamed indirect group DN is not
>  added. It may be fixed by running every once in a while the memberOf
>  fixup task BUT in this particular case the attribute will never be
>  fixed - because of the problem in §4 :
>
>  §4. I propose to delete all the memberOf attribute values at the
>  beginning of the fix thread. Otherwise if we have in the entry a
>  "memberOf" attribute value equal to a DN of a direct group,  the
>  memeberOf fix task (the function memberof_fix_memberof_callback())
>  will NEVER add the indirect groupsDNs that depend on the
>  abovementioned direct group, so this thread is not really a FULL FIX
>  thread :) Here are the proposed changes to avoid it:
>  --- /tmp/fedora-ds-base-cvs/ldap/servers/plugins/memberof/memberof.c
>   2008-02-19 07:04:56.000000000 +0100
>  +++ /tmp/memberof-patched.c     2008-03-23 22:13:00.000000000 +0100
>  @@ -2006,6 +2010,7 @@
>   /* memberof_fix_memberof_callback()
>    * Add initial and/or fix up broken group list in entry
>    *
>  + * 0. Delete all the memberOf attribute values
>    * 1. Make sure direct membership groups are in the entry
>    * 2. Add all groups that current group list allows through nested membership
>    * 3. Trim groups that have no relationship to entry
>  @@ -2016,6 +2021,9 @@
>          char *dn = slapi_entry_get_dn(e);
>          memberof_add_groups data = {dn, dn};
>
>  +       /* step 0. */
>  +       slapi_entry_attr_delete(e, MEMBEROF_ATTR);
>  +
>          /* step 1. and step 2. */
>          rc = memberof_call_foreach_dn(0, dn, MEMBEROF_GROUP_ATTR,
>                  memberof_add_groups_search_callback, &data);
>
>  §5. After the code changes proposed in "§1" i still have ~600 bytes
>  memory increase per 5000 modifications (2500 DEL & 2500 ADDs to group
>  membership, the memory increases mainly at DEL operations). I don't
>  know whether it is due to connection management in slapd code or to
>  the plug-in itself but it is a much more acceptable value for me than
>  100Mb :)
>
>  §6. #define MEMBEROF_GROUP_ATTR_TYPE "uid" is declared but never used
>  in the plug-in code...
>
>  Thanks once more for this plug-in!
>     
Thank you for the information Andrey.  I'm currently doing some cleanup 
on the memberOf plug-in around the implementation of the cleanup task.  
Once I am finished with that work, I'll look into the issues that you 
raised.

We are working on making the memberOf plug-in more robust, as it's 
currently in a sort of "alpha" stage as you've noticed.  If you are 
interested in helping with this effort (or just following it), take a 
look at the design document for this plug-in on our wiki 
(http://directory.fedoraproject.org/wiki/MemberOf_Plugin).

Thanks again!
-NGK