Bug 450989 - memberOf: Make group and memberOf attributes configurable
memberOf: Make group and memberOf attributes configurable
Status: CLOSED CURRENTRELEASE
Product: 389
Classification: Community
Component: Server - memberOf Plug-in (Show other bugs)
1.1.1
All Linux
low Severity low
: ---
: ---
Assigned To: Nathan Kinder
Chandrasekar Kannan
:
Depends On:
Blocks: 249650 FDS112
  Show dependency treegraph
 
Reported: 2008-06-12 00:14 EDT by Nathan Kinder
Modified: 2015-01-04 18:32 EST (History)
5 users (show)

See Also:
Fixed In Version: 8.1
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-29 19:04:30 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
CVS Diffs (21.33 KB, patch)
2008-06-12 00:14 EDT, Nathan Kinder
no flags Details | Diff
New memberof.h header file (3.07 KB, text/plain)
2008-06-12 00:15 EDT, Nathan Kinder
no flags Details
New memberof_config.c source file (8.09 KB, text/plain)
2008-06-12 00:16 EDT, Nathan Kinder
no flags Details
CVS Diffs (62.78 KB, patch)
2008-06-17 19:20 EDT, Nathan Kinder
no flags Details | Diff
New memberof.h header file (3.21 KB, text/plain)
2008-06-17 19:21 EDT, Nathan Kinder
no flags Details
New memberof_config.c source file (9.75 KB, text/plain)
2008-06-17 19:21 EDT, Nathan Kinder
no flags Details
CVS Diffs (63.45 KB, patch)
2008-06-18 12:23 EDT, Nathan Kinder
no flags Details | Diff
New memberof.h header file (3.22 KB, text/plain)
2008-06-18 12:23 EDT, Nathan Kinder
no flags Details
New memberof_config.c source file (10.25 KB, text/plain)
2008-06-18 12:24 EDT, Nathan Kinder
no flags Details
CVS Diffs (65.47 KB, patch)
2008-06-18 19:08 EDT, Nathan Kinder
no flags Details | Diff
New memberof.h header file (3.25 KB, text/plain)
2008-06-18 19:09 EDT, Nathan Kinder
no flags Details
New memberof_config.c source file (10.61 KB, text/plain)
2008-06-18 19:10 EDT, Nathan Kinder
no flags Details

  None (edit)
Description Nathan Kinder 2008-06-12 00:14:54 EDT
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.
Comment 1 Nathan Kinder 2008-06-12 00:14:54 EDT
Created attachment 309023 [details]
CVS Diffs
Comment 2 Nathan Kinder 2008-06-12 00:15:53 EDT
Created attachment 309024 [details]
New memberof.h header file
Comment 3 Nathan Kinder 2008-06-12 00:16:21 EDT
Created attachment 309025 [details]
New memberof_config.c source file
Comment 4 Rich Megginson 2008-06-12 10:18:13 EDT
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?
Comment 5 Nathan Kinder 2008-06-12 11:22:39 EDT
(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!
Comment 6 Nathan Kinder 2008-06-17 19:20:44 EDT
Created attachment 309681 [details]
CVS Diffs
Comment 7 Nathan Kinder 2008-06-17 19:21:30 EDT
Created attachment 309682 [details]
New memberof.h header file
Comment 8 Nathan Kinder 2008-06-17 19:21:46 EDT
Created attachment 309683 [details]
New memberof_config.c source file
Comment 9 Nathan Kinder 2008-06-17 19:28:27 EDT
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.
Comment 10 Nathan Kinder 2008-06-18 12:22:26 EDT
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.
Comment 11 Nathan Kinder 2008-06-18 12:23:25 EDT
Created attachment 309753 [details]
CVS Diffs
Comment 12 Nathan Kinder 2008-06-18 12:23:58 EDT
Created attachment 309754 [details]
New memberof.h header file
Comment 13 Nathan Kinder 2008-06-18 12:24:14 EDT
Created attachment 309755 [details]
New memberof_config.c source file
Comment 14 Nathan Kinder 2008-06-18 19:07:03 EDT
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.
Comment 15 Nathan Kinder 2008-06-18 19:08:46 EDT
Created attachment 309804 [details]
CVS Diffs
Comment 16 Nathan Kinder 2008-06-18 19:09:44 EDT
Created attachment 309805 [details]
New memberof.h header file
Comment 17 Nathan Kinder 2008-06-18 19:10:32 EDT
Created attachment 309806 [details]
New memberof_config.c source file
Comment 18 Noriko Hosoi 2008-06-18 20:16:46 EDT
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.

Comment 19 Nathan Kinder 2008-06-19 11:19:31 EDT
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
Comment 20 Jenny Galipeau 2009-04-09 14:36:57 EDT
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
Comment 21 Chandrasekar Kannan 2009-04-29 19:04:30 EDT
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

Note You need to log in before you can comment on or make changes to this bug.