Blame SOURCES/0026-Ticket-49624-cont-DB-Deadlock-on-modrdn-appears-to-c.patch

8394b4
From f9c0ae9e0c143359ef12c8f5ae3070e34afd5495 Mon Sep 17 00:00:00 2001
8394b4
From: Ludwig Krispenz <lkrispen@redhat.com>
8394b4
Date: Wed, 15 Jan 2020 13:40:36 +0100
8394b4
Subject: [PATCH] Ticket 49624 cont - DB Deadlock on modrdn appears to corrupt
8394b4
 database and entry cache
8394b4
8394b4
Bug: If there are deadlocks a transaction will be retried. In the case
8394b4
	of modrdn operation there is an error in handling the newsuperior
8394b4
	dn, which has to be reset when the txn is repeated.
8394b4
     There is also an error in freeing the entry stored in the pblock which can
8394b4
	lead to a double free
8394b4
     There is also a memory leak for ec entries
8394b4
8394b4
Fix: check if the newsuperior in the pblock was changed before the retry and
8394b4
	only then free and reset it.
8394b4
     check and protect pblock entry from double free
8394b4
     remove ec entry from cache
8394b4
     fix the txn_test_thread to run
8394b4
8394b4
     There is also a message at shutdown that entries remain in the entry cache
8394b4
	although no leaks are reported and a hash dump didn't show entries.
8394b4
	Change log level to avoid confusion
8394b4
8394b4
Reviewed by: Thierry, William, Viktor - Thanks
8394b4
---
8394b4
 ldap/servers/slapd/back-ldbm/cache.c          |  2 +-
8394b4
 .../slapd/back-ldbm/db-bdb/bdb_layer.c        |  2 +-
8394b4
 ldap/servers/slapd/back-ldbm/ldbm_modrdn.c    | 60 +++++++++++++------
8394b4
 3 files changed, 45 insertions(+), 19 deletions(-)
8394b4
8394b4
diff --git a/ldap/servers/slapd/back-ldbm/cache.c b/ldap/servers/slapd/back-ldbm/cache.c
8394b4
index a03cdaa83..89f958a35 100644
8394b4
--- a/ldap/servers/slapd/back-ldbm/cache.c
8394b4
+++ b/ldap/servers/slapd/back-ldbm/cache.c
8394b4
@@ -723,7 +723,7 @@ entrycache_clear_int(struct cache *cache)
8394b4
     }
8394b4
     cache->c_maxsize = size;
8394b4
     if (cache->c_curentries > 0) {
8394b4
-        slapi_log_err(SLAPI_LOG_WARNING,
8394b4
+        slapi_log_err(SLAPI_LOG_CACHE,
8394b4
                       "entrycache_clear_int", "There are still %" PRIu64 " entries "
8394b4
                                               "in the entry cache.\n",
8394b4
                       cache->c_curentries);
8394b4
diff --git a/ldap/servers/slapd/back-ldbm/db-bdb/bdb_layer.c b/ldap/servers/slapd/back-ldbm/db-bdb/bdb_layer.c
8394b4
index 5a6a2a2e5..36bf42dab 100644
8394b4
--- a/ldap/servers/slapd/back-ldbm/db-bdb/bdb_layer.c
8394b4
+++ b/ldap/servers/slapd/back-ldbm/db-bdb/bdb_layer.c
8394b4
@@ -3064,7 +3064,7 @@ txn_test_threadmain(void *param)
8394b4
 
8394b4
     txn_test_init_cfg(&cfg);
8394b4
 
8394b4
-    if(BDB_CONFIG(li)->bdb_enable_transactions) {
8394b4
+    if(!BDB_CONFIG(li)->bdb_enable_transactions) {
8394b4
         goto end;
8394b4
     }
8394b4
 
8394b4
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c
8394b4
index 433ed88fb..26698012a 100644
8394b4
--- a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c
8394b4
+++ b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c
8394b4
@@ -67,6 +67,7 @@ ldbm_back_modrdn(Slapi_PBlock *pb)
8394b4
     Slapi_DN *dn_newsuperiordn = NULL;
8394b4
     Slapi_DN dn_parentdn;
8394b4
     Slapi_DN *orig_dn_newsuperiordn = NULL;
8394b4
+    Slapi_DN *pb_dn_newsuperiordn = NULL; /* used to check what is currently in the pblock */
8394b4
     Slapi_Entry *target_entry = NULL;
8394b4
     Slapi_Entry *original_targetentry = NULL;
8394b4
     int rc;
8394b4
@@ -248,30 +249,45 @@ ldbm_back_modrdn(Slapi_PBlock *pb)
8394b4
             slapi_sdn_set_dn_byref(&dn_newrdn, original_newrdn);
8394b4
             original_newrdn = slapi_ch_strdup(original_newrdn);
8394b4
 
8394b4
-            slapi_pblock_get(pb, SLAPI_MODRDN_NEWSUPERIOR_SDN, &dn_newsuperiordn);
8394b4
-            slapi_sdn_free(&dn_newsuperiordn);
8394b4
-            slapi_pblock_set(pb, SLAPI_MODRDN_NEWSUPERIOR_SDN, orig_dn_newsuperiordn);
8394b4
-            dn_newsuperiordn = slapi_sdn_dup(orig_dn_newsuperiordn);
8394b4
+            /* we need to restart with the original newsuperiordn which could have
8394b4
+             * been modified. So check what is in the pblock, if it was changed
8394b4
+             * free it, reset orig dn in th epblock and recreate a working superior
8394b4
+             */
8394b4
+            slapi_pblock_get(pb, SLAPI_MODRDN_NEWSUPERIOR_SDN, &pb_dn_newsuperiordn);
8394b4
+            if (pb_dn_newsuperiordn != orig_dn_newsuperiordn) {
8394b4
+                slapi_sdn_free(&pb_dn_newsuperiordn);
8394b4
+                slapi_pblock_set(pb, SLAPI_MODRDN_NEWSUPERIOR_SDN, orig_dn_newsuperiordn);
8394b4
+                dn_newsuperiordn = slapi_sdn_dup(orig_dn_newsuperiordn);
8394b4
+            }
8394b4
             /* must duplicate ec before returning it to cache,
8394b4
              * which could free the entry. */
8394b4
-            if ((tmpentry = backentry_dup(original_entry ? original_entry : ec)) == NULL) {
8394b4
+            if (!original_entry) {
8394b4
+                slapi_log_err(SLAPI_LOG_ERR, "ldbm_back_modrdn",
8394b4
+                    "retrying transaction, but no original entry found\n");
8394b4
+                ldap_result_code = LDAP_OPERATIONS_ERROR;
8394b4
+                goto error_return;
8394b4
+            }
8394b4
+            if ((tmpentry = backentry_dup(original_entry)) == NULL) {
8394b4
                 ldap_result_code = LDAP_OPERATIONS_ERROR;
8394b4
                 goto error_return;
8394b4
             }
8394b4
             slapi_pblock_get(pb, SLAPI_MODRDN_EXISTING_ENTRY, &ent);
8394b4
             if (cache_is_in_cache(&inst->inst_cache, ec)) {
8394b4
                 CACHE_REMOVE(&inst->inst_cache, ec);
8394b4
-                if (ent && (ent == ec->ep_entry)) {
8394b4
-                    /*
8394b4
-                     * On a retry, it's possible that ec is now stored in the
8394b4
-                     * pblock as SLAPI_MODRDN_EXISTING_ENTRY.  "ec" will be freed
8394b4
-                     * by CACHE_RETURN below, so set ent to NULL so don't free
8394b4
-                     * it again.
8394b4
-                     */
8394b4
-                    ent = NULL;
8394b4
-                }
8394b4
+            }
8394b4
+            if (ent && (ent == ec->ep_entry)) {
8394b4
+                /*
8394b4
+                 * On a retry, it's possible that ec is now stored in the
8394b4
+                 * pblock as SLAPI_MODRDN_EXISTING_ENTRY.  "ec" will be freed
8394b4
+                 * by CACHE_RETURN below, so set ent to NULL so don't free
8394b4
+                 * it again.
8394b4
+                 * And it needs to be checked always.
8394b4
+                 */
8394b4
+                ent = NULL;
8394b4
             }
8394b4
             CACHE_RETURN(&inst->inst_cache, &ec);
8394b4
+
8394b4
+            /* LK why do we need this ????? */
8394b4
             if (!cache_is_in_cache(&inst->inst_cache, e)) {
8394b4
                 if (CACHE_ADD(&inst->inst_cache, e, NULL) < 0) {
8394b4
                     slapi_log_err(SLAPI_LOG_CACHE,
8394b4
@@ -1087,8 +1103,9 @@ ldbm_back_modrdn(Slapi_PBlock *pb)
8394b4
         if (slapi_sdn_get_dn(dn_newsuperiordn) != NULL) {
8394b4
             retval = ldbm_ancestorid_move_subtree(be, sdn, &dn_newdn, e->ep_id, children, &txn);
8394b4
             if (retval != 0) {
8394b4
-                if (retval == DB_LOCK_DEADLOCK)
8394b4
+                if (retval == DB_LOCK_DEADLOCK) {
8394b4
                     continue;
8394b4
+                }
8394b4
                 if (retval == DB_RUNRECOVERY || LDBM_OS_ERR_IS_DISKFULL(retval))
8394b4
                     disk_full = 1;
8394b4
                 MOD_SET_ERROR(ldap_result_code,
8394b4
@@ -1108,8 +1125,9 @@ ldbm_back_modrdn(Slapi_PBlock *pb)
8394b4
                                              e->ep_id, &txn, is_tombstone);
8394b4
             slapi_rdn_done(&newsrdn);
8394b4
             if (retval != 0) {
8394b4
-                if (retval == DB_LOCK_DEADLOCK)
8394b4
+                if (retval == DB_LOCK_DEADLOCK) {
8394b4
                     continue;
8394b4
+                }
8394b4
                 if (retval == DB_RUNRECOVERY || LDBM_OS_ERR_IS_DISKFULL(retval))
8394b4
                     disk_full = 1;
8394b4
                 MOD_SET_ERROR(ldap_result_code, LDAP_OPERATIONS_ERROR, retry_count);
8394b4
@@ -1500,7 +1518,12 @@ common_return:
8394b4
     done_with_pblock_entry(pb, SLAPI_MODRDN_NEWPARENT_ENTRY);
8394b4
     done_with_pblock_entry(pb, SLAPI_MODRDN_TARGET_ENTRY);
8394b4
     slapi_ch_free_string(&original_newrdn);
8394b4
-    slapi_sdn_free(&orig_dn_newsuperiordn);
8394b4
+    slapi_pblock_get(pb, SLAPI_MODRDN_NEWSUPERIOR_SDN, &pb_dn_newsuperiordn);
8394b4
+    if (pb_dn_newsuperiordn != orig_dn_newsuperiordn) {
8394b4
+        slapi_sdn_free(&orig_dn_newsuperiordn);
8394b4
+    } else {
8394b4
+        slapi_sdn_free(&dn_newsuperiordn);
8394b4
+    }
8394b4
     backentry_free(&original_entry);
8394b4
     backentry_free(&tmpentry);
8394b4
     slapi_entry_free(original_targetentry);
8394b4
@@ -1561,6 +1584,9 @@ moddn_unlock_and_return_entry(
8394b4
     /* Something bad happened so we should give back all the entries */
8394b4
     if (*targetentry != NULL) {
8394b4
         cache_unlock_entry(&inst->inst_cache, *targetentry);
8394b4
+        if (cache_is_in_cache(&inst->inst_cache, *targetentry)) {
8394b4
+            CACHE_REMOVE(&inst->inst_cache, *targetentry);
8394b4
+        }
8394b4
         CACHE_RETURN(&inst->inst_cache, targetentry);
8394b4
         *targetentry = NULL;
8394b4
     }
8394b4
-- 
8394b4
2.21.1
8394b4