From 64cd7d3346b26ea840bfd328b11e1faae6922116 Mon Sep 17 00:00:00 2001
From: Kurt Zeilenga <kurt@openldap.org>
Date: Wed, 30 Dec 1998 03:35:23 +0000
Subject: [PATCH] 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.
---
servers/slapd/acl.c | 4 +-
servers/slapd/back-ldbm/add.c | 80 +++++++++++++++++------------
servers/slapd/back-ldbm/back-ldbm.h | 1 +
servers/slapd/back-ldbm/group.c | 37 +++++++++----
servers/slapd/back-ldbm/init.c | 1 +
servers/slapd/backend.c | 4 +-
servers/slapd/proto-slap.h | 6 ++-
servers/slapd/slap.h | 4 +-
8 files changed, 89 insertions(+), 48 deletions(-)
diff --git a/servers/slapd/acl.c b/servers/slapd/acl.c
index 3afcda9f02..e94bfe3cd1 100644
--- 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 3b3dcee662..801359e27b 100644
--- 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 c85df11367..7bf2861002 100644
--- 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 a889041c25..1b2b2e8581 100644
--- 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 36ebbc963c..1f322e67bc 100644
--- 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 c3b5d33283..445db7bbb1 100644
--- 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 f213ac202e..1bf95deb2d 100644
--- 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 fbce7eba8e..df7ac5dab6 100644
--- 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
};
--
GitLab