From 55e8358edd9c53cf96f7745e3cf241fd72b11b77 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Wed, 16 Apr 2014 15:21:21 -0400 Subject: [PATCH 202/225] Ticket 47782 - Parent numbordinate count can be incorrectly updated if an error occurs Bug Description: When adding and deleting entries there is a small chance that the modified parent entry(numbsubordinates) could be replaced in the entry cache, even if the operation fails. Fix Description: For deletes we can simply move the cache entry switching to a safe location, but for adds we need to unswitch the parent's entry in the cache. https://fedorahosted.org/389/ticket/47782 Reviewed by: nhosoi(Thanks!) (cherry picked from commit ac8ada32de4bcc3dfd0a3e2958cefd91f98c4170) --- ldap/servers/slapd/back-ldbm/ldbm_add.c | 13 +++++++++ ldap/servers/slapd/back-ldbm/ldbm_delete.c | 12 ++++---- ldap/servers/slapd/back-ldbm/ldbm_modify.c | 38 ++++++++++++++++++++++++++ ldap/servers/slapd/back-ldbm/proto-back-ldbm.h | 1 + 4 files changed, 58 insertions(+), 6 deletions(-) diff --git a/ldap/servers/slapd/back-ldbm/ldbm_add.c b/ldap/servers/slapd/back-ldbm/ldbm_add.c index 554822c..0ade23c 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_add.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_add.c @@ -117,6 +117,7 @@ ldbm_back_add( Slapi_PBlock *pb ) int is_ruv = 0; /* True if the current entry is RUV */ CSN *opcsn = NULL; entry_address addr = {0}; + int parent_switched = 0; slapi_pblock_get( pb, SLAPI_PLUGIN_PRIVATE, &li ); slapi_pblock_get( pb, SLAPI_ADD_ENTRY, &e ); @@ -974,6 +975,7 @@ ldbm_back_add( Slapi_PBlock *pb ) { /* switch the parent entry copy into play */ modify_switch_entries( &parent_modify_c,be); + parent_switched = 1; } if (ruv_c_init) { @@ -1046,6 +1048,17 @@ error_return: dblayer_remember_disk_filled(li); ldbm_nasty("Add",80,rc); disk_full = 1; + } else if (0 == rc) { + rc = SLAPI_FAIL_GENERAL; + } + + if (parent_switched){ + /* + * Restore the old parent entry, switch the new with the original. + * Otherwise the numsubordinate count will be off, and could later + * be written to disk. + */ + modify_unswitch_entries( &parent_modify_c,be); } diskfull_return: diff --git a/ldap/servers/slapd/back-ldbm/ldbm_delete.c b/ldap/servers/slapd/back-ldbm/ldbm_delete.c index 7c036df..c80d75a 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_delete.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_delete.c @@ -1095,12 +1095,6 @@ ldbm_back_delete( Slapi_PBlock *pb ) CACHE_RETURN(&inst->inst_cache, &e); e = NULL; } - - if (parent_found) - { - /* Replace the old parent entry with the newly modified one */ - modify_switch_entries( &parent_modify_c,be); - } if (ruv_c_init) { if (modify_switch_entries(&ruv_c, be) != 0 ) { @@ -1112,6 +1106,12 @@ ldbm_back_delete( Slapi_PBlock *pb ) } } + if (parent_found) + { + /* Replace the old parent entry with the newly modified one */ + modify_switch_entries( &parent_modify_c,be); + } + rc= 0; goto common_return; diff --git a/ldap/servers/slapd/back-ldbm/ldbm_modify.c b/ldap/servers/slapd/back-ldbm/ldbm_modify.c index b8b6114..be47fcc 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_modify.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_modify.c @@ -122,6 +122,44 @@ int modify_switch_entries(modify_context *mc,backend *be) return ret; } +/* + * Switch the new with the old(original) - undoing modify_switch_entries() + * This expects modify_term() to be called next, as the old "new" entry + * is now gone(replaced by the original entry). + */ +int +modify_unswitch_entries(modify_context *mc,backend *be) +{ + struct backentry *tmp_be; + ldbm_instance *inst = (ldbm_instance *) be->be_instance_info; + int ret = 0; + + if (mc->old_entry!=NULL && mc->new_entry!=NULL) { + /* switch the entries, and reset the new, new, entry */ + tmp_be = mc->new_entry; + mc->new_entry = mc->old_entry; + mc->new_entry->ep_state = 0; + mc->new_entry->ep_refcnt = 0; + mc->new_entry_in_cache = 0; + mc->old_entry = tmp_be; + + ret = cache_replace(&(inst->inst_cache), mc->old_entry, mc->new_entry); + if (ret == 0) { + /* + * The new entry was originally locked, so since we did the + * switch we need to unlock the "new" entry, and return the + * "old" one. modify_term() will then return the "new" entry. + */ + cache_unlock_entry(&inst->inst_cache, mc->new_entry); + CACHE_RETURN( &(inst->inst_cache), &(mc->old_entry) ); + mc->new_entry_in_cache = 1; + mc->old_entry = NULL; + } + } + + return ret; +} + /* This routine does that part of a modify operation which involves updating the on-disk data: updates idices, id2entry. Copes properly with DB_LOCK_DEADLOCK. The caller must be able to cope with diff --git a/ldap/servers/slapd/back-ldbm/proto-back-ldbm.h b/ldap/servers/slapd/back-ldbm/proto-back-ldbm.h index ee30c45..02b261b 100644 --- a/ldap/servers/slapd/back-ldbm/proto-back-ldbm.h +++ b/ldap/servers/slapd/back-ldbm/proto-back-ldbm.h @@ -350,6 +350,7 @@ void modify_init(modify_context *mc,struct backentry *old_entry); int modify_apply_mods(modify_context *mc, Slapi_Mods *smods); int modify_term(modify_context *mc,backend *be); int modify_switch_entries(modify_context *mc,backend *be); +int modify_unswitch_entries(modify_context *mc,backend *be); int modify_apply_mods_ignore_error(modify_context *mc, Slapi_Mods *smods, int error); /* -- 1.8.1.4