Bug 470918 - replica_set_updatedn does not handle modifications of type ldap_add correctly
replica_set_updatedn does not handle modifications of type ldap_add correctly
Status: CLOSED CURRENTRELEASE
Product: 389
Classification: Community
Component: Replication - General (Show other bugs)
1.2.0
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nathan Kinder
Chandrasekar Kannan
:
Depends On:
Blocks: 249650 FDS1.1.4 FDS1.2.0
  Show dependency treegraph
 
Reported: 2008-11-10 15:52 EST by Ade Lee
Modified: 2015-01-04 18:34 EST (History)
4 users (show)

See Also:
Fixed In Version: 8.1
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-29 19:07:44 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


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

  None (edit)
Description Ade Lee 2008-11-10 15:52:24 EST
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);

        PR_Lock(r->repl_lock);

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

        PR_Unlock(r->repl_lock);
}

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:
1.
2.
3.
  
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 12:38:33 EST
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 12:53:52 EST
Ok.  Looks like a candidate for a convenience macro . . .
Comment 3 Nathan Kinder 2008-11-13 15:35:03 EST
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 17:34:34 EST
Looks good.

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

#define SLAPI_IS_MOD_ADD(x) (((x) & ~LDAP_MOD_BVALUES) == LDAP_MOD_ADD)
Comment 5 Nathan Kinder 2008-11-13 18:06:24 EST
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 18:10:00 EST
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
done
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
done
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
done
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
done
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
done
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
done
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
done
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
done
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
done
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
done
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
done
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
done
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
done
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
done
Comment 7 Jenny Galipeau 2009-03-20 15:51:13 EDT
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 11:41:31 EDT
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 Galipeau 2009-04-15 15:02:50 EDT
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 19:07:44 EDT
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.

http://rhn.redhat.com/errata/RHEA-2009-0455.html

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