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!!)

ldap/servers/slapd/attrsyntax.c

@@ -96,13 +96,28 @@ attr_syntax_read_lock(void)
 	AS_LOCK_READ(name2asi_lock);
 }
 
+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. */

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

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;