ldap/servers/plugins/dna/dna.c | 45 +++------------
ldap/servers/slapd/attrsyntax.c | 116 +++++++++++++++++++++++++++++-----------
ldap/servers/slapd/proto-slap.h | 3 +
ldap/servers/slapd/schema.c | 48 +++++++++-------
4 files changed, 128 insertions(+), 84 deletions(-)
New commits:
commit 81b997480956b2b6fa3a5d0e8d6abf5113a06400
Author: Noriko Hosoi <nhosoi(a)redhat.com>
Date: Mon Mar 25 14:37:18 2013 -0700
Ticket #627 - ns-slapd crashes sporadically with segmentation fault in libslapd.so
Bug Description: Schema reload task (schema-reload.pl) was not
thread safe.
Fix Description: Attribute Syntax is stored in the hash and
retrieved based upon the attribute syntax. When Schema reload
task is invoked, the attribute syntax objects were completely
replaced ignoring the lock protection. This patch protects
the attribute syntax replacement (attr_syntax_delete_all_for_
schemareload) with the write lock. Also, attribute syntax
object maintains the reference count. The schema reload
respects the reference count instead of blindly deleting them.
https://fedorahosted.org/389/ticket/627
Reviewed by Rich (Thank you!!)
diff --git a/ldap/servers/slapd/attrsyntax.c b/ldap/servers/slapd/attrsyntax.c
index 227369c..4326f03 100644
--- a/ldap/servers/slapd/attrsyntax.c
+++ b/ldap/servers/slapd/attrsyntax.c
@@ -97,12 +97,27 @@ attr_syntax_read_lock(void)
}
void
+attr_syntax_write_lock(void)
+{
+ if (0 != attr_syntax_init()) return;
+
+ AS_LOCK_WRITE(oid2asi_lock);
+ AS_LOCK_WRITE(name2asi_lock);
+}
+
+void
attr_syntax_unlock_read(void)
{
- if(name2asi_lock) AS_UNLOCK_READ(name2asi_lock);
- if(oid2asi_lock) AS_UNLOCK_READ(oid2asi_lock);
+ AS_UNLOCK_READ(name2asi_lock);
+ AS_UNLOCK_READ(oid2asi_lock);
}
+void
+attr_syntax_unlock_write(void)
+{
+ AS_UNLOCK_WRITE(name2asi_lock);
+ AS_UNLOCK_WRITE(oid2asi_lock);
+}
#if 0
@@ -233,13 +248,17 @@ attr_syntax_get_by_oid_locking_optional( const char *oid, PRBool
use_lock )
struct asyntaxinfo *asi = 0;
if (oid2asi)
{
- if ( use_lock ) AS_LOCK_READ(oid2asi_lock);
+ if ( use_lock ) {
+ AS_LOCK_READ(oid2asi_lock);
+ }
asi = (struct asyntaxinfo *)PL_HashTableLookup_const(oid2asi, oid);
if (asi)
{
PR_AtomicIncrement( &asi->asi_refcnt );
}
- if ( use_lock ) AS_UNLOCK_READ(oid2asi_lock);
+ if ( use_lock ) {
+ AS_UNLOCK_READ(oid2asi_lock);
+ }
}
return asi;
@@ -257,13 +276,15 @@ attr_syntax_add_by_oid(const char *oid, struct asyntaxinfo *a, int
lock)
{
if (0 != attr_syntax_init()) return;
- if (lock)
+ if (lock) {
AS_LOCK_WRITE(oid2asi_lock);
+ }
PL_HashTableAdd(oid2asi, oid, a);
- if (lock)
+ if (lock) {
AS_UNLOCK_WRITE(oid2asi_lock);
+ }
}
/*
@@ -304,12 +325,16 @@ attr_syntax_get_by_name_locking_optional(const char *name, PRBool
use_lock)
struct asyntaxinfo *asi = 0;
if (name2asi)
{
- if ( use_lock ) AS_LOCK_READ(name2asi_lock);
+ if ( use_lock ) {
+ AS_LOCK_READ(name2asi_lock);
+ }
asi = (struct asyntaxinfo *)PL_HashTableLookup_const(name2asi, name);
if ( NULL != asi ) {
PR_AtomicIncrement( &asi->asi_refcnt );
}
- if ( use_lock ) AS_UNLOCK_READ(name2asi_lock);
+ if ( use_lock ) {
+ AS_UNLOCK_READ(name2asi_lock);
+ }
}
if (!asi) /* given name may be an OID */
asi = attr_syntax_get_by_oid_locking_optional(name, use_lock);
@@ -331,30 +356,38 @@ attr_syntax_return( struct asyntaxinfo *asi )
}
void
-attr_syntax_return_locking_optional( struct asyntaxinfo *asi, PRBool use_lock )
+attr_syntax_return_locking_optional(struct asyntaxinfo *asi, PRBool use_lock)
{
+ int locked = 0;
+ if(use_lock) {
+ AS_LOCK_READ(name2asi_lock);
+ locked = 1;
+ }
if ( NULL != asi ) {
- if ( 0 == PR_AtomicDecrement( &asi->asi_refcnt ))
- {
- PRBool delete_it;
-
- if(use_lock) AS_LOCK_READ(name2asi_lock);
+ PRBool delete_it = PR_FALSE;
+ if ( 0 == PR_AtomicDecrement( &asi->asi_refcnt )) {
delete_it = asi->asi_marked_for_delete;
- if(use_lock) AS_UNLOCK_READ(name2asi_lock);
-
- if ( delete_it )
- {
- AS_LOCK_WRITE(name2asi_lock); /* get a write lock */
- if ( asi->asi_marked_for_delete ) /* one final check */
- {
- /* ref count is 0 and it's flagged for
- * deletion, so it's safe to free now */
- attr_syntax_free(asi);
+ }
+
+ if (delete_it) {
+ if ( asi->asi_marked_for_delete ) { /* one final check */
+ if(use_lock) {
+ AS_UNLOCK_READ(name2asi_lock);
+ AS_LOCK_WRITE(name2asi_lock);
+ }
+ /* ref count is 0 and it's flagged for
+ * deletion, so it's safe to free now */
+ attr_syntax_free(asi);
+ if(use_lock) {
+ AS_UNLOCK_WRITE(name2asi_lock);
+ locked = 0;
}
- AS_UNLOCK_WRITE(name2asi_lock);
}
}
}
+ if(locked) {
+ AS_UNLOCK_READ(name2asi_lock);
+ }
}
/*
@@ -371,8 +404,9 @@ attr_syntax_add_by_name(struct asyntaxinfo *a, int lock)
{
if (0 != attr_syntax_init()) return;
- if (lock)
+ if (lock) {
AS_LOCK_WRITE(name2asi_lock);
+ }
PL_HashTableAdd(name2asi, a->asi_name, a);
if ( a->asi_aliases != NULL ) {
@@ -383,8 +417,9 @@ attr_syntax_add_by_name(struct asyntaxinfo *a, int lock)
}
}
- if (lock)
+ if (lock) {
AS_UNLOCK_WRITE(name2asi_lock);
+ }
}
@@ -990,11 +1025,11 @@ attr_syntax_enumerate_attrs(AttrEnumFunc aef, void *arg, PRBool
writelock )
attr_syntax_enumerate_attrs_ext(oid2asi, aef, arg);
if ( writelock ) {
- AS_UNLOCK_WRITE(oid2asi_lock);
AS_UNLOCK_WRITE(name2asi_lock);
+ AS_UNLOCK_WRITE(oid2asi_lock);
} else {
- AS_UNLOCK_READ(oid2asi_lock);
AS_UNLOCK_READ(name2asi_lock);
+ AS_UNLOCK_READ(oid2asi_lock);
}
}
@@ -1092,6 +1127,21 @@ attr_syntax_delete_all()
(void *)&fi, PR_TRUE );
}
+/*
+ * Delete all attribute definitions without attr_syntax lock.
+ * The caller is responsible for the lock.
+ */
+void
+attr_syntax_delete_all_for_schemareload(unsigned long flag)
+{
+ struct attr_syntax_enum_flaginfo fi;
+
+ memset(&fi, 0, sizeof(fi));
+ fi.asef_flag = flag;
+ attr_syntax_enumerate_attrs_ext(oid2asi, attr_syntax_delete_if_not_flagged,
+ (void *)&fi);
+}
+
static int
attr_syntax_init(void)
{
@@ -1215,13 +1265,19 @@ static int
attr_syntax_internal_asi_add(struct asyntaxinfo *asip, void *arg)
{
struct asyntaxinfo *asip_copy;
+ int rc = 0;
+
if (!asip) {
return 1;
}
/* Copy is needed since when reloading the schema,
* existing syntax info is cleaned up. */
asip_copy = attr_syntax_dup(asip);
- return attr_syntax_add(asip_copy);
+ rc = attr_syntax_add(asip_copy);
+ if (LDAP_SUCCESS != rc) {
+ attr_syntax_free(asip_copy);
+ }
+ return rc;
}
/* Reload internal attribute syntax stashed in the internalasi hashtable. */
diff --git a/ldap/servers/slapd/proto-slap.h b/ldap/servers/slapd/proto-slap.h
index 4077daa..a34136f 100644
--- a/ldap/servers/slapd/proto-slap.h
+++ b/ldap/servers/slapd/proto-slap.h
@@ -114,7 +114,9 @@ int attrlist_replace_with_flags(Slapi_Attr **alist, const char *type,
struct ber
* attrsyntax.c
*/
void attr_syntax_read_lock(void);
+void attr_syntax_write_lock(void);
void attr_syntax_unlock_read(void);
+void attr_syntax_unlock_write(void);
int attr_syntax_exists (const char *attr_name);
void attr_syntax_delete ( struct asyntaxinfo *asip );
#define SLAPI_SYNTAXLENGTH_NONE (-1) /* for syntaxlength parameter */
@@ -142,6 +144,7 @@ struct asyntaxinfo *attr_syntax_get_by_name_locking_optional ( const
char *name,
void attr_syntax_return( struct asyntaxinfo *asi );
void attr_syntax_return_locking_optional( struct asyntaxinfo *asi, PRBool use_lock );
void attr_syntax_delete_all(void);
+void attr_syntax_delete_all_for_schemareload(unsigned long flag);
/*
* value.c
diff --git a/ldap/servers/slapd/schema.c b/ldap/servers/slapd/schema.c
index 5eb6520..dc56534 100644
--- a/ldap/servers/slapd/schema.c
+++ b/ldap/servers/slapd/schema.c
@@ -1393,7 +1393,7 @@ slapi_schema_list_attribute_names(unsigned long flag)
aew.flag=flag;
attr_syntax_enumerate_attrs(schema_list_attributes_callback, &aew,
- PR_FALSE);
+ PR_FALSE);
return aew.attrs;
}
@@ -2405,8 +2405,9 @@ static int
schema_replace_attributes ( Slapi_PBlock *pb, LDAPMod *mod, char *errorbuf,
size_t errorbufsize )
{
- int i, rc = LDAP_SUCCESS;
- struct asyntaxinfo *newasip, *oldasip;
+ int i, rc = LDAP_SUCCESS;
+ struct asyntaxinfo *newasip, *oldasip;
+ PRUint32 schema_flags = 0;
if ( NULL == mod->mod_bvalues ) {
schema_create_errormsg( errorbuf, errorbufsize, schema_errprefix_at,
@@ -2414,8 +2415,11 @@ schema_replace_attributes ( Slapi_PBlock *pb, LDAPMod *mod, char
*errorbuf,
return LDAP_UNWILLING_TO_PERFORM;
}
- /* clear all of the "keep" flags */
- attr_syntax_all_clear_flag( SLAPI_ATTR_FLAG_KEEP );
+ slapi_pblock_get(pb, SLAPI_SCHEMA_FLAGS, &schema_flags);
+ if (!(schema_flags & (DSE_SCHEMA_NO_LOAD|DSE_SCHEMA_NO_CHECK))) {
+ /* clear all of the "keep" flags unless it's from schema-reload */
+ attr_syntax_all_clear_flag( SLAPI_ATTR_FLAG_KEEP );
+ }
for ( i = 0; mod->mod_bvalues[i] != NULL; ++i ) {
if ( LDAP_SUCCESS != ( rc = read_at_ldif( mod->mod_bvalues[i]->bv_val,
@@ -2473,12 +2477,14 @@ schema_replace_attributes ( Slapi_PBlock *pb, LDAPMod *mod, char
*errorbuf,
* XXXmcs: we should consider reporting an error if any read only types
* remain....
*/
- attr_syntax_delete_all_not_flagged( SLAPI_ATTR_FLAG_KEEP
- | SLAPI_ATTR_FLAG_STD_ATTR );
+ attr_syntax_delete_all_not_flagged( SLAPI_ATTR_FLAG_KEEP |
+ SLAPI_ATTR_FLAG_STD_ATTR );
clean_up_and_return:
- /* clear all of the "keep" flags */
- attr_syntax_all_clear_flag( SLAPI_ATTR_FLAG_KEEP );
+ if (!(schema_flags & (DSE_SCHEMA_NO_LOAD|DSE_SCHEMA_NO_CHECK))) {
+ /* clear all of the "keep" flags unless it's from schema-reload */
+ attr_syntax_all_clear_flag( SLAPI_ATTR_FLAG_KEEP );
+ }
return rc;
}
@@ -3894,14 +3900,12 @@ load_schema_dse(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Entry
*ignored,
int primary_file = 0; /* this is the primary (writeable) schema file */
int schema_ds4x_compat = config_get_ds4_compatible_schema();
PRUint32 flags = *(PRUint32 *)arg;
- flags |= DSE_SCHEMA_NO_GLOCK; /* don't lock global resources
- during initialization */
*returncode = 0;
/*
* Note: there is no need to call schema_lock_write() here because this
- * function is only called during server startup.
+ * function is only called during server startup.
*/
slapi_pblock_get( pb, SLAPI_DSE_IS_PRIMARY_FILE, &primary_file );
@@ -3943,6 +3947,8 @@ load_schema_dse(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Entry
*ignored,
if (*returncode)
return SLAPI_DSE_CALLBACK_ERROR;
+ flags |= DSE_SCHEMA_NO_GLOCK; /* don't lock global resources
+ during initialization */
if (!slapi_entry_attr_find(e, "objectclasses", &attr) && attr)
{
/* enumerate the values in attr */
@@ -4013,7 +4019,6 @@ load_schema_dse(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Entry
*ignored,
* DSE_SCHEMA_NO_CHECK -- schema won't be checked
* DSE_SCHEMA_NO_BACKEND -- don't add as backend
* DSE_SCHEMA_LOCKED -- already locked; no further lock needed
-
*/
static int
init_schema_dse_ext(char *schemadir, Slapi_Backend *be,
@@ -4119,7 +4124,7 @@ init_schema_dse_ext(char *schemadir, Slapi_Backend *be,
"DESC 'Standard schema for LDAP' SYNTAX "
"1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'RFC 2252' )",
NULL, errorbuf, SLAPI_DSE_RETURNTEXT_SIZE,
- DSE_SCHEMA_NO_GLOCK|schema_flags, 0, 0, 0);
+ schema_flags, 0, 0, 0);
}
if (rc)
{
@@ -4192,7 +4197,7 @@ init_schema_dse(const char *configdir)
{
schemadir = slapi_ch_smprintf("%s/%s", configdir, SCHEMA_SUBDIR_NAME);
}
- rc = init_schema_dse_ext(schemadir, NULL, &pschemadse, 0);
+ rc = init_schema_dse_ext(schemadir, NULL, &pschemadse, DSE_SCHEMA_NO_GLOCK);
slapi_ch_free_string(&schemadir);
return rc;
}
@@ -4856,14 +4861,14 @@ slapi_validate_schema_files(char *schemadir)
{
struct dse *my_pschemadse = NULL;
int rc = init_schema_dse_ext(schemadir, NULL, &my_pschemadse,
- DSE_SCHEMA_NO_LOAD | DSE_SCHEMA_NO_BACKEND);
+ DSE_SCHEMA_NO_LOAD | DSE_SCHEMA_NO_BACKEND);
dse_destroy(my_pschemadse); /* my_pschemadse was created just to
- validate the schema */
+ validate the schema */
if (rc) {
return LDAP_SUCCESS;
} else {
slapi_log_error( SLAPI_LOG_FATAL, "schema_reload",
- "schema file validation failed\n" );
+ "schema file validation failed\n" );
return LDAP_OBJECT_CLASS_VIOLATION;
}
}
@@ -4889,10 +4894,13 @@ slapi_reload_schema_files(char *schemadir)
}
slapi_be_Wlock(be); /* be lock must be outer of schemafile lock */
reload_schemafile_lock();
- attr_syntax_delete_all();
+ /* Exclude attr_syntax not to grab from the hash table while cleaning up */
+ attr_syntax_write_lock();
+ attr_syntax_delete_all_for_schemareload(SLAPI_ATTR_FLAG_KEEP);
oc_delete_all_nolock();
+ attr_syntax_unlock_write();
rc = init_schema_dse_ext(schemadir, be, &my_pschemadse,
- DSE_SCHEMA_NO_CHECK | DSE_SCHEMA_LOCKED);
+ DSE_SCHEMA_NO_CHECK | DSE_SCHEMA_LOCKED);
if (rc) {
dse_destroy(pschemadse);
pschemadse = my_pschemadse;
commit 712c59690a9caf120a631da1fb8c886efd422229
Author: Noriko Hosoi <nhosoi(a)redhat.com>
Date: Thu Mar 21 11:18:07 2013 -0700
Ticket #634 - Deadlock in DNA plug-in
Bug description: Fix for Ticket #576 -
DNA: use event queue for config update only at the start up
(commit 7c11ed17796ba8b17afa5758fcfdf1c937b54ef4)
modified the timing of updating the shared config in the
database. The write operation was no need to be in the DNA
write lock, but it was located in it, which caused the dead-
lock.
Fix description: This patch releases the dna write lock before
updating the shared config in the database.
https://fedorahosted.org/389/ticket/634
Reviewed by Nathan (Thanks!!)
diff --git a/ldap/servers/plugins/dna/dna.c b/ldap/servers/plugins/dna/dna.c
index 080e357..affdadf 100644
--- a/ldap/servers/plugins/dna/dna.c
+++ b/ldap/servers/plugins/dna/dna.c
@@ -666,6 +666,7 @@ dna_load_plugin_config(int use_eventq)
if (LDAP_SUCCESS != result) {
status = DNA_FAILURE;
+ dna_unlock();
goto cleanup;
}
@@ -673,6 +674,7 @@ dna_load_plugin_config(int use_eventq)
&entries);
if (NULL == entries || NULL == entries[0]) {
status = DNA_SUCCESS;
+ dna_unlock();
goto cleanup;
}
@@ -682,6 +684,7 @@ dna_load_plugin_config(int use_eventq)
* looking for valid ones. */
dna_parse_config_entry(entries[i], 1);
}
+ dna_unlock();
if (use_eventq) {
/* Setup an event to update the shared config 30
@@ -692,14 +695,12 @@ dna_load_plugin_config(int use_eventq)
time(&now);
slapi_eq_once(dna_update_config_event, NULL, now + 30);
} else {
- int nolock = 1; /* no need to lock since dna write lock is held. */
- dna_update_config_event(0, &nolock);
+ dna_update_config_event(0, NULL);
}
cleanup:
slapi_free_search_results_internal(search_pb);
slapi_pblock_destroy(search_pb);
- dna_unlock();
slapi_log_error(SLAPI_LOG_TRACE, DNA_PLUGIN_SUBSYSTEM,
"<-- dna_load_plugin_config\n");
@@ -1247,15 +1248,9 @@ dna_update_config_event(time_t event_time, void *arg)
Slapi_PBlock *pb = NULL;
struct configEntry *config_entry = NULL;
PRCList *list = NULL;
- int nolock = 0;
- if (arg) {
- nolock = *(int *)arg;
- }
- if (!nolock) {
- /* Get read lock to prevent config changes */
- dna_read_lock();
- }
+ /* Get read lock to prevent config changes */
+ dna_read_lock();
/* Bail out if the plug-in close function was just called. */
if (!g_plugin_started) {
@@ -1300,9 +1295,7 @@ dna_update_config_event(time_t event_time, void *arg)
}
bail:
- if (!nolock) {
- dna_unlock();
- }
+ dna_unlock();
slapi_pblock_destroy(pb);
}
@@ -2785,7 +2778,6 @@ _dna_pre_op_add(Slapi_PBlock *pb, Slapi_Entry *e, char **errstr)
char **generated_types = NULL;
PRUint64 setval = 0;
int i;
- int issharedconfig = 0;
/* Bail out if the plug-in close function was just called. */
if (!g_plugin_started) {
@@ -2803,12 +2795,7 @@ _dna_pre_op_add(Slapi_PBlock *pb, Slapi_Entry *e, char **errstr)
* We also check if we need to get the next range of values, and grab them.
* We do this here so we don't have to do it in the be_txn_preop.
*/
- /* Has write lock for config entries. */
- issharedconfig = slapi_entry_attr_hasvalue(e, SLAPI_ATTR_OBJECTCLASS,
- DNA_SHAREDCONFIG);
- if (!issharedconfig) {
- dna_read_lock();
- }
+ dna_read_lock();
if (!PR_CLIST_IS_EMPTY(dna_global_config)) {
list = PR_LIST_HEAD(dna_global_config);
@@ -2966,9 +2953,7 @@ next:
}
}
- if (!issharedconfig) {
- dna_unlock();
- }
+ dna_unlock();
slapi_ch_array_free(generated_types);
bail:
@@ -3427,7 +3412,6 @@ static int dna_be_txn_pre_op(Slapi_PBlock *pb, int modtype)
char *type = NULL;
int numvals, e_numvals = 0;
int i, len, ret = 0;
- int issharedconfig = 0;
slapi_log_error(SLAPI_LOG_TRACE, DNA_PLUGIN_SUBSYSTEM,
"--> dna_be_txn_pre_op\n");
@@ -3463,12 +3447,7 @@ static int dna_be_txn_pre_op(Slapi_PBlock *pb, int modtype)
slapi_mods_init_passin(smods, mods);
}
- /* Has write lock for config entries. */
- issharedconfig = slapi_entry_attr_hasvalue(e, SLAPI_ATTR_OBJECTCLASS,
- DNA_SHAREDCONFIG);
- if (!issharedconfig) {
- dna_read_lock();
- }
+ dna_read_lock();
if (!PR_CLIST_IS_EMPTY(dna_global_config)) {
list = PR_LIST_HEAD(dna_global_config);
@@ -3659,9 +3638,7 @@ next:
list = PR_NEXT_LINK(list);
}
}
- if (!issharedconfig) {
- dna_unlock();
- }
+ dna_unlock();
bail:
if (LDAP_CHANGETYPE_MODIFY == modtype) {