Preliminary Fixes for ITS#24, ITS#26, and ldbm_back_add race condition.

Resolved deadlock by passing target entry to be_group and using
this if dn same as bdn.  It might actually be safer to check
entry ids instead of dns.
Resolved bogus add to cache after failed acl check by deferring
cache add until after parent/acl checks have successful been
completed.
Eliminated race condition caused by concurrent adds
of same dn by adding 'li_add_mutex' around the critical section
of code (most of ldbm_back_add).
This code is preliminary and still needs significant testing.

diff --git a/servers/slapd/acl.c b/servers/slapd/acl.c
index 3afcda9f029154dbc21591ff30d708c39f55d6b6..e94bfe3cd1f6ee356ba03b9c1afa8a016780d12c 100644 (file)
--- a/servers/slapd/acl.c
+++ b/servers/slapd/acl.c
@@ -360,7 +360,9 @@ acl_access_allowed(
                        /* see if asker is listed in dnattr */
                        string_expand(buf, sizeof(buf), b->a_group, edn, matches);
 
-                       if (be_group(be, buf, odn, b->a_objectclassvalue, b->a_groupattrname) == 0) {
+                       if (be_group(be, e, buf, odn,
+                               b->a_objectclassvalue, b->a_groupattrname) == 0)
+                       {
                                Debug( LDAP_DEBUG_ACL,
                                        "<= acl_access_allowed: matched by clause #%d (group) access granted\n",
                                        i, 0, 0 );

diff --git a/servers/slapd/back-ldbm/add.c b/servers/slapd/back-ldbm/add.c
index 3b3dcee66258665c51ebc5bb00ae96d46d072e2f..801359e27bf4d78ed57595a62649f618c557ba43 100644 (file)
--- a/servers/slapd/back-ldbm/add.c
+++ b/servers/slapd/back-ldbm/add.c
@@ -28,22 +28,22 @@ ldbm_back_add(
 
        Debug(LDAP_DEBUG_ARGS, "==> ldbm_back_add: %s\n", dn, 0, 0);
 
+       pthread_mutex_lock(&li->li_add_mutex);
+
        if ( ( dn2id( be, dn ) ) != NOID ) {
+               pthread_mutex_unlock(&li->li_add_mutex);
                entry_free( e );
                free( dn );
                send_ldap_result( conn, op, LDAP_ALREADY_EXISTS, "", "" );
                return( -1 );
        }
 
-       /* XXX race condition here til we cache_add_entry_lock below XXX */
-
        if ( global_schemacheck && oc_schema_check( e ) != 0 ) {
+               pthread_mutex_unlock(&li->li_add_mutex);
+
                Debug( LDAP_DEBUG_TRACE, "entry failed schema check\n",
                        0, 0, 0 );
 
-               /* XXX this should be ok, no other thread should have access
-                * because e hasn't been added to the cache yet
-                */
                entry_free( e );
                free( dn );
                send_ldap_result( conn, op, LDAP_OBJECT_CLASS_VIOLATION, "",
@@ -51,28 +51,6 @@ ldbm_back_add(
                return( -1 );
        }
 
-       /*
-        * Try to add the entry to the cache, assign it a new dnid
-        * and mark it locked.  This should only fail if the entry
-        * already exists.
-        */
-
-       e->e_id = next_id( be );
-       if ( cache_add_entry_lock( &li->li_cache, e, ENTRY_STATE_CREATING )
-           != 0 ) {
-               Debug( LDAP_DEBUG_ANY, "cache_add_entry_lock failed\n", 0, 0,
-                   0 );
-               next_id_return( be, e->e_id );
-                
-               /* XXX this should be ok, no other thread should have access
-                * because e hasn't been added to the cache yet
-                */
-               entry_free( e );
-               free( dn );
-               send_ldap_result( conn, op, LDAP_ALREADY_EXISTS, "", "" );
-               return( -1 );
-       }
-
        /*
         * Get the parent dn and see if the corresponding entry exists.
         * If the parent does not exist, only allow the "root" user to
@@ -85,6 +63,7 @@ ldbm_back_add(
 
                /* get entry with reader lock */
                if ( (p = dn2entry_r( be, pdn, &matched )) == NULL ) {
+                       pthread_mutex_unlock(&li->li_add_mutex);
                        Debug( LDAP_DEBUG_TRACE, "parent does not exist\n", 0,
                            0, 0 );
                        send_ldap_result( conn, op, LDAP_NO_SUCH_OBJECT,
@@ -94,32 +73,62 @@ ldbm_back_add(
                                free( matched );
                        }
 
-                       rc = -1;
-                       goto return_results;
+                       entry_free( e );
+                       free( dn );
+                       return -1;
                }
 
                if ( ! access_allowed( be, conn, op, p, "children", NULL,
-                   op->o_dn, ACL_WRITE ) ) {
+                   op->o_dn, ACL_WRITE ) )
+               {
+                       pthread_mutex_unlock(&li->li_add_mutex);
                        Debug( LDAP_DEBUG_TRACE, "no access to parent\n", 0,
                            0, 0 );
                        send_ldap_result( conn, op, LDAP_INSUFFICIENT_ACCESS,
                            "", "" );
 
-                       rc = -1;
-                       goto return_results;
+                       entry_free( e );
+                       free( dn );
+                       return -1;
                }
+
        } else {
                if ( ! be_isroot( be, op->o_dn ) ) {
+                       pthread_mutex_unlock(&li->li_add_mutex);
                        Debug( LDAP_DEBUG_TRACE, "no parent & not root\n", 0,
                            0, 0 );
                        send_ldap_result( conn, op, LDAP_INSUFFICIENT_ACCESS,
                            "", "" );
 
-                       rc = -1;
-                       goto return_results;
+                       entry_free( e );
+                       free( dn );
+                       return -1;
                }
        }
 
+       /*
+        * Try to add the entry to the cache, assign it a new dnid
+        * and mark it locked.  This should only fail if the entry
+        * already exists.
+        */
+
+       e->e_id = next_id( be );
+       if ( cache_add_entry_lock( &li->li_cache, e, ENTRY_STATE_CREATING ) != 0 ) {
+               pthread_mutex_unlock(&li->li_add_mutex);
+
+               Debug( LDAP_DEBUG_ANY, "cache_add_entry_lock failed\n", 0, 0,
+                   0 );
+               next_id_return( be, e->e_id );
+                
+               /* XXX this should be ok, no other thread should have access
+                * because e hasn't been added to the cache yet
+                */
+               entry_free( e );
+               free( dn );
+               send_ldap_result( conn, op, LDAP_ALREADY_EXISTS, "", "" );
+               return( -1 );
+       }
+
        /*
         * add it to the id2children index for the parent
         */
@@ -192,5 +201,8 @@ return_results:;
                cache_return_entry_r( &li->li_cache, p ); 
        }
 
+       /* it might actually be okay to release this lock sooner */
+       pthread_mutex_unlock(&li->li_add_mutex);
+
        return( rc );
 }

diff --git a/servers/slapd/back-ldbm/back-ldbm.h b/servers/slapd/back-ldbm/back-ldbm.h
index c85df113672c2ebec3e6afb71e66c4dff854d61d..7bf2861002a64c4806204c6f88e56a23f11a6ab8 100644 (file)
--- a/servers/slapd/back-ldbm/back-ldbm.h
+++ b/servers/slapd/back-ldbm/back-ldbm.h
@@ -106,6 +106,7 @@ struct attrinfo {
 
 struct ldbminfo {
        ID                      li_nextid;
+       pthread_mutex_t         li_add_mutex;
        pthread_mutex_t         li_nextid_mutex;
        int                     li_mode;
        char                    *li_directory;

diff --git a/servers/slapd/back-ldbm/group.c b/servers/slapd/back-ldbm/group.c
index a889041c25254e242e4c88d328e074dd87604c44..1b2b2e85815b86b8e8287f71e0ed81fadd943d4d 100644 (file)
--- a/servers/slapd/back-ldbm/group.c
+++ b/servers/slapd/back-ldbm/group.c
@@ -20,6 +20,7 @@
 int
 ldbm_back_group(
        Backend     *be,
+       Entry   *target,
         char        *bdn,
         char        *edn,
         char        *objectclassValue,
@@ -28,6 +29,7 @@ ldbm_back_group(
 {
         struct ldbminfo *li = (struct ldbminfo *) be->be_private;    
         Entry        *e;
+               char            *tdn;
         char        *matched;
         Attribute   *objectClass;
         Attribute   *member;
@@ -38,15 +40,30 @@ ldbm_back_group(
        Debug( LDAP_DEBUG_TRACE, "=> ldbm_back_group: objectClass: %s attrName: %s\n", 
                 objectclassValue, groupattrName, 0 ); 
 
-        /* can we find bdn entry with reader lock */
-        if ((e = dn2entry_r(be, bdn, &matched )) == NULL) {
-                Debug( LDAP_DEBUG_TRACE, "=> ldbm_back_group: cannot find bdn: %s matched: %s\n", bdn, (matched ? matched : ""), 0 ); 
-                if (matched != NULL)
-                        free(matched);
-                return( 1 );
+       tdn = dn_normalize_case( ch_strdup( target->e_dn ) );
+       if (strcmp(tdn, bdn) == 0) {
+               /* we already have a LOCKED copy of the entry */
+               e = target;
+               Debug( LDAP_DEBUG_ARGS,
+                       "=> ldbm_back_group: target is bdn: %s\n",
+                       bdn, 0, 0 ); 
+       } else {
+               /* can we find bdn entry with reader lock */
+               if ((e = dn2entry_r(be, bdn, &matched )) == NULL) {
+                       Debug( LDAP_DEBUG_TRACE,
+                               "=> ldbm_back_group: cannot find bdn: %s matched: %s\n",
+                                       bdn, (matched ? matched : ""), 0 ); 
+                       if (matched != NULL)
+                               free(matched);
+                       free(tdn);
+                       return( 1 );
+               }
+               Debug( LDAP_DEBUG_ARGS,
+                       "=> ldbm_back_group: found bdn: %s\n",
+                       bdn, 0, 0 ); 
         }
+       free(tdn);
 
-        Debug( LDAP_DEBUG_ARGS, "=> ldbm_back_group: found bdn: %s\n", bdn, 0, 0 ); 
 
         /* check for deleted */
 
@@ -90,8 +107,10 @@ ldbm_back_group(
             }
         }
 
-        /* free entry and reader lock */
-        cache_return_entry_r( &li->li_cache, e );                 
+       if( target != e ) {
+               /* free entry and reader lock */
+               cache_return_entry_r( &li->li_cache, e );                 
+       }
         Debug( LDAP_DEBUG_ARGS, "ldbm_back_group: rc: %d\n", rc, 0, 0 ); 
         return(rc);
 }

diff --git a/servers/slapd/back-ldbm/init.c b/servers/slapd/back-ldbm/init.c
index 36ebbc963c6cdfcc5d3e9b41523c5c36ba221f9b..1f322e67bc64010fa45b26c6d8eca958ebdd3c88 100644 (file)
--- a/servers/slapd/back-ldbm/init.c
+++ b/servers/slapd/back-ldbm/init.c
@@ -63,6 +63,7 @@ ldbm_back_init(
        free( argv[ 1 ] );
 
        /* initialize various mutex locks & condition variables */
+       pthread_mutex_init( &li->li_add_mutex, pthread_mutexattr_default );
        pthread_mutex_init( &li->li_cache.c_mutex, pthread_mutexattr_default );
        pthread_mutex_init( &li->li_nextid_mutex, pthread_mutexattr_default );
        pthread_mutex_init( &li->li_dbcache_mutex, pthread_mutexattr_default );

diff --git a/servers/slapd/backend.c b/servers/slapd/backend.c
index c3b5d332832ab6f5931d9a4fca374f5e05644028..445db7bbb1592c83b0b53241a9110301263936d1 100644 (file)
--- a/servers/slapd/backend.c
+++ b/servers/slapd/backend.c
@@ -261,6 +261,7 @@ be_unbind(
 int 
 be_group(
        Backend *be,
+       Entry   *e,
        char    *bdn,
        char    *edn,
        char    *objectclassValue,
@@ -268,7 +269,8 @@ be_group(
 )
 {
         if (be->be_group)
-                return(be->be_group(be, bdn, edn, objectclassValue, groupattrName));
+                return(be->be_group(be, e, bdn, edn,
+                                       objectclassValue, groupattrName));
         else
                 return(1);
 }

diff --git a/servers/slapd/proto-slap.h b/servers/slapd/proto-slap.h
index f213ac202ebd121d5459c4d5c66575d862fdbc9f..1bf95deb2d7597373dba2e2601750a7f152757b0 100644 (file)
--- a/servers/slapd/proto-slap.h
+++ b/servers/slapd/proto-slap.h
@@ -256,7 +256,8 @@ extern struct acl   *global_acl;
 extern struct objclass *global_oc;
 extern time_t          currenttime;
 
-extern int     be_group LDAP_P((Backend *be, char *bdn, char *edn, char *objectclassValue, char *groupattrName));
+extern int     be_group LDAP_P((Backend *be, Entry *e,
+       char *bdn, char *edn, char *objectclassValue, char *groupattrName));
 extern void    init LDAP_P((void));
 extern void    be_unbind LDAP_P((Connection *conn, Operation *op));
 extern void    config_info LDAP_P((Connection *conn, Operation *op));
@@ -295,7 +296,8 @@ extern void ldbm_back_abandon LDAP_P((Backend *be, Connection *c, Operation *o,
 extern void ldbm_back_config LDAP_P((Backend *be, char *fname, int lineno, int argc, char **argv ));
 extern void ldbm_back_init   LDAP_P((Backend *be));
 extern void ldbm_back_close  LDAP_P((Backend *be));
-extern int  ldbm_back_group  LDAP_P((Backend *be, char *bdn, char *edn, char *objectclassValue, char *groupattrName ));
+extern int  ldbm_back_group  LDAP_P((Backend *be, Entry *target,
+       char *bdn, char *edn, char *objectclassValue, char *groupattrName ));
 #endif
 
 #ifdef SLAPD_PASSWD

diff --git a/servers/slapd/slap.h b/servers/slapd/slap.h
index fbce7eba8e81e8626cd26a62e11cb77c6e6dda7b..df7ac5dab61d2af2ba3815a2f9ebd27b4982c6dd 100644 (file)
--- a/servers/slapd/slap.h
+++ b/servers/slapd/slap.h
@@ -249,7 +249,9 @@ struct backend {
        void    (*be_close)  LDAP_P((Backend *be));
 
 #ifdef SLAPD_ACLGROUPS
-       int     (*be_group)  LDAP_P((Backend *be, char *bdn, char *edn, char *objectclassValue, char *groupattrName ));
+       int     (*be_group)  LDAP_P((Backend *be, Entry *e,
+               char *bdn, char *edn,
+               char *objectclassValue, char *groupattrName ));
 #endif
 };