Bug 474945 - Adding duplicate schema over LDAP triggers assertion in attrsyntax
Summary: Adding duplicate schema over LDAP triggers assertion in attrsyntax
Alias: None
Product: 389
Classification: Retired
Component: Directory Server
Version: 1.1.3
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Nathan Kinder
QA Contact: Chandrasekar Kannan
: 480262 (view as bug list)
Depends On:
Blocks: 249650 FDS1.2.0
TreeView+ depends on / blocked
Reported: 2008-12-05 23:55 UTC by Rich Megginson
Modified: 2015-01-04 23:35 UTC (History)
3 users (show)

Fixed In Version: 8.1
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2009-04-29 23:08:36 UTC

Attachments (Terms of Use)
CVS Diffs (954 bytes, patch)
2009-01-19 19:09 UTC, Nathan Kinder
no flags Details | Diff
Additional CVS Diffs (21.13 KB, patch)
2009-01-20 20:11 UTC, Nathan Kinder
no flags Details | Diff
Revised CVS Diffs (21.55 KB, patch)
2009-01-20 23:32 UTC, Nathan Kinder
no flags Details | Diff

Description Rich Megginson 2008-12-05 23:55:03 UTC
With the directory server built with DEBUG to enable PR_ASSERT, the following assertion is triggered:
attr_syntax_free( struct asyntaxinfo *a )
	PR_ASSERT( a->asi_refcnt == 0 );

I triggered this by adding the sudo schema via ldapmodify - it was already present in the schema directory, so I was adding a duplicate.  The refcnt was -2.

Comment 1 Nathan Kinder 2009-01-19 19:09:58 UTC
Created attachment 329387 [details]
CVS Diffs

We were calling attr_syntax_delete() on a syntax that we fetched using attr_syntax_get_by_name_locking_optional().

You are supposed to use attr_syntax_return() when you are finished with a syntax that you fetch in this way, which will take care of freeing it if it is required.  There was already code that was doing the proper thing, so the call to attr_syntax_delete() can simply be removed.

Comment 2 Nathan Kinder 2009-01-19 19:44:18 UTC
Checked into ldapserver (HEAD).  Thanks to Rich for his review!

Checking in ldap/servers/slapd/attrsyntax.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/attrsyntax.c,v  <--  attrsyntax.c
new revision: 1.9; previous revision: 1.8

Comment 3 Nathan Kinder 2009-01-20 20:09:59 UTC
There are other areas in the code where we trigger this same assertion.  There look to be some inconsistencies in the usage of the attribute syntax API that are triggering these assertions.

When we look up an attribute syntax from the global oid and name hashtables, we increment a reference count for the returned struct.  When the caller is finished, they are supposed to release the struct by calling attr_syntax_return().

The problem that we have is that there are locking optional versions of the lookup functions that allow the caller to skip incrementing the reference count.  Despite this, there is no associated way of releasing the returned syntax structs that bypasses decrementing the reference count.

The solution is to not allow the increment of the reference count to be skipped.  Whenever a syntax struct is fetched from the global hashtable, we should increment the reference count to prevent it from being free'd by another thread while we are still using it.  We should always release the syntax struct with the appropriate return function that decrements the reference count.  We should just refactor the functions that allow one to skip the reference count increment to take this ability away.

Comment 4 Nathan Kinder 2009-01-20 20:11:59 UTC
Created attachment 329503 [details]
Additional CVS Diffs

This patch refactors a handful of functions to remove a parameter that allows the reference count increment to be skipped.  In addition, I also scanned through all of the server code to ensure that anytime we lookup a syntax from the global hashtables, we use the proper return function to release it.

Comment 5 Nathan Kinder 2009-01-20 23:32:27 UTC
Created attachment 329518 [details]
Revised CVS Diffs

This modifies the last patch by reverting the first patch attached to this bug.  As described in the description of the last patch, the reference count was not being handled correctly, which was resulting in the value being negative.  This was the real cause of the original assertion, not the fact that we were flagging the struct for deletion.  We indeed want to flag the struct for deletion in the code from the first patch, since we are overriding the definition with a new one.

Comment 6 Nathan Kinder 2009-01-21 00:01:03 UTC
Checked into ldapserver (HEAD).  Thanks to Rich for his reviews!

Checking in ldap/servers/slapd/attr.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/attr.c,v  <--  attr.c
new revision: 1.11; previous revision: 1.10
Checking in ldap/servers/slapd/attrlist.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/attrlist.c,v  <--  attrlist.c
new revision: 1.8; previous revision: 1.7
Checking in ldap/servers/slapd/attrsyntax.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/attrsyntax.c,v  <--  attrsyntax.c
new revision: 1.10; previous revision: 1.9
Checking in ldap/servers/slapd/entry.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/entry.c,v  <--  entry.c
new revision: 1.21; previous revision: 1.20
Checking in ldap/servers/slapd/proto-slap.h;
/cvs/dirsec/ldapserver/ldap/servers/slapd/proto-slap.h,v  <--  proto-slap.h
new revision: 1.45; previous revision: 1.44
Checking in ldap/servers/slapd/pw.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/pw.c,v  <--  pw.c
new revision: 1.22; previous revision: 1.21
Checking in ldap/servers/slapd/schema.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/schema.c,v  <--  schema.c
new revision: 1.20; previous revision: 1.19
Checking in ldap/servers/slapd/slapi-private.h;
/cvs/dirsec/ldapserver/ldap/servers/slapd/slapi-private.h,v  <--  slapi-private.h
new revision: 1.34; previous revision: 1.33

Comment 7 Nathan Kinder 2009-01-26 21:53:29 UTC
*** Bug 480262 has been marked as a duplicate of this bug. ***

Comment 8 Jenny Severance 2009-03-19 13:16:59 UTC
Can you please add steps to verify - or would successful scal_02 run be sufficient in verifying?  Thanks

Comment 9 Nathan Kinder 2009-03-19 15:19:53 UTC
Thia can't be verified with optimized bits, as assertions only happen with debug builds.  I think it's safe to close this.

Comment 10 Michael Gregg 2009-04-13 21:44:05 UTC
Scal_02 was run with optimized bits, and with debug bits without problems.

A report of a successful run is here:

Comment 11 Chandrasekar Kannan 2009-04-29 23:08:36 UTC
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.


Note You need to log in before you can comment on or make changes to this bug.