ldap/servers/slapd/attrsyntax.c | 116 +++++++++++++++++++++++++++++-----------
ldap/servers/slapd/proto-slap.h | 3 +
ldap/servers/slapd/schema.c | 48 +++++++++-------
3 files changed, 117 insertions(+), 50 deletions(-)
New commits:
commit a7ae8de6e50ae209737f8927cf3a63e875620911
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!!)
(cherry picked from commit 81b997480956b2b6fa3a5d0e8d6abf5113a06400)
diff --git a/ldap/servers/slapd/attrsyntax.c b/ldap/servers/slapd/attrsyntax.c
index 7ac3b46..bd1a6f1 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);
+ }
}
@@ -948,11 +983,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);
}
}
@@ -1050,6 +1085,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)
{
@@ -1173,13 +1223,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 e947248..e872cec 100644
--- a/ldap/servers/slapd/proto-slap.h
+++ b/ldap/servers/slapd/proto-slap.h
@@ -113,7 +113,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 */
@@ -140,6 +142,7 @@ struct asyntaxinfo *attr_syntax_get_by_name_with_default ( 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;