Resolves: 474945

Summary: Consistently deal with attr syntax info struct ref count when fetcvhing and returning them to the global hashtables.

8 files changed, 60 insertions, 34 deletions

diff --git a/ldap/servers/slapd/attr.c b/ldap/servers/slapd/attr.c
index 6054df0f..6c3a6a0f 100644
--- a/ldap/servers/slapd/attr.c
+++ b/ldap/servers/slapd/attr.c
@@ -226,11 +226,11 @@ slapi_attr_new()
 Slapi_Attr *
 slapi_attr_init(Slapi_Attr *a, const char *type)
 {
-	return slapi_attr_init_locking_optional(a, type, PR_TRUE, PR_TRUE);
+	return slapi_attr_init_locking_optional(a, type, PR_TRUE);
 }
 
 Slapi_Attr *
-slapi_attr_init_locking_optional(Slapi_Attr *a, const char *type, PRBool use_lock, PRBool ref_count)
+slapi_attr_init_locking_optional(Slapi_Attr *a, const char *type, PRBool use_lock)
 {
 	PR_ASSERT(a!=NULL);
 
@@ -249,7 +249,7 @@ slapi_attr_init_locking_optional(Slapi_Attr *a, const char *type, PRBool use_loc
 			{
 				basetype = tmp;	/* basetype was malloc'd */
 			}
-			asi = attr_syntax_get_by_name_locking_optional(basetype, use_lock, ref_count);
+			asi = attr_syntax_get_by_name_locking_optional(basetype, use_lock);
 		}
 		if(NULL == asi)
 		{
@@ -260,7 +260,7 @@ slapi_attr_init_locking_optional(Slapi_Attr *a, const char *type, PRBool use_loc
 			 * attribute type that has that syntax.
 			 */
 			asi = attr_syntax_get_by_name_locking_optional(
-					ATTR_WITH_DIRSTRING_SYNTAX, use_lock, ref_count);
+					ATTR_WITH_DIRSTRING_SYNTAX, use_lock);
 		}
 		else
 		{
diff --git a/ldap/servers/slapd/attrlist.c b/ldap/servers/slapd/attrlist.c
index 6946c0cc..b200472b 100644
--- a/ldap/servers/slapd/attrlist.c
+++ b/ldap/servers/slapd/attrlist.c
@@ -63,11 +63,11 @@ attrlist_free(Slapi_Attr *alist)
 int
 attrlist_find_or_create(Slapi_Attr **alist, const char *type, Slapi_Attr ***a)
 {
-	return attrlist_find_or_create_locking_optional(alist, type, a, PR_TRUE, PR_TRUE);
+	return attrlist_find_or_create_locking_optional(alist, type, a, PR_TRUE);
 }
 
 int
-attrlist_find_or_create_locking_optional(Slapi_Attr **alist, const char *type, Slapi_Attr ***a, PRBool use_lock, PRBool ref_count)
+attrlist_find_or_create_locking_optional(Slapi_Attr **alist, const char *type, Slapi_Attr ***a, PRBool use_lock)
 {
 	int rc= 0; /* found */
 	if ( *a==NULL )
@@ -82,7 +82,7 @@ attrlist_find_or_create_locking_optional(Slapi_Attr **alist, const char *type, S
 	if( **a==NULL )
 	{
 		**a = slapi_attr_new();
-		slapi_attr_init_locking_optional(**a, type, use_lock, ref_count);
+		slapi_attr_init_locking_optional(**a, type, use_lock);
 		rc= 1; /* created */
 	}
 	return rc;
diff --git a/ldap/servers/slapd/attrsyntax.c b/ldap/servers/slapd/attrsyntax.c
index a65d0a99..32ad4105 100644
--- a/ldap/servers/slapd/attrsyntax.c
+++ b/ldap/servers/slapd/attrsyntax.c
@@ -78,7 +78,7 @@ static void *attr_syntax_get_plugin_by_name_with_default( const char *type );
 static void attr_syntax_delete_no_lock( struct asyntaxinfo *asip,
 		PRBool remove_from_oid_table );
 static struct asyntaxinfo *attr_syntax_get_by_oid_locking_optional( const
-		char *oid, PRBool use_lock, PRBool ref_count);
+		char *oid, PRBool use_lock);
 
 #ifdef ATTR_LDAP_DEBUG
 static void attr_syntax_print();
@@ -236,12 +236,20 @@ hashNocaseCompare(const void *v1, const void *v2)
 struct asyntaxinfo *
 attr_syntax_get_by_oid(const char *oid)
 {
-	return attr_syntax_get_by_oid_locking_optional( oid, PR_TRUE, PR_TRUE);
+	return attr_syntax_get_by_oid_locking_optional( oid, PR_TRUE);
 }
 
 
+/*
+ * A version of attr_syntax_get_by_oid() that allows you to bypass using
+ * a lock to access the global oid hash table.
+ *
+ * Note: once the caller is finished using it, the structure must be
+ * returned by calling attr_syntax_return_locking_optional() with the
+ * same use_lock parameter.
+ */
 static struct asyntaxinfo *
-attr_syntax_get_by_oid_locking_optional( const char *oid, PRBool use_lock, PRBool ref_count )
+attr_syntax_get_by_oid_locking_optional( const char *oid, PRBool use_lock )
 {
 	struct asyntaxinfo *asi = 0;
 	if (oid2asi)
@@ -250,7 +258,7 @@ attr_syntax_get_by_oid_locking_optional( const char *oid, PRBool use_lock, PRBoo
 		asi = (struct asyntaxinfo *)PL_HashTableLookup_const(oid2asi, oid);
 		if (asi)
 		{
-			if(ref_count) PR_AtomicIncrement( &asi->asi_refcnt );
+			PR_AtomicIncrement( &asi->asi_refcnt );
 		}
 		if ( use_lock ) AS_UNLOCK_READ(oid2asi_lock);
 	}
@@ -290,12 +298,20 @@ attr_syntax_add_by_oid(const char *oid, struct asyntaxinfo *a, int lock)
 struct asyntaxinfo *
 attr_syntax_get_by_name(const char *name)
 {
-	return attr_syntax_get_by_name_locking_optional(name, PR_TRUE, PR_TRUE);
+	return attr_syntax_get_by_name_locking_optional(name, PR_TRUE);
 }
 
 
+/*
+ * A version of attr_syntax_get_by_name() that allows you to bypass using
+ * a lock around the global name hashtable.
+ *
+ * Note: once the caller is finished using it, the structure must be
+ * returned by calling attr_syntax_return_locking_optional() with the
+ * same use_lock parameter.
+ */
 struct asyntaxinfo *
-attr_syntax_get_by_name_locking_optional(const char *name, PRBool use_lock, PRBool ref_count)
+attr_syntax_get_by_name_locking_optional(const char *name, PRBool use_lock)
 {
 	struct asyntaxinfo *asi = 0;
 	if (name2asi)
@@ -303,12 +319,12 @@ attr_syntax_get_by_name_locking_optional(const char *name, PRBool use_lock, PRBo
 		if ( use_lock ) AS_LOCK_READ(name2asi_lock);
 		asi = (struct asyntaxinfo *)PL_HashTableLookup_const(name2asi, name);
 		if ( NULL != asi ) {
-			if(ref_count) PR_AtomicIncrement( &asi->asi_refcnt );
+			PR_AtomicIncrement( &asi->asi_refcnt );
 		}
 		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, ref_count);
+		asi = attr_syntax_get_by_oid_locking_optional(name, use_lock);
 
 	return asi;
 }
@@ -343,6 +359,8 @@ attr_syntax_return_locking_optional( struct asyntaxinfo *asi, PRBool use_lock )
 				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);
 				}
 				AS_UNLOCK_WRITE(name2asi_lock);
@@ -427,6 +445,10 @@ attr_syntax_delete_no_lock( struct asyntaxinfo *asi,
 		if ( asi->asi_refcnt > 0 ) {
 			asi->asi_marked_for_delete = PR_TRUE;
 		} else {
+			/* This is ok, but the correct thing is to call delete first, 
+			 * then to call return.  The last return will then take care of
+			 * the free.  The only way this free would happen here is if
+			 * you return the syntax before calling delete. */
 			attr_syntax_free(asi);
 		}
 	}
@@ -450,7 +472,7 @@ slapi_attr_syntax_normalize( const char *s )
 	char *r;
 	
 
-    if((asi=attr_syntax_get_by_name_locking_optional(s, PR_TRUE, PR_FALSE)) != NULL ) {
+    if((asi=attr_syntax_get_by_name(s)) != NULL ) {
 		r = slapi_ch_strdup(asi->asi_name);
 		attr_syntax_return( asi );
 	}
@@ -480,7 +502,7 @@ attr_syntax_exists(const char *attr_name)
 	return 0;
 }
 
-/* check syntax without incrementing refcount -- handles locking itself */
+/* check syntax */
 
 static void *
 attr_syntax_get_plugin_by_name_with_default( const char *type )
@@ -491,14 +513,13 @@ attr_syntax_get_plugin_by_name_with_default( const char *type )
 	/*
 	 * first we look for this attribute type explictly
 	 */
-	if ( (asi = attr_syntax_get_by_name_locking_optional(type, PR_TRUE, PR_FALSE)) == NULL ) {
+	if ( (asi = attr_syntax_get_by_name(type)) == NULL ) {
 		/*
 		 * no syntax for this type... return DirectoryString
 		 * syntax.  we accomplish this by looking up a well known
 		 * attribute type that has that syntax.
 		 */
-		asi = attr_syntax_get_by_name_locking_optional(
-				ATTR_WITH_DIRSTRING_SYNTAX, PR_TRUE, PR_FALSE);
+		asi = attr_syntax_get_by_name(ATTR_WITH_DIRSTRING_SYNTAX);
 	}
 	if ( NULL != asi ) {
 		plugin = asi->asi_plugin;
@@ -548,7 +569,7 @@ attr_syntax_add( struct asyntaxinfo *asip )
 
 	/* make sure the oid is unique */
 	if ( NULL != ( oldas_from_oid = attr_syntax_get_by_oid_locking_optional(
-					asip->asi_oid, !nolock, PR_TRUE))) {
+					asip->asi_oid, !nolock))) {
 		if ( 0 == (asip->asi_flags & SLAPI_ATTR_FLAG_OVERRIDE)) {
 			/* failure - OID is in use; no override flag */
 			rc = LDAP_TYPE_OR_VALUE_EXISTS;
@@ -560,13 +581,15 @@ attr_syntax_add( struct asyntaxinfo *asip )
      * the primary name and OID point to the same schema definition.
 	 */
 	if ( NULL != ( oldas_from_name = attr_syntax_get_by_name_locking_optional(
-					asip->asi_name, !nolock, PR_TRUE))) {
+					asip->asi_name, !nolock))) {
 		if ( 0 == (asip->asi_flags & SLAPI_ATTR_FLAG_OVERRIDE)
 					|| ( oldas_from_oid != oldas_from_name )) {
 			/* failure; no override flag OR OID and name don't match */
 			rc = LDAP_TYPE_OR_VALUE_EXISTS;
 			goto cleanup_and_return;
 		}
+		/* Flag for deletion.  We are going to override this attr */
+		attr_syntax_delete(oldas_from_name);
 	} else if ( NULL != oldas_from_oid ) {
 		/* failure - OID is in use but name does not exist */
 		rc = LDAP_TYPE_OR_VALUE_EXISTS;
@@ -580,15 +603,17 @@ attr_syntax_add( struct asyntaxinfo *asip )
 
 			if ( NULL != ( tmpasi =
 							attr_syntax_get_by_name_locking_optional(
-							asip->asi_aliases[i], !nolock,PR_TRUE))) {
+							asip->asi_aliases[i], !nolock))) {
 				if (asip->asi_flags & SLAPI_ATTR_FLAG_OVERRIDE) {
+					/* Flag for tmpasi for deletion.  It will be free'd
+					 * when attr_syntax_return is called. */
 					attr_syntax_delete(tmpasi);
 				} else {
 					/* failure - one of the aliases is already in use */
 					rc = LDAP_TYPE_OR_VALUE_EXISTS;
 				}
 
-				attr_syntax_return( tmpasi );
+				attr_syntax_return_locking_optional( tmpasi, !nolock );
 				if ( LDAP_SUCCESS != rc ) {
 					goto cleanup_and_return;
 				}
@@ -605,8 +630,8 @@ attr_syntax_add( struct asyntaxinfo *asip )
 	attr_syntax_add_by_name( asip, !nolock);
 
 cleanup_and_return:
-	attr_syntax_return( oldas_from_oid );
-	attr_syntax_return( oldas_from_name );
+	attr_syntax_return_locking_optional( oldas_from_oid, !nolock );
+	attr_syntax_return_locking_optional( oldas_from_name, !nolock );
 	return rc;
 }
 
diff --git a/ldap/servers/slapd/entry.c b/ldap/servers/slapd/entry.c
index 01d45b26..72b3e340 100644
--- a/ldap/servers/slapd/entry.c
+++ b/ldap/servers/slapd/entry.c
@@ -304,7 +304,7 @@ str2entry_fast( char *s, int flags, int read_stateinfo )
 				switch(attr_state)
 				{
 				case ATTRIBUTE_PRESENT:
-					if(attrlist_find_or_create_locking_optional(&e->e_attrs, type, &a, PR_FALSE, PR_TRUE)==0 /* Found */)
+					if(attrlist_find_or_create_locking_optional(&e->e_attrs, type, &a, PR_FALSE)==0 /* Found */)
 					{
 						LDAPDebug (LDAP_DEBUG_ANY, "str2entry_fast: Error. Non-contiguous attribute values for %s\n", type, 0, 0);
 						PR_ASSERT(0);
@@ -312,7 +312,7 @@ str2entry_fast( char *s, int flags, int read_stateinfo )
 					}
 					break;
 				case ATTRIBUTE_DELETED:
-					if(attrlist_find_or_create_locking_optional(&e->e_deleted_attrs, type, &a, PR_FALSE, PR_TRUE)==0 /* Found */)
+					if(attrlist_find_or_create_locking_optional(&e->e_deleted_attrs, type, &a, PR_FALSE)==0 /* Found */)
 					{
 						LDAPDebug (LDAP_DEBUG_ANY, "str2entry_fast: Error. Non-contiguous deleted attribute values for %s\n", type, 0, 0);
 						PR_ASSERT(0);
@@ -940,7 +940,7 @@ str2entry_dupcheck( char *s, int flags, int read_stateinfo )
 			{
 				int maxvals = 0;
 				Slapi_Attr **a= NULL;
-				attrlist_find_or_create_locking_optional(alist, sa->sa_type, &a, PR_FALSE, PR_TRUE);
+				attrlist_find_or_create_locking_optional(alist, sa->sa_type, &a, PR_FALSE);
 				valuearray_add_valuearray_fast( /* JCM should be calling a valueset function */
 					&(*a)->a_present_values.va, /* JCM .va is private */
 					sa->sa_present_values.va,
diff --git a/ldap/servers/slapd/proto-slap.h b/ldap/servers/slapd/proto-slap.h
index e088f7d1..4a8a2e08 100644
--- a/ldap/servers/slapd/proto-slap.h
+++ b/ldap/servers/slapd/proto-slap.h
@@ -76,7 +76,7 @@ int attr_check_minmax ( const char *attr_name, char *value, long minval, long ma
 
 void attrlist_free(Slapi_Attr *alist);
 int attrlist_find_or_create(Slapi_Attr **alist, const char *type, Slapi_Attr ***a);
-int attrlist_find_or_create_locking_optional(Slapi_Attr **alist, const char *type, Slapi_Attr ***a, PRBool use_lock, PRBool ref_count);
+int attrlist_find_or_create_locking_optional(Slapi_Attr **alist, const char *type, Slapi_Attr ***a, PRBool use_lock);
 void attrlist_merge( Slapi_Attr **alist, const char *type, struct berval **vals );
 void attrlist_merge_valuearray( Slapi_Attr **alist, const char *type, Slapi_Value **vals );
 int attrlist_delete( Slapi_Attr **attrs, const char *type );
@@ -110,7 +110,7 @@ void attr_syntax_all_clear_flag( unsigned long flag );
 void attr_syntax_delete_all_not_flagged( unsigned long flag );
 struct asyntaxinfo *attr_syntax_get_by_oid ( const char *oid );
 struct asyntaxinfo *attr_syntax_get_by_name ( const char *name );
-struct asyntaxinfo *attr_syntax_get_by_name_locking_optional ( const char *name, PRBool use_lock, PRBool ref_count );
+struct asyntaxinfo *attr_syntax_get_by_name_locking_optional ( const char *name, PRBool use_lock );
 /*
  * Call attr_syntax_return() when you are done using a value returned
  * by attr_syntax_get_by_oid() or attr_syntax_get_by_name().
diff --git a/ldap/servers/slapd/pw.c b/ldap/servers/slapd/pw.c
index 29230f95..2cea6afd 100644
--- a/ldap/servers/slapd/pw.c
+++ b/ldap/servers/slapd/pw.c
@@ -160,7 +160,6 @@ slapi_pw_find_sv(
 /* Checks if the specified value is encoded.
    Returns 1 if it is and 0 otherwise 
  */
-/* NGK - Use this for checking if the password is hashed */
 int slapi_is_encoded (char *value)
 {
 	struct pw_scheme *is_hashed = NULL;
diff --git a/ldap/servers/slapd/schema.c b/ldap/servers/slapd/schema.c
index 5383006d..04b13d09 100644
--- a/ldap/servers/slapd/schema.c
+++ b/ldap/servers/slapd/schema.c
@@ -2443,6 +2443,7 @@ schema_replace_attributes ( Slapi_PBlock *pb, LDAPMod *mod, char *errorbuf,
 				LDAPDebug( LDAP_DEBUG_TRACE, "schema_replace_attributes:"
 						" replacing type %s (OID %s)\n",
 						newasip->asi_name, newasip->asi_oid, 0 );
+				/* flag for deletion */
 				attr_syntax_delete( oldasip );
 			}
 
@@ -3149,7 +3150,8 @@ slapi_check_at_sup_dependency(char *sup, char *oid)
 
 /*
  * if asipp is NULL, the attribute type is added to the global set of schema.
- * if asipp is not NULL, the AT is not added but *asipp is set.
+ * if asipp is not NULL, the AT is not added but *asipp is set.  When you are
+ * finished with *asipp, use attr_syntax_free() to dispose of it.
  * 
  *    schema_flags: Any or none of the following bits could be set
  *        DSE_SCHEMA_NO_CHECK -- schema won't be checked
diff --git a/ldap/servers/slapd/slapi-private.h b/ldap/servers/slapd/slapi-private.h
index cbd7d32e..8ff5caef 100644
--- a/ldap/servers/slapd/slapi-private.h
+++ b/ldap/servers/slapd/slapi-private.h
@@ -344,7 +344,7 @@ void entry_add_rdn_csn(Slapi_Entry *e, const CSN *csn);
 int entry_add_dncsn_ext(Slapi_Entry *entry, const CSN *csn, PRUint32 flags);
 
 /* attr.c */
-Slapi_Attr *slapi_attr_init_locking_optional(Slapi_Attr *a, const char *type, PRBool use_lock, PRBool ref_count);
+Slapi_Attr *slapi_attr_init_locking_optional(Slapi_Attr *a, const char *type, PRBool use_lock);
 int attr_set_csn( Slapi_Attr *a, const CSN *csn);
 int attr_set_deletion_csn( Slapi_Attr *a, const CSN *csn);
 const CSN *attr_get_deletion_csn(const Slapi_Attr *a);