The memberOf plug-in is currently hardcoded to use the "member" and "memberOf" attributes for group membership. These attributes should be configurable. The attached diffs allow these attributes to be configured in the plug-in configuration entry. The configuration can be dynamically changed over LDAP with the server running. We ensure that the configuration doesn't change while a memberOf operation is in progress by obtaining the memberOf lock while the changes are applied. I also made the filter that is used to check if a group membership change is made a part of the configuration struct since it is based off of one of the configurable attributes. In addition to the above changes, I removed an unnecessary function that was wrapping slapi_str2filter(). The previous code was doing a malloc of the filter string, needlessly duplicating the string, then creating the Slapi_Filter (which does a malloc as well). The two copies of the filter string were then just free'd. This was inefficient, so I removed the wrapper function so that we simply malloc the filter string and pass it to slapi_str2filter() to allocate the Slapi_Filter. This saves us one malloc/free.
Created attachment 309023 [details] CVS Diffs
Created attachment 309024 [details] New memberof.h header file
Created attachment 309025 [details] New memberof_config.c source file
I would strongly advise against using a global variable, convenient as it may be. I don't think there is a way to define a variable with plugin scope, at least not in a relatively sane, cross platform manner. So either declare it static in one file or the other, and provide an access function to get it. I would declare it in the file in which it is used the most, which may not be the memberof_config.c file. You could use slapi_smprintf in memberof_apply_config() to avoid the filter size + malloc + sprintf calls. In the cases where theConfig.groupattr or theConfig.group_filter are used in memberof callbacks - are you sure that's thread safe? What happens if someone modifies the memberof config in the middle of an operation to delete the dn from the groups, for example?
(In reply to comment #4) > I would strongly advise against using a global variable, convenient as it may > be. I don't think there is a way to define a variable with plugin scope, at > least not in a relatively sane, cross platform manner. So either declare it > static in one file or the other, and provide an access function to get it. I > would declare it in the file in which it is used the most, which may not be the > memberof_config.c file. Point taken. I had that setup at first with it declared static in memberof_config.c. That made it a bit cumbersome since it's used so much in memberof.c. I'll declare it static in memberof.c and add a public access function there. This will be much simpler that doing it the other way around. > > You could use slapi_smprintf in memberof_apply_config() to avoid the filter size > + malloc + sprintf calls. I'll look into doing this. > > In the cases where theConfig.groupattr or theConfig.group_filter are used in > memberof callbacks - are you sure that's thread safe? What happens if someone > modifies the memberof config in the middle of an operation to delete the dn from > the groups, for example? There do appear to be three spots where we use theConfig.group_filter just before acquiring the lock (the mod, modrdn, and add callbacks). I'll make sure the lock is acquired before using those variables. The general rule should be that theConfig is only used if you hold the memberof_operation_lock. The memberof_apply_config() function is the only function that writes to theConfig. It acquires the memberof_operation_lock before making any changes to it, then releases it once it's updated. This approach should be thread safe. I also noticed that the task callback function doesn't use the lock, which is a problem. Pete must've missed this when he did the initial implementation. This needs to use the lock so it doesn't interfere with any other ongoing memberOf operations. I'll address these issues and get a new set of diffs out. Thanks for the feedback!
Created attachment 309681 [details] CVS Diffs
Created attachment 309682 [details] New memberof.h header file
Created attachment 309683 [details] New memberof_config.c source file
I've attached the revised fix for this bug. The big changes to this revision are the introduction of a new mutex used around the main config struct. This mutex is used to prevent the configuration changes occuring while a running memberOf operation is checking the config. To prevent holding this config lock for a potentially long period of time, each memberOf thread makes a local copy of the main config once it is determined that there is indeed memberOf work to do. This copy takes place while the config lock is held. I also made sure that both the config lock and the memberOf operation lock are used by the fix-up task thread, which wasn't using any synchronization mechanism previously. I addressed the other points that Rich brought up in his review in comment #4 as well. I have tested these changes under heavy memberOf operation load of all types (ADD, DEL, MOD, MODRDN, and fix-up tasks). No deadlocks or crashes were observed, and correct memberOf attributes resulted.
I received this feedback on my fix from Noriko: "Your changes look good. I got one minor question about this diff: https://bugzilla.redhat.com/attachment.cgi?id=309681&action=diff#ldap/servers/plugins/memberof/memberof.c_sec11 interested is supposed to be reset to 0 every time in the while loop? Or could it be okay to use the same copy in the series of the mod operation? If that's the case, you could call memberof_get_config before the while loop? (I'm not sure how much accuracy we should guarantee for the config attribute value...)" This indeed needs to be adjusted. We need to be sure to reset interested between each smod. We also should not recopy the config between smods in the same modify operation. We should only copy the config once, and then only if we find that we have an operation that we actually need to act on to prevent doing a malloc for all modifies. I will attach a new set of files that addresses the issue Noriko raised as well as a change I made to use a reader/writer lock around the config instead of just a simple mutex. Using a reader/writer lock should improve performance since multiple threads will be able to read and copy the main config at the same time.
Created attachment 309753 [details] CVS Diffs
Created attachment 309754 [details] New memberof.h header file
Created attachment 309755 [details] New memberof_config.c source file
Another good finding from Noriko's review: "One more question... In this block: https://bugzilla.redhat.com/attachment.cgi?id=309753&action=diff#ldap/servers/plugins/memberof/memberof.c_sec46 The flag first_time is used to indicate to initialize the Slapi_Attr attr. I wonder if there is any chance to reset it. If not, memberof_qsort_compare never has the chance to update the attr when memberof_replace_list is called after a config update? Probably, instead of passing qsortConfig, could it be easier to pass just the up-to-dated attr? int memberof_qsort_compare(const void *a, const void *b) { static Slapi_Attr *attr = 0; static int first_time = 1; " This needs to be addressed since the internal static variable first time will never be reset to 1. The approach I prefer to take with this is to put a pointer to the needed Slapi_Attr in the config struct. This leaves most of the existing code alone since we already pass the config struct around. The memberof compare functions can then just use the copied version of this Slapi_Attr. The nice thing about this approach is that we will only allocate the Slapi_Attr when the config is modified or copied (memberof_config_apply() and memberof_config_copy()). In addition to this change, I found a double free in memberof_is_member_r() that would occur with a recursive group. This was detected in valgrind. The issue was that the memberof_is_member_r() function recurses. If it encounters a recursive grouping, it goes to a bail section, which will free the "ll" variable that was allocated by it's caller. When control returns to the caller, the caller will hit this same bail section and free it again. The fix is to simply set it "ll" to NULL when we encounter the circular grouping. This will result in the allocation and free always happening in the same function call only. New attachments to follow.
Created attachment 309804 [details] CVS Diffs
Created attachment 309805 [details] New memberof.h header file
Created attachment 309806 [details] New memberof_config.c source file
I like the new MemberOfConfig a lot. It made the code simple and clean. Also, it was really nice to find out the double free case.
Checked into ldapserver (HEAD). Thanks to Noriko and Rich for their reviews! Checking in Makefile.am; /cvs/dirsec/ldapserver/Makefile.am,v <-- Makefile.am new revision: 1.70; previous revision: 1.69 done Checking in Makefile.in; /cvs/dirsec/ldapserver/Makefile.in,v <-- Makefile.in new revision: 1.90; previous revision: 1.89 done Checking in ldap/ldif/template-dse.ldif.in; /cvs/dirsec/ldapserver/ldap/ldif/template-dse.ldif.in,v <-- template-dse.ldif.in new revision: 1.8; previous revision: 1.7 done Checking in ldap/servers/plugins/memberof/memberof.c; /cvs/dirsec/ldapserver/ldap/servers/plugins/memberof/memberof.c,v <-- memberof.c new revision: 1.9; previous revision: 1.8 done RCS file: /cvs/dirsec/ldapserver/ldap/servers/plugins/memberof/memberof.h,v done Checking in ldap/servers/plugins/memberof/memberof.h; /cvs/dirsec/ldapserver/ldap/servers/plugins/memberof/memberof.h,v <-- memberof.h initial revision: 1.1 done RCS file: /cvs/dirsec/ldapserver/ldap/servers/plugins/memberof/memberof_config.c,v done Checking in ldap/servers/plugins/memberof/memberof_config.c; /cvs/dirsec/ldapserver/ldap/servers/plugins/memberof/memberof_config.c,v <-- memberof_config.c initial revision: 1.1 done
fix verified - DS 8.1 RHEL 4 Default configuration: # MemberOf Plugin, plugins, config dn: cn=MemberOf Plugin,cn=plugins,cn=config objectClass: top objectClass: nsSlapdPlugin objectClass: extensibleObject cn: MemberOf Plugin nsslapd-pluginPath: libmemberof-plugin nsslapd-pluginInitfunc: memberof_postop_init nsslapd-pluginType: postoperation nsslapd-pluginEnabled: on nsslapd-plugin-depends-on-type: database memberofgroupattr: member memberofattr: memberOf nsslapd-pluginId: memberof nsslapd-pluginVersion: 8.1.0 nsslapd-pluginVendor: Red Hat, Inc. nsslapd-pluginDescription: memberof plugin after live change: # MemberOf Plugin, plugins, config dn: cn=MemberOf Plugin,cn=plugins,cn=config objectClass: top objectClass: nsSlapdPlugin objectClass: extensibleObject cn: MemberOf Plugin nsslapd-pluginPath: libmemberof-plugin nsslapd-pluginInitfunc: memberof_postop_init nsslapd-pluginType: postoperation nsslapd-pluginEnabled: on nsslapd-plugin-depends-on-type: database memberofgroupattr: uniquemember memberofattr: belongTo nsslapd-pluginId: memberof nsslapd-pluginVersion: 8.1.0 nsslapd-pluginVendor: Red Hat, Inc. nsslapd-pluginDescription: memberof plugin
An advisory has been issued which should help the problem described in this bug report. This report is therefore being closed with a resolution of ERRATA. For more information on therefore solution and/or where to find the updated files, please follow the link below. You may reopen this bug report if the solution does not work for you. http://rhn.redhat.com/errata/RHEA-2009-0455.html