Bug 557224 - subtree rename breaks the referential integrity plug-in
Summary: subtree rename breaks the referential integrity plug-in
Status: CLOSED CURRENTRELEASE
Alias: None
Product: 389
Classification: Retired
Component: Server - Plugins
Version: 1.3.0
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Noriko Hosoi
QA Contact: Viktor Ashirov
URL:
Whiteboard:
Keywords:
Depends On:
Blocks: 639035
TreeView+ depends on / blocked
 
Reported: 2010-01-20 19:00 UTC by Noriko Hosoi
Modified: 2015-12-07 16:35 UTC (History)
2 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2015-12-07 16:35:28 UTC


Attachments (Terms of Use)
git patch for ldap/servers/plugins/referint/referint.c (18.67 KB, patch)
2010-01-26 18:45 UTC, Noriko Hosoi
rmeggins: review+
Details | Diff
test data (1.45 MB, text/plain)
2010-01-26 19:03 UTC, Noriko Hosoi
no flags Details
git patch for ldap/servers/plugins/referint/referint.c (30.91 KB, patch)
2010-01-28 00:24 UTC, Noriko Hosoi
nhosoi: review?
rmeggins: review+
Details | Diff

Description Noriko Hosoi 2010-01-20 19:00:20 UTC
Description of problem:
Analysis by andrey.ivanov@polytechnique.fr
> I'm pretty sure the referential integrity plug-in will not work for
> modrdn operations with a new superior. Looking more thoroughly through
> the code ( ldap / servers / plugins / referint / referint.c) confirms
> my suspicion that new rdn superior is not taken into account. The
> function  referint_postop_modrdn extracts from the parameter block
> only SLAPI_MODRDN_TARGET and SLAPI_MODRDN_NEWRDN, it does not extract
> SLAPI_MODRDN_NEWSUPERIOR neither passes it further down the utility
> functions - update_integrity(argv, dn, newrdn, logChanges) and
> writeintegritylog(argv[1],dn, newrdn). The same applies to the delayed
> referint operations (the plug-in writes to the special integrity log
> file only the old DN and the new RDN, but never the new superior :
> writeintegritylog(argv[1],dn, newrdn);)

Another thought on the subject of referential integrity plug-in - in
the previous mail i have only mentioned one-entry renames with a new
superior. The things get even worse when we rename a whole non-empty
sub-tree. It means that the referential integrity plug-in should
change to the new DNs all the references to all the entries of the
whole sub-tree, not only for one entry.

And what if we rename a sub-tree containing both the referenced entry
and the entry referencing the first one's DN in one of its "integrity"
attributes? It actually means that we need at first make the rename
and then all the searches and replacements. Though it seems it's
already the case as it's a post-op(?) plug-in...

Comment 1 Noriko Hosoi 2010-01-26 18:45:53 UTC
Created attachment 386896 [details]
git patch for ldap/servers/plugins/referint/referint.c

Fix Description: The referential integrity plugin has not supported
the subtree rename (modrdn with newsuperior).  This patch is adding
the support.

Comment 2 Noriko Hosoi 2010-01-26 19:03:19 UTC
Created attachment 386898 [details]
test data

How to verify:
1. import the attached data (example1k.ldif) and restart the server
2. rename uid=admin to uid=admin0
   dn: uid=admin, dc=example,dc=com
   changetype: modrdn
   newrdn: uid=admin0
   deleteoldrdn: -1
2-1. check the seeAlso, owner, and member attribute values to verify that uid=admin has been replaced with uid=admin0.
3. move uid=admin0 to ou=admin,dc=example,dc=com
   dn: uid=admin0, dc=example,dc=com
   changetype: modrdn
   newrdn: uid=admin0
   deleteoldrdn: -1
   newsuperior: ou=admin,dc=example,dc=com
3-1. check the seeAlso, owner, and member attribute values to verify that uid=admin0 has moved to ou=admin,dc=example,dc=com.
4. move the rdn and superior to the original
   dn: uid=admin0, ou=admin,dc=example,dc=com
   changetype: modrdn
   newrdn: uid=admin
   deleteoldrdn: -1
   newsuperior: dc=example,dc=com
4-1. check the seeAlso, owner, and member attribute values are replaced with uid=admin,dc=example,dc=com.

Comment 3 Noriko Hosoi 2010-01-28 00:24:55 UTC
Created attachment 387204 [details]
git patch for ldap/servers/plugins/referint/referint.c

Comment from andrey.ivanov@polytechnique.fr:
The patch you provided does take into
account the modRDN with new superior but it does not seem to take care
of subtree renames (smth like rename "ou=users" to "ou=utilisateurs"
with child entries). For subtree renames all the entries in the
renamed subtree change their DNs so  for each  of these renamed
entries we have to make the search
"slapi_search_internal_set_pb(search_result_pb, search_base,
LDAP_SCOPE_SUBTREE, filter, attrs..." with a different filter.
So for each modrdn we should verify if the renamed entry has any child
entries and if it is the case then treat them either directly or
recursively...

Fix Description: The referential integrity plugin has not supported
the subtree rename (modrdn with newsuperior).  This patch is adding
the support.

There are 2 typical cases.
(case 1)
DN that modrdn modifies matches the value of attributes which is the
target of the referential integrity.
E.g.,
modrdn: uid=A,ou=B,o=C --> uid=AA,ou=BB,o=C
then,
member: uid=A,ou=B,ou=C --> uid=AA,ou=BB,ou=C
seeAlso: uid=A,ou=B,ou=C --> uid=AA,ou=BB,ou=C

(case 2)
DN that modrdn modifies is the ancestor of the value of attributes
which is the target of the referential integrity.
E.g.,
modrdn: ou=B,o=C --> ou=BB,o=C
then,
member: uid=A,ou=B,ou=C --> uid=A,ou=BB,ou=C
seeAlso: uid=A,ou=B,ou=C --> uid=A,ou=BB,ou=C

Comment 4 Noriko Hosoi 2010-01-28 17:28:16 UTC
Reviewed by Rich (Thank you!!)

Pushed to master.

$ git merge subtree
Updating d9fdaf2..3fdcbdd
Fast forward
 ldap/servers/plugins/referint/referint.c |  702 +++++++++++++++++++++---------
 1 files changed, 493 insertions(+), 209 deletions(-)
$ git push
Counting objects: 13, done.
Delta compression using 2 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 4.18 KiB, done.
Total 7 (delta 4), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
   d9fdaf2..3fdcbdd  master -> master

Comment 5 Andrey Ivanov 2010-01-28 22:30:37 UTC
Noriko, the patch looks ok. I have only two comments concerning this patch.

1) Documentation impact :
Since the new code makes use of the substring filter (%a=*%e), maybe the documentation should contain a recommendation to make not only presence and equality, but also substring index on referential plug-in attributes. Otherwise the internal search will be unindexed.


2) A question : in _update_one_per_mod() you do the following with the sval string :

p = PL_strstr(sval, norm_origDN);
bak = *p;
*p = '\0';
newvalue = slapi_ch_smprintf("%s%s", sval, newDN);
*p = bak;

...
 slapi_ch_free_string(&sval);

Apparently you want to save the previous value of *p to make a correct slapi_ch_free_string (the string is freed up to '\0'?) Now, in _update_all_per_mod():

p = PL_strstr(sval, norm_origDN);
*p = '\0';
...
slapi_ch_free_string(&sval);

Here you don't save the previous value of *p. Will the  slapi_ch_free_string function free the memory correctly?  If the answer is yes, then  we don't need the "bak" variable in _update_one_per_mod function...

Comment 6 Noriko Hosoi 2010-01-28 22:52:25 UTC
Hi Andrey, thanks a lot for your review!  My comments are inline.

(In reply to comment #5)
> Noriko, the patch looks ok. I have only two comments concerning this patch.
> 
> 1) Documentation impact :
> Since the new code makes use of the substring filter (%a=*%e), maybe the
> documentation should contain a recommendation to make not only presence and
> equality, but also substring index on referential plug-in attributes. Otherwise
> the internal search will be unindexed.

A very good point!  I'm going to open a doc bug.

> 2) A question : in _update_one_per_mod() you do the following with the sval
> string :
> 
> p = PL_strstr(sval, norm_origDN);
> bak = *p;
> *p = '\0';
> newvalue = slapi_ch_smprintf("%s%s", sval, newDN);
> *p = bak;
> 
> ...
>  slapi_ch_free_string(&sval);
> 
> Apparently you want to save the previous value of *p to make a correct
> slapi_ch_free_string (the string is freed up to '\0'?) 

I don't think so...  "free" releases the memory allocated by "malloc" or its friends regardless of the contents.  For instance, if you allocate some amount of memory using calloc, which fills the allocated memory with '\0', but the following "free" correctly frees the memory.

The reason why I recovered the character in _update_one_per_mod is I wanted to print the original "sval" value in the error at line 465.  On the other hand, it's not done in _update_all_per_mod.  Thus, I did not get bothered to put the character back.

> Now, in
> _update_all_per_mod():
> 
> p = PL_strstr(sval, norm_origDN);
> *p = '\0';
> ...
> slapi_ch_free_string(&sval);
> 
> Here you don't save the previous value of *p. Will the  slapi_ch_free_string
> function free the memory correctly?  If the answer is yes, then  we don't need
> the "bak" variable in _update_one_per_mod function...

Comment 7 Amita Sharma 2011-07-27 10:29:39 UTC
We have a test case for this bug in subtree renaming test suit :
/home/amsharma/DS9.0/trunk/testcases/DS/6.0/subtreeRenames

subtree_35

subtreeRenames startup 	100% (1/1) 	  	 
subtreeRenames run 	100% (32/32) 	  	 
subtreeRenames cleanup 	100% (1/1)

Hence marking as VERIFIED.


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