Blob Blame Raw
From 5497023c6857ae0fcf046e57744a91107d362c41 Mon Sep 17 00:00:00 2001
From: Thierry Bordaz <tbordaz@redhat.com>
Date: Mon, 25 Nov 2019 10:59:44 +0100
Subject: [PATCH] Ticket 50736 - RetroCL trimming may crash at shutdown if
 trimming configuration is invalid

Bug Description:
	If config of retroCL trimming contains invalid value for trim-interval
        and/or maxage, then the trimming initialization is skipped.
        In such case the trimming structures are not allocated and if they
        are freed at shutdown it triggers a crash

Fix Description:
        When trimming mechanism is stopped (at shutdown) check that
        it was successfully initialized before freeing the structs

https://pagure.io/389-ds-base/issue/50736

Reviewed by: Mark Reynolds

Platforms tested: F30

Flag Day: no

Doc impact: no
---
 .../suites/replication/changelog_test.py      | 185 ++++++++++++++++++
 ldap/servers/plugins/retrocl/retrocl_trim.c   |  17 +-
 2 files changed, 196 insertions(+), 6 deletions(-)

diff --git a/dirsrvtests/tests/suites/replication/changelog_test.py b/dirsrvtests/tests/suites/replication/changelog_test.py
index 0b6b886f3..0d3e85bb2 100755
--- a/dirsrvtests/tests/suites/replication/changelog_test.py
+++ b/dirsrvtests/tests/suites/replication/changelog_test.py
@@ -16,6 +16,12 @@ from lib389.replica import Replicas
 from lib389.idm.user import UserAccounts
 from lib389.topologies import topology_m2 as topo
 from lib389._constants import *
+from lib389.plugins import RetroChangelogPlugin
+from lib389.dseldif import DSEldif
+from lib389.tasks import *
+from lib389.utils import *
+
+pytestmark = pytest.mark.tier1
 
 TEST_ENTRY_NAME = 'replusr'
 NEW_RDN_NAME = 'cl5usr'
@@ -235,6 +241,185 @@ def test_verify_changelog_offline_backup(topo):
     _check_changelog_ldif(topo, changelog_ldif)
 
 
+@pytest.mark.ds47669
+def test_changelog_maxage(topo, changelog_init):
+    """Check nsslapd-changelog max age values
+
+    :id: d284ff27-03b2-412c-ac74-ac4f2d2fae3b
+    :setup: Replication with two master, change nsslapd-changelogdir to
+    '/var/lib/dirsrv/slapd-master1/changelog' and
+    set cn=Retro Changelog Plugin,cn=plugins,cn=config to 'on'
+    :steps:
+        1. Set nsslapd-changelogmaxage in cn=changelog5,cn=config to values - '12345','10s','30M','12h','2D','4w'
+        2. Set nsslapd-changelogmaxage in cn=changelog5,cn=config to values - '-123','xyz'
+
+    :expectedresults:
+        1. Operation should be successful
+        2. Operation should be unsuccessful
+     """
+    log.info('1. Test nsslapd-changelogmaxage in cn=changelog5,cn=config')
+
+    # bind as directory manager
+    topo.ms["master1"].log.info("Bind as %s" % DN_DM)
+    topo.ms["master1"].simple_bind_s(DN_DM, PASSWORD)
+
+    add_and_check(topo, CHANGELOG, MAXAGE, '12345', True)
+    add_and_check(topo, CHANGELOG, MAXAGE, '10s', True)
+    add_and_check(topo, CHANGELOG, MAXAGE, '30M', True)
+    add_and_check(topo, CHANGELOG, MAXAGE, '12h', True)
+    add_and_check(topo, CHANGELOG, MAXAGE, '2D', True)
+    add_and_check(topo, CHANGELOG, MAXAGE, '4w', True)
+    add_and_check(topo, CHANGELOG, MAXAGE, '-123', False)
+    add_and_check(topo, CHANGELOG, MAXAGE, 'xyz', False)
+
+
+@pytest.mark.ds47669
+def test_ticket47669_changelog_triminterval(topo, changelog_init):
+    """Check nsslapd-changelog triminterval values
+
+    :id: 8f850c37-7e7c-49dd-a4e0-9344638616d6
+    :setup: Replication with two master, change nsslapd-changelogdir to
+    '/var/lib/dirsrv/slapd-master1/changelog' and
+    set cn=Retro Changelog Plugin,cn=plugins,cn=config to 'on'
+    :steps:
+        1. Set nsslapd-changelogtrim-interval in cn=changelog5,cn=config to values -
+           '12345','10s','30M','12h','2D','4w'
+        2. Set nsslapd-changelogtrim-interval in cn=changelog5,cn=config to values - '-123','xyz'
+
+    :expectedresults:
+        1. Operation should be successful
+        2. Operation should be unsuccessful
+     """
+    log.info('2. Test nsslapd-changelogtrim-interval in cn=changelog5,cn=config')
+
+    # bind as directory manager
+    topo.ms["master1"].log.info("Bind as %s" % DN_DM)
+    topo.ms["master1"].simple_bind_s(DN_DM, PASSWORD)
+
+    add_and_check(topo, CHANGELOG, TRIMINTERVAL, '12345', True)
+    add_and_check(topo, CHANGELOG, TRIMINTERVAL, '10s', True)
+    add_and_check(topo, CHANGELOG, TRIMINTERVAL, '30M', True)
+    add_and_check(topo, CHANGELOG, TRIMINTERVAL, '12h', True)
+    add_and_check(topo, CHANGELOG, TRIMINTERVAL, '2D', True)
+    add_and_check(topo, CHANGELOG, TRIMINTERVAL, '4w', True)
+    add_and_check(topo, CHANGELOG, TRIMINTERVAL, '-123', False)
+    add_and_check(topo, CHANGELOG, TRIMINTERVAL, 'xyz', False)
+
+
+@pytest.mark.ds47669
+def test_changelog_compactdbinterval(topo, changelog_init):
+    """Check nsslapd-changelog compactdbinterval values
+
+    :id: 0f4b3118-9dfa-4c2a-945c-72847b42a48c
+    :setup: Replication with two master, change nsslapd-changelogdir to
+    '/var/lib/dirsrv/slapd-master1/changelog' and
+    set cn=Retro Changelog Plugin,cn=plugins,cn=config to 'on'
+    :steps:
+        1. Set nsslapd-changelogcompactdb-interval in cn=changelog5,cn=config to values -
+           '12345','10s','30M','12h','2D','4w'
+        2. Set nsslapd-changelogcompactdb-interval in cn=changelog5,cn=config to values -
+           '-123','xyz'
+
+    :expectedresults:
+        1. Operation should be successful
+        2. Operation should be unsuccessful
+     """
+    log.info('3. Test nsslapd-changelogcompactdb-interval in cn=changelog5,cn=config')
+
+    # bind as directory manager
+    topo.ms["master1"].log.info("Bind as %s" % DN_DM)
+    topo.ms["master1"].simple_bind_s(DN_DM, PASSWORD)
+
+    add_and_check(topo, CHANGELOG, COMPACTDBINTERVAL, '12345', True)
+    add_and_check(topo, CHANGELOG, COMPACTDBINTERVAL, '10s', True)
+    add_and_check(topo, CHANGELOG, COMPACTDBINTERVAL, '30M', True)
+    add_and_check(topo, CHANGELOG, COMPACTDBINTERVAL, '12h', True)
+    add_and_check(topo, CHANGELOG, COMPACTDBINTERVAL, '2D', True)
+    add_and_check(topo, CHANGELOG, COMPACTDBINTERVAL, '4w', True)
+    add_and_check(topo, CHANGELOG, COMPACTDBINTERVAL, '-123', False)
+    add_and_check(topo, CHANGELOG, COMPACTDBINTERVAL, 'xyz', False)
+
+
+@pytest.mark.ds47669
+def test_retrochangelog_maxage(topo, changelog_init):
+    """Check nsslapd-retrochangelog max age values
+
+    :id: 0cb84d81-3e86-4dbf-84a2-66aefd8281db
+    :setup: Replication with two master, change nsslapd-changelogdir to
+    '/var/lib/dirsrv/slapd-master1/changelog' and
+    set cn=Retro Changelog Plugin,cn=plugins,cn=config to 'on'
+    :steps:
+        1. Set nsslapd-changelogmaxage in cn=Retro Changelog Plugin,cn=plugins,cn=config to values -
+           '12345','10s','30M','12h','2D','4w'
+        2. Set nsslapd-changelogmaxage in cn=Retro Changelog Plugin,cn=plugins,cn=config to values -
+           '-123','xyz'
+
+    :expectedresults:
+        1. Operation should be successful
+        2. Operation should be unsuccessful
+     """
+    log.info('4. Test nsslapd-changelogmaxage in cn=Retro Changelog Plugin,cn=plugins,cn=config')
+
+    # bind as directory manager
+    topo.ms["master1"].log.info("Bind as %s" % DN_DM)
+    topo.ms["master1"].simple_bind_s(DN_DM, PASSWORD)
+
+    add_and_check(topo, RETROCHANGELOG, MAXAGE, '12345', True)
+    add_and_check(topo, RETROCHANGELOG, MAXAGE, '10s', True)
+    add_and_check(topo, RETROCHANGELOG, MAXAGE, '30M', True)
+    add_and_check(topo, RETROCHANGELOG, MAXAGE, '12h', True)
+    add_and_check(topo, RETROCHANGELOG, MAXAGE, '2D', True)
+    add_and_check(topo, RETROCHANGELOG, MAXAGE, '4w', True)
+    add_and_check(topo, RETROCHANGELOG, MAXAGE, '-123', False)
+    add_and_check(topo, RETROCHANGELOG, MAXAGE, 'xyz', False)
+
+    topo.ms["master1"].log.info("ticket47669 was successfully verified.")
+
+@pytest.mark.ds50736
+def test_retrochangelog_trimming_crash(topo, changelog_init):
+    """Check that when retroCL nsslapd-retrocthangelog contains invalid
+    value, then the instance does not crash at shutdown
+
+    :id: 5d9bd7ca-e9bf-4be9-8fc8-902aa5513052
+    :setup: Replication with two master, change nsslapd-changelogdir to
+    '/var/lib/dirsrv/slapd-master1/changelog' and
+    set cn=Retro Changelog Plugin,cn=plugins,cn=config to 'on'
+    :steps:
+        1. Set nsslapd-changelogmaxage in cn=Retro Changelog Plugin,cn=plugins,cn=config to value '-1'
+           This value is invalid. To disable retroCL trimming it should be set to 0
+        2. Do several restart
+        3. check there is no 'Detected Disorderly Shutdown' message (crash)
+        4. restore valid value for nsslapd-changelogmaxage '1w'
+
+    :expectedresults:
+        1. Operation should be successful
+        2. Operation should be successful
+        3. Operation should be successful
+        4. Operation should be successful
+     """
+    log.info('1. Test retroCL trimming crash in cn=Retro Changelog Plugin,cn=plugins,cn=config')
+
+    # set the nsslapd-changelogmaxage directly on dse.ldif
+    # because the set value is invalid
+    topo.ms["master1"].log.info("ticket50736 start verification")
+    topo.ms["master1"].stop()
+    retroPlugin = RetroChangelogPlugin(topo.ms["master1"])
+    dse_ldif = DSEldif(topo.ms["master1"])
+    dse_ldif.replace(retroPlugin.dn, 'nsslapd-changelogmaxage', '-1')
+    topo.ms["master1"].start()
+
+    # The crash should be systematic, but just in case do several restart
+    # with a delay to let all plugin init
+    for i in range(5):
+        time.sleep(1)
+        topo.ms["master1"].stop()
+        topo.ms["master1"].start()
+
+    assert not topo.ms["master1"].detectDisorderlyShutdown()
+
+    topo.ms["master1"].log.info("ticket 50736 was successfully verified.")
+
+
 if __name__ == '__main__':
     # Run isolated
     # -s for DEBUG mode
diff --git a/ldap/servers/plugins/retrocl/retrocl_trim.c b/ldap/servers/plugins/retrocl/retrocl_trim.c
index a46534984..0378eb7f6 100644
--- a/ldap/servers/plugins/retrocl/retrocl_trim.c
+++ b/ldap/servers/plugins/retrocl/retrocl_trim.c
@@ -481,11 +481,16 @@ retrocl_init_trimming(void)
 void
 retrocl_stop_trimming(void)
 {
-    retrocl_trimming = 0;
-    if (retrocl_trim_ctx) {
-        slapi_eq_cancel(retrocl_trim_ctx);
-        retrocl_trim_ctx = NULL;
+    if (retrocl_trimming) {
+        /* RetroCL trimming config was valid and trimming struct allocated
+         * Let's free them
+         */
+        retrocl_trimming = 0;
+        if (retrocl_trim_ctx) {
+            slapi_eq_cancel(retrocl_trim_ctx);
+            retrocl_trim_ctx = NULL;
+        }
+        PR_DestroyLock(ts.ts_s_trim_mutex);
+        ts.ts_s_trim_mutex = NULL;
     }
-    PR_DestroyLock(ts.ts_s_trim_mutex);
-    ts.ts_s_trim_mutex = NULL;
 }
-- 
2.24.1