From 81b997480956b2b6fa3a5d0e8d6abf5113a06400 Mon Sep 17 00:00:00 2001 From: Noriko Hosoi Date: Mar 25 2013 22:55:37 +0000 Subject: 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;