From 9a8ee0954699da05501ded2900a834584346ef85 Mon Sep 17 00:00:00 2001 From: Thierry Bordaz 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.21.1