[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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3241 bytes
Desc: S/MIME Cryptographic Signature
Url : http://lists.fedoraproject.org/pipermail/389-devel/attachments/20080324/4b0865c7/attachment.bin