From 9098fc70e0ca0145acfccb928b8749fb6d5497a5 Mon Sep 17 00:00:00 2001 From: Noriko Hosoi Date: Tue, 14 Sep 2010 13:46:42 -0700 Subject: Bug 631862 - crash - delete entries not in cache + referint https://bugzilla.redhat.com/show_bug.cgi?id=631862 Resolves: bug 631862 Bug Description: crash - delete entries not in cache + referint Reviewed by: rmeggins and nhosoi Branch: master Fix Description: When deleting an entry, the referential integrity (referint) plugin does an internal search to find references to this entry (e.g. in group entries) and removes them. The search code wants to ensure that the entrydn attribute is present in the entry when using entryrdn (subtree rename). The search code sets a flag to tell the id2entry code to add the entrydn attribute if it is not present. However, it was doing this to an entry in the cache, which may be in use by another thread. The solution is to add the entrydn attribute before adding the entry to the cache. In the id2entry code, this is after the entry has been read from the id2entry db successfully, but before the entry is added to the cache. In the LDAP ADD code, this is done when the other computed operational attributes are added to the new entry. In addition to the above fix by rmeggins@redhat.com, following changes are made: 1) entrydn attribute is always added to the entry in memory before putting it in the entry cache, and the attribute is removed before writing the entry to the database. 2) eliminating id2entry_ext, which was introduced to pass flags, but it is no longer needed since only a flag ID2ENTRY_ADD_ENTRYDN was removed. Platforms tested: RHEL5 x86_64 Flag Day: no Doc impact: no --- ldap/servers/slapd/back-ldbm/back-ldbm.h | 3 - ldap/servers/slapd/back-ldbm/id2entry.c | 86 ++++++++++++++++---------- ldap/servers/slapd/back-ldbm/ldbm_add.c | 17 ++--- ldap/servers/slapd/back-ldbm/ldbm_search.c | 2 +- ldap/servers/slapd/back-ldbm/proto-back-ldbm.h | 2 - 5 files changed, 60 insertions(+), 50 deletions(-) diff --git a/ldap/servers/slapd/back-ldbm/back-ldbm.h b/ldap/servers/slapd/back-ldbm/back-ldbm.h index 29d18458..980551b9 100644 --- a/ldap/servers/slapd/back-ldbm/back-ldbm.h +++ b/ldap/servers/slapd/back-ldbm/back-ldbm.h @@ -829,9 +829,6 @@ typedef struct _back_search_result_set /* whether we call fat lock or not [608146] */ #define SERIALLOCK(li) (li->li_fat_lock) -/* id2entry_ext flags */ -#define ID2ENTRY_ADD_ENTRYDN 0x1 - /* * 0: SUCCESS * libdb returns negative error codes diff --git a/ldap/servers/slapd/back-ldbm/id2entry.c b/ldap/servers/slapd/back-ldbm/id2entry.c index 198fdd51..b58591e8 100644 --- a/ldap/servers/slapd/back-ldbm/id2entry.c +++ b/ldap/servers/slapd/back-ldbm/id2entry.c @@ -60,6 +60,7 @@ id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt int len, rc; char temp_id[sizeof(ID)]; struct backentry *encrypted_entry = NULL; + char *entrydn = NULL; LDAPDebug( LDAP_DEBUG_TRACE, "=> id2entry_add( %lu, \"%s\" )\n", (u_long)e->ep_id, backentry_get_ndn(e), 0 ); @@ -94,6 +95,7 @@ id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt memset(&data, 0, sizeof(data)); if (entryrdn_get_switch()) { + Slapi_Attr *eattr = NULL; struct backdn *oldbdn = NULL; Slapi_DN *sdn = slapi_sdn_dup(slapi_entry_get_sdn_const(e->ep_entry)); @@ -107,7 +109,7 @@ id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt if (cache_replace( &inst->inst_dncache, oldbdn, bdn ) != 0) { /* The entry was not in the cache for some reason (this * should not happen since CACHE_ADD said it existed above). */ - LDAPDebug( LDAP_DEBUG_ANY, "id2entry_add_ext(): Entry disappeared " + LDAPDebug( LDAP_DEBUG_ANY, "id2entry_add_ext(): Entry disappeared " "from cache (%s)\n", oldbdn->dn_sdn, 0, 0 ); } } @@ -118,6 +120,13 @@ id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt LDAPDebug( LDAP_DEBUG_TRACE, "=> id2entry_add (dncache) ( %lu, \"%s\" )\n", (u_long)e->ep_id, slapi_entry_get_dn_const(e->ep_entry), 0 ); + /* If entrydn exists in the entry, we have to remove it before + * writing the entry to the database. */ + if (0 == slapi_entry_attr_find(e->ep_entry, + LDBM_ENTRYDN_STR, &eattr)) { + /* entrydn exists in the entry. let's removed it. */ + slapi_entry_delete_values(e->ep_entry, LDBM_ENTRYDN_STR, NULL); + } } data.dptr = slapi_entry2str_with_options(entry_to_use, &len, options); data.dsize = len + 1; @@ -145,6 +154,7 @@ id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt const char *myrdn = slapi_entry_get_rdn_const(e->ep_entry); const char *parentdn = NULL; char *myparentdn = NULL; + Slapi_Attr *eattr = NULL; /* If the parent is in the cache, check the parent's DN and * adjust to it if they don't match. (bz628300) */ if (parentid && myrdn) { @@ -167,6 +177,24 @@ id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt CACHE_RETURN(&inst->inst_cache, &parententry); } } + /* + * Adding entrydn attribute value to the entry, + * which should be done before adding the entry to the entry cache. + * Note: since we removed entrydn from the entry before writing + * it to the database, it is guaranteed not in the entry. + */ + /* slapi_ch_strdup and slapi_dn_ignore_case never returns NULL */ + entrydn = slapi_ch_strdup(slapi_entry_get_dn_const(e->ep_entry)); + entrydn = slapi_dn_ignore_case(entrydn); + slapi_entry_attr_set_charptr (e->ep_entry, + LDBM_ENTRYDN_STR, entrydn); + if (0 == slapi_entry_attr_find(e->ep_entry, + LDBM_ENTRYDN_STR, &eattr)) { + /* now entrydn should exist in the entry */ + /* Set it to operational attribute */ + eattr->a_flags = SLAPI_ATTR_FLAG_OPATTR; + } + slapi_ch_free_string(&entrydn); } /* * For ldbm_back_add and ldbm_back_modify, this entry had been already @@ -245,7 +273,7 @@ id2entry_delete( backend *be, struct backentry *e, back_txn *txn ) } struct backentry * -id2entry_ext( backend *be, ID id, back_txn *txn, int *err, int flags ) +id2entry( backend *be, ID id, back_txn *txn, int *err ) { ldbm_instance *inst = (ldbm_instance *) be->be_instance_info; DB *db = NULL; @@ -387,6 +415,30 @@ id2entry_ext( backend *be, ID id, back_txn *txn, int *err, int flags ) "attrcrypt_decrypt_entry failed in id2entry\n"); } + /* + * If return entry exists AND entryrdn switch is on, + * add the entrydn value. + */ + if (entryrdn_get_switch()) { + Slapi_Attr *eattr = NULL; + /* Check if entrydn is in the entry or not */ + if (slapi_entry_attr_find(e->ep_entry, LDBM_ENTRYDN_STR, &eattr)) { + /* entrydn does not exist in the entry */ + char *entrydn = NULL; + /* slapi_ch_strdup and slapi_dn_ignore_case never returns NULL */ + entrydn = slapi_ch_strdup(slapi_entry_get_dn_const(e->ep_entry)); + entrydn = slapi_dn_ignore_case(entrydn); + slapi_entry_attr_set_charptr (e->ep_entry, + LDBM_ENTRYDN_STR, entrydn); + if (0 == slapi_entry_attr_find(e->ep_entry, + LDBM_ENTRYDN_STR, &eattr)) { + /* now entrydn should exist in the entry */ + /* Set it to operational attribute */ + eattr->a_flags = SLAPI_ATTR_FLAG_OPATTR; + } + slapi_ch_free_string(&entrydn); + } + } retval = CACHE_ADD( &inst->inst_cache, e, &imposter ); if (1 == retval) { /* This means that someone else put the entry in the cache @@ -413,29 +465,6 @@ id2entry_ext( backend *be, ID id, back_txn *txn, int *err, int flags ) } bail: - /* - * If return entry exists AND adding entrydn is requested AND - * entryrdn switch is on, add the entrydn value. - */ - if (e && e->ep_entry && (flags & ID2ENTRY_ADD_ENTRYDN) && - entryrdn_get_switch()) { - Slapi_Attr *eattr = NULL; - /* Check if entrydn is in the entry or not */ - if (slapi_entry_attr_find(e->ep_entry, "entrydn", &eattr)) { - /* entrydn does not exist in the entry */ - char *entrydn = NULL; - /* slapi_ch_strdup and slapi_dn_ignore_case never returns NULL */ - entrydn = slapi_ch_strdup(slapi_entry_get_dn_const(e->ep_entry)); - entrydn = slapi_dn_ignore_case(entrydn); - slapi_entry_attr_set_charptr (e->ep_entry, "entrydn", entrydn); - if (0 == slapi_entry_attr_find(e->ep_entry, "entrydn", &eattr)) { - /* now entrydn should exist in the entry */ - /* Set it to operational attribute */ - eattr->a_flags = SLAPI_ATTR_FLAG_OPATTR; - } - slapi_ch_free_string(&entrydn); - } - } slapi_ch_free( &(data.data) ); dblayer_release_id2entry( be, db ); @@ -444,10 +473,3 @@ bail: "<= id2entry( %lu ) %p (disk)\n", (u_long)id, e); return( e ); } - -struct backentry * -id2entry( backend *be, ID id, back_txn *txn, int *err ) -{ - return id2entry_ext(be, id, txn, err, 0); -} - diff --git a/ldap/servers/slapd/back-ldbm/ldbm_add.c b/ldap/servers/slapd/back-ldbm/ldbm_add.c index d2d6197e..c2e86f28 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_add.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_add.c @@ -575,17 +575,12 @@ ldbm_back_add( Slapi_PBlock *pb ) if(is_resurect_operation) { - if (!entryrdn_get_switch()) { /* subtree-rename: off */ - /* add the entrydn operational attributes to the addingentry */ - add_update_entrydn_operational_attributes(addingentry); - } + add_update_entrydn_operational_attributes(addingentry); } else if (is_tombstone_operation) { - if (!entryrdn_get_switch()) { /* subtree-rename: off */ - /* Remove the entrydn operational attributes from the addingentry */ - delete_update_entrydn_operational_attributes(addingentry); - } + /* Remove the entrydn operational attributes from the addingentry */ + delete_update_entrydn_operational_attributes(addingentry); } else { @@ -978,10 +973,8 @@ add_update_entry_operational_attributes(struct backentry *ep, ID pid) bv.bv_len = strlen( buf ); entry_replace_values( ep->ep_entry, "entryid", bvp ); - if (!entryrdn_get_switch()) { /* subtree-rename: off */ - /* add the entrydn operational attribute to the entry. */ - add_update_entrydn_operational_attributes(ep); - } + /* add the entrydn operational attribute to the entry. */ + add_update_entrydn_operational_attributes(ep); } /* diff --git a/ldap/servers/slapd/back-ldbm/ldbm_search.c b/ldap/servers/slapd/back-ldbm/ldbm_search.c index 72537f4d..0f97d861 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_search.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_search.c @@ -1257,7 +1257,7 @@ ldbm_back_next_search_entry_ext( Slapi_PBlock *pb, int use_extension ) ++sr->sr_lookthroughcount; /* checked above */ /* get the entry */ - e = id2entry_ext( be, id, NULL, &err, ID2ENTRY_ADD_ENTRYDN ); + e = id2entry( be, id, NULL, &err ); if ( e == NULL ) { if ( err != 0 && err != DB_NOTFOUND ) diff --git a/ldap/servers/slapd/back-ldbm/proto-back-ldbm.h b/ldap/servers/slapd/back-ldbm/proto-back-ldbm.h index 363bec27..00344c69 100644 --- a/ldap/servers/slapd/back-ldbm/proto-back-ldbm.h +++ b/ldap/servers/slapd/back-ldbm/proto-back-ldbm.h @@ -217,8 +217,6 @@ int id2entry_add( backend *be, struct backentry *e, back_txn *txn ); int id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt ); int id2entry_delete( backend *be, struct backentry *e, back_txn *txn ); struct backentry * id2entry( backend *be, ID id, back_txn *txn, int *err ); -struct backentry * id2entry_ext( backend *be, ID id, back_txn *txn, int *err, int flags ); - /* * idl.c -- cgit