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

[Ivanov Andrey (M.)]

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!

[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