Bug 470918 - replica_set_updatedn does not handle modifications of type ldap_add correctly
Summary: replica_set_updatedn does not handle modifications of type ldap_add correctly
Alias: None
Product: 389
Classification: Retired
Component: Replication - General
Version: 1.2.0
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Nathan Kinder
QA Contact: Chandrasekar Kannan
Depends On:
Blocks: 249650 FDS1.1.4 FDS1.2.0
TreeView+ depends on / blocked
Reported: 2008-11-10 20:52 UTC by Ade Lee
Modified: 2015-01-04 23:34 UTC (History)
4 users (show)

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

Attachments (Terms of Use)
CVS Diffs (1.25 KB, patch)
2008-11-13 17:38 UTC, Nathan Kinder
no flags Details | Diff
Revised Diffs (32.37 KB, patch)
2008-11-13 20:35 UTC, Nathan Kinder
no flags Details | Diff
Re-revised Diffs (32.38 KB, patch)
2008-11-13 23:06 UTC, Nathan Kinder
no flags Details | Diff

Description Ade Lee 2008-11-10 20:52:24 UTC
Description of problem:
Whenever the bind DN for a replica entry for a database is modified, a callback function is called: replica_set_updatedn is called.  This function is supposed to call code that will update the bindDN values in a hash so that they are available to the server to authorize replication requests.

If we attempt to add a second or subsequent bindDN, the hash is not updated correctly.  This is because conditional in the if-statement is incorrect.

replica_set_updatedn (Replica *r, const Slapi_ValueSet *vs, int mod_op)
        PR_ASSERT (r);


        if (!r->updatedn_list)
                r->updatedn_list = replica_updatedn_list_new(NULL);

        if (mod_op & LDAP_MOD_DELETE || vs == NULL ||
                (0 == slapi_valueset_count(vs))) /* null value also causes list deletion */
                replica_updatedn_list_delete(r->updatedn_list, vs);
        else if (mod_op & LDAP_MOD_REPLACE)
                replica_updatedn_list_replace(r->updatedn_list, vs);
        else if (mod_op & LDAP_MOD_ADD)
                replica_updatedn_list_add(r->updatedn_list, vs);


Notice, the last conditional: 
else if (mod_op & LDAP_MOD_ADD)  -- but LDAP_MOD_ADD is 0x0, so mod_op & 0x0 will always be 0 - which is false.  Hence, this code will never be called.

Elsewhere in the code, we use: 
    if ((mod_op & ~LDAP_MOD_BVALUES) == LDAP_MOD_ADD)

which is altogether much better.

Version-Release number of selected component (if applicable):

How reproducible:

Steps to Reproduce:
Actual results:
The net effect of this is that replication fails because the bind user is not added to the replica entry hash, even though the user appears in the replica entry.

Expected results:

Additional info:

Comment 1 Nathan Kinder 2008-11-13 17:38:33 UTC
Created attachment 323481 [details]
CVS Diffs

The fix is to check the value of the op type instead of checking if a specific bit is set.

Comment 2 Rich Megginson 2008-11-13 17:53:52 UTC
Ok.  Looks like a candidate for a convenience macro . . .

Comment 3 Nathan Kinder 2008-11-13 20:35:03 UTC
Created attachment 323496 [details]
Revised Diffs

Here is a revision to the previous patch which defines and uses convenience macros (as suggested by Rich) for checking the modify type.  I converted other areas of the server to use these macros as well.

Comment 4 Rich Megginson 2008-11-13 22:34:34 UTC
Looks good.

One minor nit - the macro should protect x in case it is the result of an expression or evaluation other than ->:


Comment 5 Nathan Kinder 2008-11-13 23:06:24 UTC
Created attachment 323508 [details]
Re-revised Diffs

These diffs implement Rich's suggestion of protecting "x" in the new macros.  The diffs are the same as the previous attachment aside from this change.

Comment 6 Nathan Kinder 2008-11-13 23:10:00 UTC
Checked diffs from comment#5 into ldapserver (HEAD).  Thanks to Rich for his review!

Checking in ldap/servers/plugins/acl/acl.c;
/cvs/dirsec/ldapserver/ldap/servers/plugins/acl/acl.c,v  <--  acl.c
new revision: 1.13; previous revision: 1.12
Checking in ldap/servers/plugins/chainingdb/cb_config.c;
/cvs/dirsec/ldapserver/ldap/servers/plugins/chainingdb/cb_config.c,v  <--  cb_config.c
new revision: 1.8; previous revision: 1.7
Checking in ldap/servers/plugins/chainingdb/cb_instance.c;
/cvs/dirsec/ldapserver/ldap/servers/plugins/chainingdb/cb_instance.c,v  <--  cb_instance.c
new revision: 1.12; previous revision: 1.11
Checking in ldap/servers/plugins/replication/repl5_replica.c;
/cvs/dirsec/ldapserver/ldap/servers/plugins/replication/repl5_replica.c,v  <--  repl5_replica.c
new revision: 1.20; previous revision: 1.19
Checking in ldap/servers/plugins/uiduniq/7bit.c;
/cvs/dirsec/ldapserver/ldap/servers/plugins/uiduniq/7bit.c,v  <--  7bit.c
new revision: 1.8; previous revision: 1.7
Checking in ldap/servers/plugins/uiduniq/uid.c;
/cvs/dirsec/ldapserver/ldap/servers/plugins/uiduniq/uid.c,v  <--  uid.c
new revision: 1.9; previous revision: 1.8
Checking in ldap/servers/slapd/mapping_tree.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/mapping_tree.c,v  <--  mapping_tree.c
new revision: 1.16; previous revision: 1.15
Checking in ldap/servers/slapd/modify.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/modify.c,v  <--  modify.c
new revision: 1.19; previous revision: 1.18
Checking in ldap/servers/slapd/schema.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/schema.c,v  <--  schema.c
new revision: 1.17; previous revision: 1.16
Checking in ldap/servers/slapd/slapi-plugin.h;
/cvs/dirsec/ldapserver/ldap/servers/slapd/slapi-plugin.h,v  <--  slapi-plugin.h
new revision: 1.35; previous revision: 1.34
Checking in ldap/servers/slapd/back-ldbm/ldbm_attrcrypt_config.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/back-ldbm/ldbm_attrcrypt_config.c,v  <--  ldbm_attrcrypt_config.c
new revision: 1.7; previous revision: 1.6
Checking in ldap/servers/slapd/back-ldbm/ldbm_config.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/back-ldbm/ldbm_config.c,v  <--  ldbm_config.c
new revision: 1.17; previous revision: 1.16
Checking in ldap/servers/slapd/back-ldbm/ldbm_index_config.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/back-ldbm/ldbm_index_config.c,v  <--  ldbm_index_config.c
new revision: 1.9; previous revision: 1.8
Checking in ldap/servers/slapd/back-ldbm/ldbm_modrdn.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c,v  <--  ldbm_modrdn.c
new revision: 1.10; previous revision: 1.9

Comment 7 Jenny Severance 2009-03-20 19:51:13 UTC
Can you confirm that if the replication bindDN is modified twice and replication does not fail, this bug is verified?  Thanks

Comment 8 Nathan Kinder 2009-03-23 15:41:31 UTC
To verify this, you will need to add a second replication bind DN and attempt to use it for replication.  Here is how I would go about verification:

- Install 3 instances (a, b, and c).
- Setup replication between a and b (both as masters is fine).
- On master b, add a new replica bind DN value to the replica entry.  Use 
  ldapmodify to do this, and be sure to use "add" a new value instead of
  using "replace".
- Setup replication between b and c.  Make master c use the new replica bind DN
  to connect to master b.

If changes made to master c replicate to b, then you can mark this as verified.

Comment 9 Jenny Severance 2009-04-15 19:02:50 UTC
fix verified - DS 8.1

master1: Solaris 9
master2: RHEL 4
master3: RHEL 5

master2 - assigned 2 replication DNs

master1 connection established with master2 dn1
master3 connection established with master2 dn2

user added to master3 - replication to master2 and subsequently to master1

Comment 10 Chandrasekar Kannan 2009-04-29 23:07:44 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.