From e2ee8bba6ff5e0f733142889e0ccaa08cebbdb75 Mon Sep 17 00:00:00 2001

From: Noriko Hosoi <nhosoi@redhat.com>

Date: Mon, 25 Mar 2013 14:37:18 -0700

Subject: [PATCH 40/42] 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)

---

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

diff --git a/ldap/servers/slapd/attrsyntax.c b/ldap/servers/slapd/attrsyntax.c

index ff137a8..2dabf1f 100644

--- a/ldap/servers/slapd/attrsyntax.c

+++ b/ldap/servers/slapd/attrsyntax.c

@@ -96,12 +96,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

@@ -255,13 +270,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;

@@ -279,13 +298,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);

+	}

 }

 /*

@@ -317,12 +338,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);

@@ -344,30 +369,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);

+	}

 }

 /*

@@ -384,8 +417,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 ) {

@@ -396,8 +430,9 @@ attr_syntax_add_by_name(struct asyntaxinfo *a, int lock)

 		}

 	}

-	if (lock)

+	if (lock) {

 		AS_UNLOCK_WRITE(name2asi_lock);

+	}

 }

@@ -943,11 +978,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);

 	}

 }

@@ -1045,6 +1080,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)

 {

@@ -1165,13 +1215,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 bdcef9a..5112471 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 */

@@ -139,6 +141,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 12f11c3..1768c7e 100644

--- a/ldap/servers/slapd/schema.c

+++ b/ldap/servers/slapd/schema.c

@@ -1406,7 +1406,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;

 }

@@ -2409,8 +2409,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,

@@ -2418,8 +2419,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,

@@ -2477,12 +2481,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;

 }

@@ -3898,14 +3904,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 );

@@ -3947,6 +3951,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 */

@@ -4017,7 +4023,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,

@@ -4123,7 +4128,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)

 		{

@@ -4196,7 +4201,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;

 }

@@ -4860,14 +4865,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;

 	}

 }

@@ -4893,10 +4898,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;

-- 

1.8.1.4