From 33fbced25277b88695bfba7262e606380e9d891f Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Mar 18 2019 16:42:49 +0000 Subject: Ticket 50260 - Invalid cache flushing improvements Description: The original version of the fix only checked if backend transaction "post" operation plugins failed, but it did not check for errors from the backend transaction "pre" operation plugin. To address this we flush invalid entries whenever any error occurs. We were also not flushing invalid cache entries when modrdn errors occurred. Modrdns only make changes to the DN hashtable inside the entry cache, but we were only checking the ID hashtable. So we also need to check the DN hashtable in the entry cache for invalid entries. https://pagure.io/389-ds-base/issue/50260 Reviewed by: firstyear & tbordaz(Thanks!!) --- diff --git a/dirsrvtests/tests/suites/betxns/betxn_test.py b/dirsrvtests/tests/suites/betxns/betxn_test.py index 2aaddde..94e6081 100644 --- a/dirsrvtests/tests/suites/betxns/betxn_test.py +++ b/dirsrvtests/tests/suites/betxns/betxn_test.py @@ -57,24 +57,20 @@ def test_betxt_7bit(topology_st): user = users.create(properties=TEST_USER_PROPERTIES) # Attempt a modrdn, this should fail - - try: + with pytest.raises(ldap.LDAPError): user.rename(BAD_RDN) - log.fatal('test_betxt_7bit: Modrdn operation incorrectly succeeded') - assert False - except ldap.LDAPError as e: - log.info('Modrdn failed as expected: error %s' % str(e)) # Make sure the operation did not succeed, attempt to search for the new RDN + with pytest.raises(ldap.LDAPError): + users.get(u'Fu\u00c4\u00e8') + # Make sure original entry is present user_check = users.get("testuser") - assert user_check.dn.lower() == user.dn.lower() - # # Cleanup - remove the user - # user.delete() + log.info('test_betxt_7bit: PASSED') @@ -153,17 +149,11 @@ def test_betxn_memberof(topology_st): memberof = MemberOfPlugin(topology_st.standalone) memberof.enable() memberof.set_autoaddoc('referral') - # memberof.add_groupattr('member') # This is already the default. topology_st.standalone.restart() groups = Groups(topology_st.standalone, DEFAULT_SUFFIX) - group1 = groups.create(properties={ - 'cn': 'group1', - }) - - group2 = groups.create(properties={ - 'cn': 'group2', - }) + group1 = groups.create(properties={'cn': 'group1'}) + group2 = groups.create(properties={'cn': 'group2'}) # We may need to mod groups to not have nsMemberOf ... ? if not ds_is_older('1.3.7'): @@ -171,21 +161,18 @@ def test_betxn_memberof(topology_st): group2.remove('objectClass', 'nsMemberOf') # Add group2 to group1 - it should fail with objectclass violation - try: + with pytest.raises(ldap.OBJECT_CLASS_VIOLATION): group1.add_member(group2.dn) - log.fatal('test_betxn_memberof: Group2 was incorrectly allowed to be added to group1') - assert False - except ldap.LDAPError as e: - log.info('test_betxn_memberof: Group2 was correctly rejected (mod add): error: ' + str(e)) - # + # verify entry cache reflects the current/correct state of group1 + assert not group1.is_member(group2.dn) + # Done - # log.info('test_betxn_memberof: PASSED') def test_betxn_modrdn_memberof_cache_corruption(topology_st): - """Test modrdn operations and memberOf + """Test modrdn operations and memberOf be txn post op failures :id: 70d0b96e-b693-4bf7-bbf5-102a66ac5994 @@ -234,9 +221,7 @@ def test_betxn_modrdn_memberof_cache_corruption(topology_st): with pytest.raises(ldap.OBJECT_CLASS_VIOLATION): group.rename('cn=group_to_people', newsuperior=peoplebase) - # # Done - # log.info('test_betxn_modrdn_memberof: PASSED') @@ -311,15 +296,23 @@ def test_ri_and_mep_cache_corruption(topology_st): log.fatal("MEP group was not created for the user") assert False + # Test MEP be txn pre op failure does not corrupt entry cache + # Should get the same exception for both rename attempts + with pytest.raises(ldap.UNWILLING_TO_PERFORM): + mep_group.rename("cn=modrdn group") + + with pytest.raises(ldap.UNWILLING_TO_PERFORM): + mep_group.rename("cn=modrdn group") + # Mess with MEP so it fails mep_plugin.disable() mep_group.delete() mep_plugin.enable() - # Add another group for verify entry cache is not corrupted + # Add another group to verify entry cache is not corrupted test_group = groups.create(properties={'cn': 'test_group'}) - # Delete user, should fail, and user should still be a member + # Delete user, should fail in MEP be txn post op, and user should still be a member with pytest.raises(ldap.NO_SUCH_OBJECT): user.delete() diff --git a/ldap/servers/slapd/back-ldbm/cache.c b/ldap/servers/slapd/back-ldbm/cache.c index ba9d26f..a03cdaa 100644 --- a/ldap/servers/slapd/back-ldbm/cache.c +++ b/ldap/servers/slapd/back-ldbm/cache.c @@ -517,7 +517,8 @@ flush_remove_entry(struct timespec *entry_time, struct timespec *start_time) /* * Flush all the cache entries that were added after the "start time" * This is called when a backend transaction plugin fails, and we need - * to remove all the possible invalid entries in the cache. + * to remove all the possible invalid entries in the cache. We need + * to check both the ID and DN hashtables when checking the entry cache. * * If the ref count is 0, we can straight up remove it from the cache, but * if the ref count is greater than 1, then the entry is currently in use. @@ -528,8 +529,8 @@ flush_remove_entry(struct timespec *entry_time, struct timespec *start_time) static void flush_hash(struct cache *cache, struct timespec *start_time, int32_t type) { + Hashtable *ht = cache->c_idtable; /* start with the ID table as it's in both ENTRY and DN caches */ void *e, *laste = NULL; - Hashtable *ht = cache->c_idtable; cache_lock(cache); @@ -570,6 +571,43 @@ flush_hash(struct cache *cache, struct timespec *start_time, int32_t type) } } + if (type == ENTRY_CACHE) { + /* Also check the DN hashtable */ + ht = cache->c_dntable; + + for (size_t i = 0; i < ht->size; i++) { + e = ht->slot[i]; + while (e) { + struct backcommon *entry = (struct backcommon *)e; + uint64_t remove_it = 0; + if (flush_remove_entry(&entry->ep_create_time, start_time)) { + /* Mark the entry to be removed */ + slapi_log_err(SLAPI_LOG_CACHE, "flush_hash", "[ENTRY CACHE] Removing entry id (%d)\n", + entry->ep_id); + remove_it = 1; + } + laste = e; + e = HASH_NEXT(ht, e); + + if (remove_it) { + /* since we have the cache lock we know we can trust refcnt */ + entry->ep_state |= ENTRY_STATE_INVALID; + if (entry->ep_refcnt == 0) { + entry->ep_refcnt++; + lru_delete(cache, laste); + entrycache_remove_int(cache, laste); + entrycache_return(cache, (struct backentry **)&laste); + } else { + /* Entry flagged for removal */ + slapi_log_err(SLAPI_LOG_CACHE, "flush_hash", + "[ENTRY CACHE] Flagging entry to be removed later: id (%d) refcnt: %d\n", + entry->ep_id, entry->ep_refcnt); + } + } + } + } + } + cache_unlock(cache); } diff --git a/ldap/servers/slapd/back-ldbm/ldbm_add.c b/ldap/servers/slapd/back-ldbm/ldbm_add.c index 8c0439c..0d82ae9 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_add.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_add.c @@ -1221,11 +1221,6 @@ ldbm_back_add(Slapi_PBlock *pb) slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval); } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); - - /* Revert the caches if this is the parent operation */ - if (parent_op) { - revert_cache(inst, &parent_time); - } goto error_return; } @@ -1253,6 +1248,10 @@ ldbm_back_add(Slapi_PBlock *pb) goto common_return; error_return: + /* Revert the caches if this is the parent operation */ + if (parent_op) { + revert_cache(inst, &parent_time); + } if (addingentry_id_assigned) { next_id_return(be, addingentry->ep_id); } diff --git a/ldap/servers/slapd/back-ldbm/ldbm_delete.c b/ldap/servers/slapd/back-ldbm/ldbm_delete.c index 98b3d82..e9f3e32 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_delete.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_delete.c @@ -1279,11 +1279,6 @@ replace_entry: slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, &retval); } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); - - /* Revert the caches if this is the parent operation */ - if (parent_op) { - revert_cache(inst, &parent_time); - } goto error_return; } if (parent_found) { @@ -1370,6 +1365,11 @@ commit_return: goto common_return; error_return: + /* Revert the caches if this is the parent operation */ + if (parent_op) { + revert_cache(inst, &parent_time); + } + if (tombstone) { if (cache_is_in_cache(&inst->inst_cache, tombstone)) { tomb_ep_id = tombstone->ep_id; /* Otherwise, tombstone might have been freed. */ @@ -1388,6 +1388,7 @@ error_return: CACHE_RETURN(&inst->inst_cache, &tombstone); tombstone = NULL; } + if (retval == DB_RUNRECOVERY) { dblayer_remember_disk_filled(li); ldbm_nasty("ldbm_back_delete", "Delete", 79, retval); diff --git a/ldap/servers/slapd/back-ldbm/ldbm_modify.c b/ldap/servers/slapd/back-ldbm/ldbm_modify.c index b90b3e0..b0c477e 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_modify.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_modify.c @@ -873,11 +873,6 @@ ldbm_back_modify(Slapi_PBlock *pb) slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval); } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); - - /* Revert the caches if this is the parent operation */ - if (parent_op) { - revert_cache(inst, &parent_time); - } goto error_return; } retval = plugin_call_mmr_plugin_postop(pb, NULL,SLAPI_PLUGIN_BE_TXN_POST_MODIFY_FN); @@ -901,6 +896,11 @@ ldbm_back_modify(Slapi_PBlock *pb) goto common_return; error_return: + /* Revert the caches if this is the parent operation */ + if (parent_op) { + revert_cache(inst, &parent_time); + } + if (postentry != NULL) { slapi_entry_free(postentry); postentry = NULL; diff --git a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c index 73e50eb..65610d6 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c @@ -1217,11 +1217,6 @@ ldbm_back_modrdn(Slapi_PBlock *pb) slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval); } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); - - /* Revert the caches if this is the parent operation */ - if (parent_op) { - revert_cache(inst, &parent_time); - } goto error_return; } retval = plugin_call_mmr_plugin_postop(pb, NULL,SLAPI_PLUGIN_BE_TXN_POST_MODRDN_FN); @@ -1290,6 +1285,11 @@ ldbm_back_modrdn(Slapi_PBlock *pb) goto common_return; error_return: + /* Revert the caches if this is the parent operation */ + if (parent_op) { + revert_cache(inst, &parent_time); + } + /* result already sent above - just free stuff */ if (postentry) { slapi_entry_free(postentry);