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

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