Bug 470918
| Summary: | replica_set_updatedn does not handle modifications of type ldap_add correctly | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Retired] 389 | Reporter: | Ade Lee <alee> | ||||||||
| Component: | Replication - General | Assignee: | Nathan Kinder <nkinder> | ||||||||
| Status: | CLOSED CURRENTRELEASE | QA Contact: | Chandrasekar Kannan <ckannan> | ||||||||
| Severity: | medium | Docs Contact: | |||||||||
| Priority: | medium | ||||||||||
| Version: | 1.2.0 | CC: | benl, jgalipea, rmeggins, shaines | ||||||||
| Target Milestone: | --- | ||||||||||
| Target Release: | --- | ||||||||||
| Hardware: | All | ||||||||||
| OS: | Linux | ||||||||||
| Whiteboard: | |||||||||||
| Fixed In Version: | 8.1 | Doc Type: | Bug Fix | ||||||||
| Doc Text: | Story Points: | --- | |||||||||
| Clone Of: | Environment: | ||||||||||
| Last Closed: | 2009-04-29 23:07:44 UTC | Type: | --- | ||||||||
| Regression: | --- | Mount Type: | --- | ||||||||
| Documentation: | --- | CRM: | |||||||||
| Verified Versions: | Category: | --- | |||||||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||||||
| Embargoed: | |||||||||||
| Bug Depends On: | |||||||||||
| Bug Blocks: | 249650, 467277, 493682 | ||||||||||
| Attachments: |
|
||||||||||
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.
Ok. Looks like a candidate for a convenience macro . . . 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.
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) 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.
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 Can you confirm that if the replication bindDN is modified twice and replication does not fail, this bug is verified? Thanks 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. 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 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 |
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: