From 99dbba52eb45628c7f290e9ed3aeabb2a2a67db4 Mon Sep 17 00:00:00 2001
From: Mark Reynolds <mreynolds@redhat.com>
Date: Tue, 13 Dec 2022 09:41:34 -0500
Subject: [PATCH] Issue 5413 - Allow mutliple MemberOf fixup tasks with
 different bases/filters

Description:

A change was made to only allow a single fixup task at a time, but there are
cases where you would want to run mutliple tasks but on different branches/filters.

Now we maintain a linked list of bases/filters of the current running tasks to
monitor this.

relates: https://github.com/389ds/389-ds-base/issues/5413

Reviewed by: tbordaz(Thanks!)
---
 .../suites/memberof_plugin/fixup_test.py      |   5 +-
 ldap/servers/plugins/memberof/memberof.c      | 101 ++++++++++++++----
 2 files changed, 85 insertions(+), 21 deletions(-)

diff --git a/dirsrvtests/tests/suites/memberof_plugin/fixup_test.py b/dirsrvtests/tests/suites/memberof_plugin/fixup_test.py
index 9566e144c..d5369439f 100644
--- a/dirsrvtests/tests/suites/memberof_plugin/fixup_test.py
+++ b/dirsrvtests/tests/suites/memberof_plugin/fixup_test.py
@@ -59,12 +59,15 @@ def test_fixup_task_limit(topo):
     with pytest.raises(ldap.UNWILLING_TO_PERFORM):
         memberof.fixup(DEFAULT_SUFFIX)
 
+    # Add second task but on different suffix which should be allowed
+    memberof.fixup("ou=people," + DEFAULT_SUFFIX)
+
     # Wait for first task to complete
     task.wait()
 
     # Add new task which should be allowed now
     memberof.fixup(DEFAULT_SUFFIX)
-     
+
 
 if __name__ == '__main__':
     # Run isolated
diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c
index 0b8cfe95c..a14617044 100644
--- a/ldap/servers/plugins/memberof/memberof.c
+++ b/ldap/servers/plugins/memberof/memberof.c
@@ -52,7 +52,6 @@ static Slapi_DN* _pluginDN = NULL;
 MemberOfConfig *qsortConfig = 0;
 static int usetxn = 0;
 static int premodfn = 0;
-static PRBool fixup_running = PR_FALSE;
 static PRLock *fixup_lock = NULL;
 static int32_t fixup_progress_count = 0;
 static int64_t fixup_progress_elapsed = 0;
@@ -65,6 +64,15 @@ typedef struct _memberofstringll
     void *next;
 } memberofstringll;
 
+typedef struct _fixup_ll
+{
+    Slapi_DN *sdn;
+    char *filter_str;
+    void *next;
+} mo_fixup_ll;
+
+static mo_fixup_ll *fixup_list = NULL;
+
 typedef struct _memberof_get_groups_data
 {
     MemberOfConfig *config;
@@ -438,6 +446,15 @@ memberof_postop_close(Slapi_PBlock *pb __attribute__((unused)))
     PR_DestroyLock(fixup_lock);
     fixup_lock = NULL;
 
+    mo_fixup_ll *fixup_task = fixup_list;
+    while (fixup_task != NULL) {
+        mo_fixup_ll *tmp = fixup_task;
+        fixup_task = fixup_task->next;
+        slapi_sdn_free(&tmp->sdn);
+        slapi_ch_free_string(&tmp->filter_str);
+        slapi_ch_free((void**)&tmp);
+    }
+
     slapi_log_err(SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM,
                   "<-- memberof_postop_close\n");
     return 0;
@@ -2817,7 +2834,6 @@ memberof_fixup_task_thread(void *arg)
     }
 
     PR_Lock(fixup_lock);
-    fixup_running = PR_TRUE;
     fixup_progress_count = 0;
     fixup_progress_elapsed = slapi_current_rel_time_t();
     fixup_start_time = slapi_current_rel_time_t();
@@ -2849,11 +2865,10 @@ memberof_fixup_task_thread(void *arg)
     /* Mark this as a task operation */
     configCopy.fixup_task = 1;
     configCopy.task = task;
-
+    Slapi_DN *sdn = slapi_sdn_new_dn_byref(td->dn);
     if (usetxn) {
-        Slapi_DN *sdn = slapi_sdn_new_dn_byref(td->dn);
         Slapi_Backend *be = slapi_be_select_exact(sdn);
-        slapi_sdn_free(&sdn);
+
         if (be) {
             fixup_pb = slapi_pblock_new();
             slapi_pblock_set(fixup_pb, SLAPI_BACKEND, be);
@@ -2894,14 +2909,37 @@ done:
                           fixup_progress_count, slapi_current_rel_time_t() - fixup_start_time);
     slapi_task_inc_progress(task);
 
+    /* Cleanup task linked list */
+    PR_Lock(fixup_lock);
+    mo_fixup_ll *prev = NULL;
+    for (mo_fixup_ll *curr = fixup_list; curr; curr = curr->next) {
+        mo_fixup_ll *next = curr->next;
+        if (slapi_sdn_compare(curr->sdn, sdn) == 0 &&
+            strcasecmp(curr->filter_str, td->filter_str) == 0)
+        {
+            /* free current code */
+            slapi_sdn_free(&curr->sdn);
+            slapi_ch_free_string(&curr->filter_str);
+            slapi_ch_free((void**)&curr);
+
+            /* update linked list */
+            if (prev == NULL) {
+                /* first node */
+                fixup_list = next;
+            } else {
+                prev->next = next;
+            }
+            break;
+        }
+        prev = curr;
+    }
+    PR_Unlock(fixup_lock);
+    slapi_sdn_free(&sdn);
+
     /* this will queue the destruction of the task */
     slapi_task_finish(task, rc);
     slapi_task_dec_refcount(task);
 
-    PR_Lock(fixup_lock);
-    fixup_running = PR_FALSE;
-    PR_Unlock(fixup_lock);
-
     slapi_log_err(SLAPI_LOG_INFO, MEMBEROF_PLUGIN_SUBSYSTEM,
                   "memberof_fixup_task_thread - Memberof task finished (processed %d entries in %ld seconds)\n",
                   fixup_progress_count, slapi_current_rel_time_t() - fixup_start_time);
@@ -2919,23 +2957,13 @@ memberof_task_add(Slapi_PBlock *pb,
     int rv = SLAPI_DSE_CALLBACK_OK;
     task_data *mytaskdata = NULL;
     Slapi_Task *task = NULL;
+    Slapi_DN *sdn = NULL;
     char *bind_dn;
     const char *filter;
     const char *dn = 0;
 
     *returncode = LDAP_SUCCESS;
 
-    PR_Lock(fixup_lock);
-    if (fixup_running) {
-        PR_Unlock(fixup_lock);
-        *returncode = LDAP_UNWILLING_TO_PERFORM;
-        slapi_log_err(SLAPI_LOG_ERR, MEMBEROF_PLUGIN_SUBSYSTEM,
-                "memberof_task_add - there is already a fixup task running\n");
-        rv = SLAPI_DSE_CALLBACK_ERROR;
-        goto out;
-    }
-    PR_Unlock(fixup_lock);
-
     /* get arg(s) */
     if ((dn = slapi_entry_attr_get_ref(e, "basedn")) == NULL) {
         *returncode = LDAP_OBJECT_CLASS_VIOLATION;
@@ -2949,6 +2977,39 @@ memberof_task_add(Slapi_PBlock *pb,
         goto out;
     }
 
+    PR_Lock(fixup_lock);
+    sdn = slapi_sdn_new_dn_byval(dn);
+    if (fixup_list == NULL) {
+        fixup_list = (mo_fixup_ll *)slapi_ch_calloc(1, sizeof(mo_fixup_ll));
+        fixup_list->sdn = sdn;
+        fixup_list->filter_str = slapi_ch_strdup(filter);
+    } else {
+        for (mo_fixup_ll *fixup_task = fixup_list; fixup_task; fixup_task = fixup_task->next) {
+            if (slapi_sdn_compare(sdn, fixup_task->sdn) == 0 &&
+                strcasecmp(filter, fixup_task->filter_str) == 0)
+            {
+                /* Found an identical running task, reject it */
+                PR_Unlock(fixup_lock);
+                slapi_log_err(SLAPI_LOG_ERR, MEMBEROF_PLUGIN_SUBSYSTEM,
+                        "memberof_task_add - there is already an identical fixup task running: base: %s filter: %s\n",
+                        slapi_sdn_get_dn(sdn), filter);
+                slapi_sdn_free(&sdn);
+                *returncode = LDAP_UNWILLING_TO_PERFORM;
+                rv = SLAPI_DSE_CALLBACK_ERROR;
+                goto out;
+            }
+        }
+        /* Add the new task DN to the top of the list */
+        mo_fixup_ll *head = fixup_list;
+        mo_fixup_ll *new_task = (mo_fixup_ll *)slapi_ch_calloc(1, sizeof(mo_fixup_ll));
+        new_task->sdn = sdn;
+        new_task->filter_str = slapi_ch_strdup(filter);
+        new_task->next = head;
+        fixup_list = new_task;
+    }
+    PR_Unlock(fixup_lock);
+
+
     /* setup our task data */
     slapi_pblock_get(pb, SLAPI_REQUESTOR_DN, &bind_dn);
     mytaskdata = (task_data *)slapi_ch_malloc(sizeof(task_data));
-- 
2.38.1