Hello,

I was fighting with random crashes for couple of days ... and discovered that run_exclusive_enter()/isc_task_beginexclusive() usage was completely incorrect and didn't actually lock anything.

This series of patches reworks internal locking (and related event system) to work around limitations of isc_task_beginexclusive() mechanism.

It would be better to get rid of isc_task_beginexclusive() completely but IMHO it is not possible because of BIND's dns_view*() functions have to be guarded with it.


Testing is going to be interesting because we are speaking about race 
conditions.

I used ~ 100 DNS zones, each zone had ~ 100 random domain names inside with random A/AAAA/TXT RRs. My LDIF is here:
http://people.redhat.com/~pspacek/a/2014/09/11/dns-test.ldif.xz

I was able to randomly reproduce various crashes when BIND was running with more threads than usually.

You can try to run BIND with this command (as root) and play games with -n parameter:
$ export KRB5_KTNAME="/etc/named.keytab"
$ named -4 -g -u named -m record -n 10

Please test also the case where BIND receives SIGINT during start-up. It is possible to run BIND with commands above and wait for message:
11-Sep-2014 11:54:58.092 running

At this point send SIGINT (CTRL+C) to BIND and see what happens. It could crash or deadlock.

It is necessary to send the signal before BIND prints this message:
11-Sep-2014 11:55:11.707 zone z1.test/IN: loaded serial 1410429304

Let me know if you need any assistance.

--
Petr^2 Spacek
From 54c4b11394863355e4da69514dd065da64f8bc9f Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Wed, 10 Sep 2014 16:48:52 +0200
Subject: [PATCH] Rework locking in settings.c module.

Locking mechanism run_exclusive_enter()/exit() works correctly only if
all calls operate on the same task.

Locking was reworked to use ordinary mutex stored in settings_set_t
structure.

Signed-off-by: Petr Spacek <pspa...@redhat.com>
---
 src/ldap_helper.c | 32 ++++++++++++++---------------
 src/settings.c    | 61 ++++++++++++++++++++++++++++++-------------------------
 src/settings.h    | 11 +++++-----
 3 files changed, 54 insertions(+), 50 deletions(-)

diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index b3cc7f8389e52decd2f90a18eae761fbc37433a0..89d585abe1cda14fb23807e612970e9bd1e84459 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -394,7 +394,7 @@ validate_local_instance_settings(ldap_instance_t *inst, settings_set_t *set) {
 
 	if (strcmp(dir_name, str_buf(buff)) != 0)
 		CHECK(setting_set("directory", inst->local_settings,
-				  str_buf(buff), inst->task));
+				  str_buf(buff)));
 	str_destroy(&buff);
 	dir_name = NULL;
 	CHECK(setting_get_str("directory", inst->local_settings, &dir_name));
@@ -429,8 +429,7 @@ validate_local_instance_settings(ldap_instance_t *inst, settings_set_t *set) {
 		CLEANUP_WITH(ISC_R_FAILURE);
 	}
 	CHECK(isc_string_printf(print_buff, PRINT_BUFF_SIZE, "%u", auth_method_enum));
-	CHECK(setting_set("auth_method_enum", inst->local_settings, print_buff,
-			  inst->task));
+	CHECK(setting_set("auth_method_enum", inst->local_settings, print_buff));
 
 	/* check we have the right data when SASL/GSSAPI is selected */
 	CHECK(setting_get_str("sasl_mech", set, &sasl_mech));
@@ -485,13 +484,11 @@ validate_local_instance_settings(ldap_instance_t *inst, settings_set_t *set) {
 						  "default '%s'",
 						  str_buf(buff));
 					CHECK(setting_set("krb5_principal", set,
-							  str_buf(buff),
-							  inst->task));
+							  str_buf(buff)));
 				}
 			} else {
 				CHECK(setting_set("krb5_principal", set,
-						  sasl_user,
-						  inst->task));
+						  sasl_user));
 			}
 		}
 	} else if (auth_method_enum == AUTH_SASL) {
@@ -559,7 +556,7 @@ new_ldap_instance(isc_mem_t *mctx, const char *db_name,
 	      sizeof(settings_global_default), settings_name,
 	      ldap_inst->local_settings, &ldap_inst->global_settings));
 
-	CHECK(settings_set_fill(ldap_inst->local_settings, argv, task));
+	CHECK(settings_set_fill(ldap_inst->local_settings, argv));
 	CHECK(validate_local_instance_settings(ldap_inst, ldap_inst->local_settings));
 	if (settings_set_isfilled(ldap_inst->global_settings) != ISC_TRUE)
 		CLEANUP_WITH(ISC_R_FAILURE);
@@ -1613,8 +1610,7 @@ cleanup:
 
 /* Parse the config object entry */
 static isc_result_t ATTR_NONNULLS ATTR_CHECKRESULT
-ldap_parse_configentry(ldap_entry_t *entry, ldap_instance_t *inst,
-		       isc_task_t * const task)
+ldap_parse_configentry(ldap_entry_t *entry, ldap_instance_t *inst)
 {
 	isc_result_t result;
 
@@ -1632,14 +1628,14 @@ ldap_parse_configentry(ldap_entry_t *entry, ldap_instance_t *inst,
 	result = setting_update_from_ldap_entry("dyn_update",
 						inst->global_settings,
 						"idnsAllowDynUpdate",
-						entry, task);
+						entry);
 	if (result != ISC_R_SUCCESS && result != ISC_R_IGNORE)
 		goto cleanup;
 
 	result = setting_update_from_ldap_entry("sync_ptr",
 						inst->global_settings,
 						"idnsAllowSyncPTR",
-						entry, task);
+						entry);
 	if (result != ISC_R_SUCCESS && result != ISC_R_IGNORE)
 		goto cleanup;
 
@@ -1991,18 +1987,18 @@ zone_master_reconfigure(ldap_entry_t *entry, settings_set_t *zone_settings,
 		dns_zone_attach(raw, &inview);
 
 	result = setting_update_from_ldap_entry("dyn_update", zone_settings,
-						"idnsAllowDynUpdate", entry, task);
+						"idnsAllowDynUpdate", entry);
 	if (result != ISC_R_SUCCESS && result != ISC_R_IGNORE)
 		goto cleanup;
 	ssu_changed = (result == ISC_R_SUCCESS);
 
 	result = setting_update_from_ldap_entry("sync_ptr", zone_settings,
-				       "idnsAllowSyncPTR", entry, task);
+				       "idnsAllowSyncPTR", entry);
 	if (result != ISC_R_SUCCESS && result != ISC_R_IGNORE)
 		goto cleanup;
 
 	result = setting_update_from_ldap_entry("update_policy", zone_settings,
-						"idnsUpdatePolicy", entry, task);
+						"idnsUpdatePolicy", entry);
 	if (result != ISC_R_SUCCESS && result != ISC_R_IGNORE)
 		goto cleanup;
 
@@ -2084,7 +2080,7 @@ zone_master_reconfigure(ldap_entry_t *entry, settings_set_t *zone_settings,
 		result = setting_update_from_ldap_entry("nsec3param",
 							zone_settings,
 							"nsec3paramRecord",
-							entry, task);
+							entry);
 		if (result == ISC_R_SUCCESS)
 			CHECK(zone_master_reconfigure_nsec3param(zone_settings,
 								 secure));
@@ -4293,10 +4289,12 @@ update_config(isc_task_t * task, isc_event_t *event)
 	ldap_entry_t *entry = pevent->entry;
 	isc_mem_t *mctx;
 
+	UNUSED(task);
+
 	mctx = pevent->mctx;
 
 	CHECK(manager_get_ldap_instance(pevent->dbname, &inst));
-	CHECK(ldap_parse_configentry(entry, inst, task));
+	CHECK(ldap_parse_configentry(entry, inst));
 
 cleanup:
 	if (inst != NULL)
diff --git a/src/settings.c b/src/settings.c
index c38c15d6c6b32dee9125951cd5de1c9c02ed5733..42a9be299736a28e1674feacd6bd6a48db5cdea5 100644
--- a/src/settings.c
+++ b/src/settings.c
@@ -32,7 +32,6 @@
 #include <string.h>
 #include <strings.h>
 
-#include "lock.h"
 #include "log.h"
 #include "settings.h"
 #include "str.h"
@@ -86,6 +85,7 @@ const settings_set_t settings_default_set = {
 	NULL,
 	"built-in defaults",
 	NULL,
+	NULL,
 	(setting_t *) &settings_default[0]
 };
 
@@ -220,28 +220,30 @@ setting_get_bool(const char *const name, const settings_set_t *const set,
 }
 
 /**
- * Convert and copy value to setting structure. Mutual exclusion during write
- * is ensured by isc_task_beginexclusive(task).
+ * Convert and copy value to setting structure.
  *
  * @retval ISC_R_SUCCESS  New value was converted and copied.
  * @retval ISC_R_IGNORE   New and old values are same, no change was made.
  * @retval ISC_R_NOMEMORY
  * @retval ISC_R_UNEXPECTEDEND
  * @retval ISC_R_UNEXPECTEDTOKEN
  * @retval others         Other errors from isc_parse_uint32().
  */
 static isc_result_t ATTR_NONNULLS ATTR_CHECKRESULT
-set_value(isc_mem_t *mctx, setting_t *setting, const char *value,
-	  isc_task_t *task)
+set_value(isc_mem_t *mctx, const settings_set_t *set, setting_t *setting,
+	  const char *value)
 {
 	isc_result_t result;
-	isc_result_t lock_state = ISC_R_IGNORE;
 	isc_uint32_t numeric_value;
 	isc_uint32_t len;
 
 	REQUIRE(setting != NULL);
 	REQUIRE(value != NULL);
-	REQUIRE(task != NULL);
+	REQUIRE(set != NULL);
+
+	/* catch attempts to modify built-in defaults */
+	REQUIRE(set->lock != NULL);
+	LOCK(set->lock);
 
 	/* Check and convert new values. */
 	switch (setting->type) {
@@ -289,9 +291,6 @@ set_value(isc_mem_t *mctx, setting_t *setting, const char *value,
 		break;
 	}
 
-	/* Switch to single thread mode and write new value. */
-	run_exclusive_enter(task, &lock_state);
-
 	switch (setting->type) {
 	case ST_STRING:
 		len = strlen(value) + 1;
@@ -318,7 +317,7 @@ set_value(isc_mem_t *mctx, setting_t *setting, const char *value,
 	result = ISC_R_SUCCESS;
 
 cleanup:
-	run_exclusive_exit(task, lock_state);
+	UNLOCK(set->lock);
 	return result;
 }
 
@@ -341,14 +340,14 @@ cleanup:
  */
 isc_result_t
 setting_set(const char *const name, const settings_set_t *set,
-	    const char *const value, isc_task_t *task)
+	    const char *const value)
 {
 	isc_result_t result;
 	setting_t *setting = NULL;
 
 	CHECK(setting_find(name, set, ISC_FALSE, ISC_FALSE, &setting));
 
-	return set_value(set->mctx, setting, value, task);
+	return set_value(set->mctx, set, setting, value);
 
 cleanup:
 	log_bug("setting '%s' was not found in set of settings '%s'", name,
@@ -371,22 +370,18 @@ cleanup:
  * @retval ISC_R_NOTFOUND Required setting was not found
  *                        in given set of settings.
  */
-isc_result_t
-setting_unset(const char *const name, const settings_set_t *set,
-	      isc_task_t *task)
+static isc_result_t
+setting_unset(const char *const name, const settings_set_t *set)
 {
 	isc_result_t result;
-	isc_result_t lock_state = ISC_R_IGNORE;
 	setting_t *setting = NULL;
 
-	REQUIRE(task != NULL);
-
 	CHECK(setting_find(name, set, ISC_FALSE, ISC_FALSE, &setting));
 
 	if (!setting->filled)
 		return ISC_R_IGNORE;
 
-	run_exclusive_enter(task, &lock_state);
+	LOCK(set->lock);
 
 	switch (setting->type) {
 	case ST_STRING:
@@ -406,7 +401,7 @@ setting_unset(const char *const name, const settings_set_t *set,
 	setting->filled = 0;
 
 cleanup:
-	run_exclusive_exit(task, lock_state);
+	UNLOCK(set->lock);
 	if (result == ISC_R_NOTFOUND)
 		log_bug("setting '%s' was not found in set of settings '%s'",
 			name, set->name);
@@ -428,16 +423,15 @@ cleanup:
  */
 isc_result_t
 setting_update_from_ldap_entry(const char *name, settings_set_t *set,
-			       const char *attr_name, ldap_entry_t *entry,
-			       isc_task_t *task) {
+			       const char *attr_name, ldap_entry_t *entry) {
 	isc_result_t result;
 	setting_t *setting = NULL;
 	ldap_valuelist_t values;
 
 	CHECK(setting_find(name, set, ISC_FALSE, ISC_FALSE, &setting));
 	result = ldap_entry_getvalues(entry, attr_name, &values);
 	if (result == ISC_R_NOTFOUND || HEAD(values) == NULL) {
-		CHECK(setting_unset(name, set, task));
+		CHECK(setting_unset(name, set));
 		log_debug(2, "setting '%s' (%s) was deleted in object '%s'",
 			  name, attr_name, entry->dn);
 		return ISC_R_SUCCESS;
@@ -452,7 +446,7 @@ setting_update_from_ldap_entry(const char *name, settings_set_t *set,
 		return ISC_R_NOTIMPLEMENTED;
 	}
 
-	CHECK(setting_set(name, set, HEAD(values)->value, task));
+	CHECK(setting_set(name, set, HEAD(values)->value));
 	log_debug(2, "setting '%s' (%s) was changed to '%s' in object '%s'",
 		  name, attr_name, HEAD(values)->value, entry->dn);
 
@@ -513,6 +507,11 @@ settings_set_create(isc_mem_t *mctx, const setting_t default_settings[],
 	CHECKED_MEM_ALLOCATE(mctx, new_set, default_set_length);
 	ZERO_PTR(new_set);
 	isc_mem_attach(mctx, &new_set->mctx);
+
+	CHECKED_MEM_GET_PTR(mctx, new_set->lock);
+	result = isc_mutex_init(new_set->lock);
+	INSIST(result == ISC_R_SUCCESS);
+
 	new_set->parent_set = parent_set;
 
 	CHECKED_MEM_ALLOCATE(mctx, new_set->first_setting, default_set_length);
@@ -546,6 +545,12 @@ settings_set_free(settings_set_t **set) {
 
 	if ((*set)->mctx != NULL) {
 		mctx = (*set)->mctx;
+
+		if ((*set)->lock != NULL) {
+			isc_mutex_destroy((*set)->lock);
+			SAFE_MEM_PUT_PTR(mctx, (*set)->lock);
+		}
+
 		for (s = (*set)->first_setting; s->name != NULL; s++) {
 			if (s->is_dynamic)
 				isc_mem_free(mctx, s->value.value_char);
@@ -556,6 +561,7 @@ settings_set_free(settings_set_t **set) {
 		isc_mem_free(mctx, *set);
 		isc_mem_detach(&mctx);
 	}
+
 	*set = NULL;
 }
 
@@ -586,8 +592,7 @@ settings_set_free(settings_set_t **set) {
  * @endcode
  */
 isc_result_t
-settings_set_fill(settings_set_t *set, const char *const *argv,
-		  isc_task_t *task)
+settings_set_fill(settings_set_t *set, const char *const *argv)
 {
 	isc_result_t result;
 	int i;
@@ -608,7 +613,7 @@ settings_set_fill(settings_set_t *set, const char *const *argv,
 				  "set of settings '%s'", name, set->name);
 			CLEANUP_WITH(ISC_R_EXISTS);
 		}
-		result = setting_set(name, set, value, task);
+		result = setting_set(name, set, value);
 		if (result != ISC_R_SUCCESS && result != ISC_R_IGNORE)
 			goto cleanup;
 	}
diff --git a/src/settings.h b/src/settings.h
index 8e3d362657b57d57179f77f6783b063b6143a1d5..610e4283254873ae63cfd7059d64f125ebe74ad9 100644
--- a/src/settings.h
+++ b/src/settings.h
@@ -58,6 +58,7 @@ struct settings_set {
 	isc_mem_t		*mctx;
 	char			*name;
 	const settings_set_t	*parent_set;
+	isc_mutex_t		*lock;  /**< locks only values */
 	setting_t		*first_setting;
 };
 
@@ -97,8 +98,8 @@ void
 settings_set_free(settings_set_t **set) ATTR_NONNULLS;
 
 isc_result_t
-settings_set_fill(settings_set_t *set, const char *const *argv,
-		  isc_task_t *task) ATTR_NONNULLS ATTR_CHECKRESULT;
+settings_set_fill(settings_set_t *set, const char *const *argv)
+		  ATTR_NONNULLS ATTR_CHECKRESULT;
 
 isc_boolean_t
 settings_set_isfilled(settings_set_t *set) ATTR_NONNULLS ATTR_CHECKRESULT;
@@ -117,12 +118,12 @@ setting_get_bool(const char * const name, const settings_set_t * const set,
 
 isc_result_t
 setting_set(const char *const name, const settings_set_t *set,
-	    const char *const value, isc_task_t *task) ATTR_NONNULLS ATTR_CHECKRESULT;
+	    const char *const value) ATTR_NONNULLS ATTR_CHECKRESULT;
 
 isc_result_t
 setting_update_from_ldap_entry(const char *name, settings_set_t *set,
-			       const char *attr_name, ldap_entry_t *entry,
-			       isc_task_t *task) ATTR_NONNULLS ATTR_CHECKRESULT;
+			       const char *attr_name, ldap_entry_t *entry)
+			       ATTR_NONNULLS ATTR_CHECKRESULT;
 
 isc_result_t
 get_enum_description(const enum_txt_assoc_t *map, int value, const char **desc) ATTR_NONNULLS ATTR_CHECKRESULT;
-- 
1.9.3

From 4e4a2d7fa25b2857d36a62d8c0a52354ab756980 Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Wed, 10 Sep 2014 19:43:09 +0200
Subject: [PATCH] Rework locking around DNS views.

It turned out that previous usage of isc_task_beginexclusive() was
completely incorrect and didn't actually lock anything. As a result the
plugin was randomly crashing.

Code was reworked in a way that isc_task_beginexclusive() is always called on
inst->task and all isc_task_beginexclusive() calls are made from inside
of inst->task.

See comments in lock.c for more details.

Signed-off-by: Petr Spacek <pspa...@redhat.com>
---
 src/ldap_helper.c   | 57 +++++++++++++++++++++-------------
 src/ldap_helper.h   |  6 ++--
 src/lock.c          | 26 ++++++++++------
 src/lock.h          |  5 +--
 src/syncrepl.c      | 89 +++++++++++++++++++++++++++++++++++++++++++++++------
 src/zone_register.c |  3 +-
 6 files changed, 140 insertions(+), 46 deletions(-)

diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 89d585abe1cda14fb23807e612970e9bd1e84459..3610cb373e525017eefb168e81b3766f934cdecc 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -1099,7 +1099,7 @@ publish_zone(isc_task_t *task, ldap_instance_t *inst, dns_zone_t *zone)
 	} /* else if (zone_in_view == NULL && view_in_zone == NULL)
 	     Publish the zone. */
 
-	run_exclusive_enter(task, &lock_state);
+	run_exclusive_enter(inst, &lock_state);
 	if (inst->view->frozen) {
 		freeze = ISC_TRUE;
 		dns_view_thaw(inst->view);
@@ -1113,7 +1113,7 @@ cleanup:
 		dns_zone_detach(&zone_in_view);
 	if (freeze)
 		dns_view_freeze(inst->view);
-	run_exclusive_exit(task, lock_state);
+	run_exclusive_exit(inst, lock_state);
 
 	return result;
 }
@@ -1292,8 +1292,7 @@ delete_forwarding_table(ldap_instance_t *inst, dns_name_t *name,
 
 /* Delete zone by dns zone name */
 isc_result_t
-ldap_delete_zone2(ldap_instance_t *inst, isc_task_t * const task,
-		  dns_name_t *name, isc_boolean_t lock,
+ldap_delete_zone2(ldap_instance_t *inst, dns_name_t *name, isc_boolean_t lock,
 		  isc_boolean_t preserve_forwarding)
 {
 	isc_result_t result;
@@ -1308,7 +1307,7 @@ ldap_delete_zone2(ldap_instance_t *inst, isc_task_t * const task,
 	dns_name_format(name, zone_name_char, DNS_NAME_FORMATSIZE);
 	log_debug(1, "deleting zone '%s'", zone_name_char);
 	if (lock)
-		run_exclusive_enter(task, &lock_state);
+		run_exclusive_enter(inst, &lock_state);
 
 	if (!preserve_forwarding) {
 		CHECK(delete_forwarding_table(inst, name, "zone",
@@ -1351,25 +1350,25 @@ ldap_delete_zone2(ldap_instance_t *inst, isc_task_t * const task,
 cleanup:
 	if (freeze)
 		dns_view_freeze(inst->view);
-	run_exclusive_exit(task, lock_state);
+	run_exclusive_exit(inst, lock_state);
 
 	return result;
 }
 
 /* Delete zone */
-static isc_result_t ATTR_NONNULL(1,3) ATTR_CHECKRESULT
-ldap_delete_zone(ldap_instance_t *inst, isc_task_t * const task, const char *dn,
-		 isc_boolean_t lock, isc_boolean_t preserve_forwarding)
+static isc_result_t ATTR_NONNULLS ATTR_CHECKRESULT
+ldap_delete_zone(ldap_instance_t *inst, const char *dn, isc_boolean_t lock,
+		 isc_boolean_t preserve_forwarding)
 {
 	isc_result_t result;
 	isc_boolean_t iszone;
 	dns_name_t name;
 	dns_name_init(&name, NULL);
 	
 	CHECK(dn_to_dnsname(inst->mctx, dn, &name, NULL, &iszone));
 	INSIST(iszone == ISC_TRUE);
 
-	result = ldap_delete_zone2(inst, task, &name, lock, preserve_forwarding);
+	result = ldap_delete_zone2(inst, &name, lock, preserve_forwarding);
 
 cleanup:
 	if (dns_name_dynamic(&name))
@@ -1405,6 +1404,7 @@ configure_zone_forwarders(ldap_entry_t *entry, ldap_instance_t *inst,
 	const char *dn = entry->dn;
 	isc_result_t result;
 	isc_result_t orig_result;
+	isc_result_t lock_state = ISC_R_IGNORE;
 	ldap_valuelist_t values;
 	ldap_value_t *value;
 	isc_sockaddrlist_t addrs;
@@ -1601,7 +1601,9 @@ cleanup:
 		log_debug(5, "%s '%s': forwarder table was updated: %s",
 			  msg_obj_type, dn, dns_result_totext(result));
 		orig_result = result;
+		run_exclusive_enter(inst, &lock_state);
 		result = dns_view_flushcache(inst->view);
+		run_exclusive_exit(inst, lock_state);
 		if (result == ISC_R_SUCCESS)
 			result = orig_result;
 	}
@@ -2235,12 +2237,12 @@ zone_security_change(ldap_entry_t * const entry, dns_name_t * const name,
 	/* Lock is necessary to ensure that no events from LDAP are lost
 	 * in period where old zone was deleted but the new zone was not
 	 * created yet. */
-	run_exclusive_enter(task, &lock_state);
-	CHECK(ldap_delete_zone2(inst, task, name, ISC_FALSE, ISC_TRUE));
+	run_exclusive_enter(inst, &lock_state);
+	CHECK(ldap_delete_zone2(inst, name, ISC_FALSE, ISC_TRUE));
 	CHECK(ldap_parse_master_zoneentry(entry, olddb, inst, task));
 
 cleanup:
-	run_exclusive_exit(task, lock_state);
+	run_exclusive_exit(inst, lock_state);
 	if (olddb != NULL)
 		dns_db_detach(&olddb);
 	return result;
@@ -2293,16 +2295,17 @@ ldap_parse_master_zoneentry(ldap_entry_t * const entry, dns_db_t * const olddb,
 
 	REQUIRE(entry != NULL);
 	REQUIRE(inst != NULL);
+	REQUIRE(task == inst->task); /* For task-exclusive mode */
 
 	dns_diff_init(inst->mctx, &diff);
 	dns_name_init(&name, NULL);
 
 	/* Derive the dns name of the zone from the DN. */
 	dn = entry->dn;
 	CHECK(dn_to_dnsname(inst->mctx, dn, &name, NULL, &iszone));
 	INSIST(iszone == ISC_TRUE);
 
-	run_exclusive_enter(task, &lock_state);
+	run_exclusive_enter(inst, &lock_state);
 
 	result = configure_zone_forwarders(entry, inst, &name);
 	if (result != ISC_R_SUCCESS && result != ISC_R_DISABLED)
@@ -2406,12 +2409,11 @@ cleanup:
 		/* Failure in ACL parsing or so. */
 		log_error_r("zone '%s': publishing failed, rolling back due to",
 			    entry->dn);
-		result = ldap_delete_zone2(inst, task, &name, ISC_TRUE,
-					   ISC_FALSE);
+		result = ldap_delete_zone2(inst, &name, ISC_TRUE, ISC_FALSE);
 		if (result != ISC_R_SUCCESS)
 			log_error_r("zone '%s': rollback failed: ", entry->dn);
 	}
-	run_exclusive_exit(task, lock_state);
+	run_exclusive_exit(inst, lock_state);
 	if (dns_name_dynamic(&name))
 		dns_name_free(&name, inst->mctx);
 	if (raw != NULL)
@@ -4209,6 +4211,7 @@ update_zone(isc_task_t *task, isc_event_t *event)
 	dns_name_init(&prevname, NULL);
 
 	CHECK(manager_get_ldap_instance(pevent->dbname, &inst));
+	INSIST(task == inst->task); /* For task-exclusive mode */
 	CHECK(dn_to_dnsname(inst->mctx, pevent->dn, &currname, NULL, &iszone));
 	INSIST(iszone == ISC_TRUE);
 
@@ -4254,7 +4257,7 @@ update_zone(isc_task_t *task, isc_event_t *event)
 		}
 		*/
 	} else {
-		CHECK(ldap_delete_zone(inst, task, pevent->dn, ISC_TRUE, ISC_FALSE));
+		CHECK(ldap_delete_zone(inst, pevent->dn, ISC_TRUE, ISC_FALSE));
 	}
 
 cleanup:
@@ -4289,11 +4292,10 @@ update_config(isc_task_t * task, isc_event_t *event)
 	ldap_entry_t *entry = pevent->entry;
 	isc_mem_t *mctx;
 
-	UNUSED(task);
-
 	mctx = pevent->mctx;
 
 	CHECK(manager_get_ldap_instance(pevent->dbname, &inst));
+	INSIST(task == inst->task); /* For task-exclusive mode */
 	CHECK(ldap_parse_configentry(entry, inst));
 
 cleanup:
@@ -4681,7 +4683,11 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t *entry, int chgtype)
 	else
 		zone_name = &entry_origin;
 
-	if ((class & (LDAP_ENTRYCLASS_MASTER | LDAP_ENTRYCLASS_RR)) != 0) {
+	/* Process ordinary records in parallel but serialize operations on
+	 * master zone objects.
+	 * See discussion about run_exclusive_begin() function in lock.c. */
+	if ((class & LDAP_ENTRYCLASS_RR) != 0 &&
+	    (class & LDAP_ENTRYCLASS_MASTER) == 0) {
 		result = zr_get_zone_ptr(inst->zone_register, zone_name,
 					 &zone_ptr, NULL);
 		if (result == ISC_R_SUCCESS && dns_zone_getmgr(zone_ptr) != NULL)
@@ -4694,6 +4700,8 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t *entry, int chgtype)
 			result = ISC_R_SUCCESS;
 		}
 	} else {
+		/* For configuration object and zone object use single task
+		 * to make sure that the exclusive mode actually works. */
 		isc_task_attach(inst->task, &task);
 	}
 	REQUIRE(task != NULL);
@@ -4722,6 +4730,7 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t *entry, int chgtype)
 	/* All events for single zone are handled by one task, so we don't
 	 * need to spend time with normal records. */
 	if (action == update_zone || action == update_config) {
+		INSIST(task == inst->task); /* For task-exclusive mode */
 		sync_state_get(inst->sctx, &sync_state);
 		if (sync_state == sync_init)
 			CHECK(sync_task_add(inst->sctx, task));
@@ -5132,3 +5141,9 @@ ldap_instance_getzr(ldap_instance_t *ldap_inst)
 {
 	return ldap_inst->zone_register;
 }
+
+isc_task_t *
+ldap_instance_gettask(ldap_instance_t *ldap_inst)
+{
+	return ldap_inst->task;
+}
diff --git a/src/ldap_helper.h b/src/ldap_helper.h
index 76e808ef60a9aac2b28ab741b629a37b581fdc62..304ab9d87c31e169d6668dc3ae4b13464a1a66ae 100644
--- a/src/ldap_helper.h
+++ b/src/ldap_helper.h
@@ -81,9 +81,9 @@ new_ldap_instance(isc_mem_t *mctx, const char *db_name,
 void destroy_ldap_instance(ldap_instance_t **ldap_inst) ATTR_NONNULLS;
 
 isc_result_t
-ldap_delete_zone2(ldap_instance_t *inst, isc_task_t *task, dns_name_t *name,
+ldap_delete_zone2(ldap_instance_t *inst, dns_name_t *name,
 		  isc_boolean_t lock, isc_boolean_t preserve_forwarding)
-		  ATTR_NONNULL(1,3);
+		  ATTR_NONNULLS;
 
 /* Functions for writing to LDAP. */
 isc_result_t write_to_ldap(dns_name_t *owner, ldap_instance_t *ldap_inst,
@@ -106,4 +106,6 @@ zone_register_t * ldap_instance_getzr(ldap_instance_t *ldap_inst) ATTR_NONNULLS;
 
 isc_result_t activate_zones(isc_task_t *task, ldap_instance_t *inst) ATTR_NONNULLS;
 
+isc_task_t * ldap_instance_gettask(ldap_instance_t *ldap_inst);
+
 #endif /* !_LD_LDAP_HELPER_H_ */
diff --git a/src/lock.c b/src/lock.c
index 7099dd36808f1993e0fce666163ddc9ad1b8e1d3..b6b2e9fe828bee54f7c2637eafdb9b15080ba663 100644
--- a/src/lock.c
+++ b/src/lock.c
@@ -24,40 +24,46 @@
 #include "lock.h"
 
 /**
- * Lock BIND dispatcher and allow only single task to run. This function
- * blocks until task-exclusive mode is entered.
+ * Lock BIND dispatcher and allow only single task to run.
+ *
+ * @warning
+ * All calls to isc_task_beginexclusive() have to operate on the same task
+ * otherwise it would not be possible to distinguish recursive locking
+ * from real conflict on the dispatcher lock.
+ * For this reason this wrapper function always works with inst->task.
+ * As a result, this function have to be be called only from inst->task.
  *
  * Recursive locking is allowed. Auxiliary variable pointed to by "statep"
  * stores information if last run_exclusive_enter() operation really locked
  * something or if the lock was called recursively and was no-op.
  *
- * The pair (task, state) used for run_exclusive_enter() has to be
+ * The pair (inst, state) used for run_exclusive_enter() has to be
  * used for run_exclusive_exit().
  *
- * @param[in]  	  task   The only task allowed to run.
+ * @param[in]  	  inst   The instance with the only task which is allowed to run.
  * @param[in,out] statep Lock state: ISC_R_SUCCESS or ISC_R_LOCKBUSY
  */
 void
-run_exclusive_enter(isc_task_t *task, isc_result_t *statep)
+run_exclusive_enter(ldap_instance_t *inst, isc_result_t *statep)
 {
 	REQUIRE(statep != NULL);
 	REQUIRE(*statep == ISC_R_IGNORE);
 
-	*statep = isc_task_beginexclusive(task);
+	*statep = isc_task_beginexclusive(ldap_instance_gettask(inst));
 	RUNTIME_CHECK(*statep == ISC_R_SUCCESS || *statep == ISC_R_LOCKBUSY);
 }
 
 /**
  * Exit task-exclusive mode.
  *
- * @param task[in]  The only task allowed to run at the moment.
- * @param state[in] Lock state as returned by run_exclusive_enter().
+ * @param[in] inst  The instance used for previous run_exclusive_enter() call.
+ * @param[in] state Lock state as returned by run_exclusive_enter().
  */
 void
-run_exclusive_exit(isc_task_t *task, isc_result_t state)
+run_exclusive_exit(ldap_instance_t *inst, isc_result_t state)
 {
 	if (state == ISC_R_SUCCESS)
-		isc_task_endexclusive(task);
+		isc_task_endexclusive(ldap_instance_gettask(inst));
 	else
 		/* Unlocking recursive lock or the lock was never locked. */
 		INSIST(state == ISC_R_LOCKBUSY || state == ISC_R_IGNORE);
diff --git a/src/lock.h b/src/lock.h
index 38ce8bf7b41b4205b24e1943f17b0f46db4d9db3..ed271646703115126dbea184a406fd0a6405ccdc 100644
--- a/src/lock.h
+++ b/src/lock.h
@@ -21,12 +21,13 @@
 #ifndef LOCK_H_
 #define LOCK_H_
 
+#include "ldap_helper.h"
 #include "util.h"
 
 void ATTR_NONNULLS
-run_exclusive_enter(isc_task_t *task, isc_result_t *statep);
+run_exclusive_enter(ldap_instance_t *inst, isc_result_t *statep);
 
 void ATTR_NONNULLS
-run_exclusive_exit(isc_task_t *task, isc_result_t state);
+run_exclusive_exit(ldap_instance_t *inst, isc_result_t state);
 
 #endif /* LOCK_H_ */
diff --git a/src/syncrepl.c b/src/syncrepl.c
index c6b326bab9733f7cb5065381d26e999ddbb570db..0d916fe8396c574807fc7258a8d390bda6009a09 100644
--- a/src/syncrepl.c
+++ b/src/syncrepl.c
@@ -33,6 +33,7 @@
 #include "zone_manager.h"
 
 #define LDAPDB_EVENT_SYNCREPL_BARRIER	(LDAPDB_EVENTCLASS + 2)
+#define LDAPDB_EVENT_SYNCREPL_FINISH	(LDAPDB_EVENTCLASS + 3)
 
 /** How many unprocessed LDAP events from syncrepl can be in event queue.
  *  Adding new events into the queue is blocked until some events
@@ -85,16 +86,18 @@ struct sync_ctx {
 	isc_mutex_t			mutex;	/**< guards rest of the structure */
 	isc_condition_t			cond;	/**< for signal when task_cnt == 0 */
 	sync_state_t			state;
+	isc_task_t			*excl_task; /**< task used for transition
+							 barrier->finished */
 	ISC_LIST(task_element_t)	tasks;	/**< list of tasks processing
 						     events from initial
 						     synchronization phase */
 };
 
 /**
  * @brief This event is used to separate event queue for particular task to
  * part 'before' and 'after' this event.
  *
- * This is auxiliary event supporting sync_barrier_wait().
+ * This is an auxiliary event supporting sync_barrier_wait().
  *
  * @todo Solution with inst_name is not very clever. Reference counting would
  *       be much better, but ldap_instance_t doesn't support reference counting.
@@ -106,21 +109,85 @@ struct sync_barrierev {
 };
 
 /**
- * @brief Event handler for 'sync barrier event'.
+ * @brief Event handler for 'sync barrier event' - part 2.
+ *
+ * This is auxiliary event handler for zone loading and publishing.
+ * See also barrier_decrement().
+ */
+void
+finish(isc_task_t *task, isc_event_t *event) {
+	isc_result_t result = ISC_R_SUCCESS;
+	ldap_instance_t *inst = NULL;
+	sync_barrierev_t *bev = NULL;
+
+	REQUIRE(ISCAPI_TASK_VALID(task));
+	REQUIRE(event != NULL);
+
+	bev = (sync_barrierev_t *)event;
+	CHECK(manager_get_ldap_instance(bev->dbname, &inst));
+	log_debug(1, "sync_barrier_wait(): finish reached");
+	LOCK(&bev->sctx->mutex);
+	REQUIRE(bev->sctx->state == sync_barrier);
+	bev->sctx->state = sync_finished;
+	isc_condition_broadcast(&bev->sctx->cond);
+	UNLOCK(&bev->sctx->mutex);
+	activate_zones(task, inst);
+
+cleanup:
+	if (result != ISC_R_SUCCESS)
+		log_error_r("syncrepl finish() failed");
+	isc_event_free(&event);
+	return;
+}
+
+static isc_result_t ATTR_NONNULLS ATTR_CHECKRESULT
+sync_finishev_create(sync_ctx_t *sctx, const char *inst_name,
+		      sync_barrierev_t **evp) {
+	sync_barrierev_t *ev = NULL;
+
+	REQUIRE(sctx != NULL);
+	REQUIRE(inst_name != NULL);
+	REQUIRE(evp != NULL && *evp == NULL);
+
+	ev = (sync_barrierev_t *)isc_event_allocate(sctx->mctx,
+				sctx, LDAPDB_EVENT_SYNCREPL_BARRIER,
+				finish, NULL,
+				sizeof(sync_barrierev_t));
+	if (ev == NULL)
+		return ISC_R_NOMEMORY;
+
+	ev->dbname = inst_name;
+	ev->sctx = sctx;
+	*evp = ev;
+
+	return ISC_R_SUCCESS;
+}
+
+/**
+ * @brief Event handler for 'sync barrier event' - part 1.
  *
  * This is auxiliary event handler for events created by
  * sync_barrierev_create() and sent by sync_barrier_wait().
  *
  * Each call decrements task_cnt counter in synchronization context associated
  * with the particular event. Broadcast will be send to condition in associated
  * synchronization context when task_cnt == 0.
+ *
+ * Secondly, "finish" event will be generated and sent to sctx->excl_task, i.e.
+ * to inst->task when task_cnt == 0.
+ * This split is necessary because we have to make sure that DNS view
+ * manipulation during zone loading is done only from inst->task
+ * (see run_exclusive_enter() comments).
  */
 void
 barrier_decrement(isc_task_t *task, isc_event_t *event) {
 	isc_result_t result = ISC_R_SUCCESS;
 	ldap_instance_t *inst = NULL;
 	sync_barrierev_t *bev = NULL;
+	sync_barrierev_t *fev = NULL;
+	isc_event_t *ev = NULL;
 	isc_uint32_t cnt;
+	isc_boolean_t locked = ISC_FALSE;
 
 	REQUIRE(ISCAPI_TASK_VALID(task));
 	REQUIRE(event != NULL);
@@ -131,14 +198,15 @@ barrier_decrement(isc_task_t *task, isc_event_t *event) {
 	if (cnt == 0) {
 		log_debug(1, "sync_barrier_wait(): barrier reached");
 		LOCK(&bev->sctx->mutex);
-		REQUIRE(bev->sctx->state == sync_barrier);
-		bev->sctx->state = sync_finished;
-		isc_condition_broadcast(&bev->sctx->cond);
+		locked = ISC_TRUE;
+		CHECK(sync_finishev_create(bev->sctx, bev->dbname, &fev));
+		ev = (isc_event_t *)fev;
+		isc_task_send(bev->sctx->excl_task, &ev);
+	}
+
+cleanup:
+	if (locked)
 		UNLOCK(&bev->sctx->mutex);
-		activate_zones(task, inst);
-	}
-
-cleanup:
 	if (result != ISC_R_SUCCESS)
 		log_error_r("barrier_decrement() failed");
 	isc_event_free(&event);
@@ -203,6 +271,8 @@ sync_ctx_init(isc_mem_t *mctx, isc_task_t *task, sync_ctx_t **sctxp) {
 	CHECK(isc_refcount_init(&sctx->task_cnt, 0));
 	refcount_ready = ISC_TRUE;
 
+	isc_task_attach(task, &sctx->excl_task);
+
 	ISC_LIST_INIT(sctx->tasks);
 
 	sctx->state = sync_init;
@@ -251,6 +321,7 @@ sync_ctx_free(sync_ctx_t **sctxp) {
 	}
 	isc_condition_destroy(&sctx->cond);
 	isc_refcount_destroy(&sctx->task_cnt);
+	isc_task_detach(&sctx->excl_task);
 	UNLOCK(&sctx->mutex);
 
 	isc_mutex_destroy(&(*sctxp)->mutex);
diff --git a/src/zone_register.c b/src/zone_register.c
index e33be86bb6d6aedd13267af439cafa106089fbc7..c949c373750d76976817f3376b142923ed1e0d56 100644
--- a/src/zone_register.c
+++ b/src/zone_register.c
@@ -173,8 +173,7 @@ zr_destroy(zone_register_t **zrp)
 		if (result == ISC_R_SUCCESS) {
 			rbt_iter_stop(&iter);
 			result = ldap_delete_zone2(zr->ldap_inst,
-						   NULL, &name,
-						   ISC_FALSE, ISC_FALSE);
+						   &name, ISC_FALSE, ISC_FALSE);
 			RUNTIME_CHECK(result == ISC_R_SUCCESS);
 		}
 	} while (result == ISC_R_SUCCESS);
-- 
1.9.3

From ac3a4b4a5fa8dbb57c470b297a6d16ad94992ac3 Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Wed, 10 Sep 2014 20:07:29 +0200
Subject: [PATCH] Lock syncrepl queue avoid race between zones and resource
 records.

Signed-off-by: Petr Spacek <pspa...@redhat.com>
---
 src/ldap_helper.c | 12 +++++++++++-
 src/syncrepl.c    | 31 +++++++++++++++++++++++++++++++
 src/syncrepl.h    |  6 ++++++
 3 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 3610cb373e525017eefb168e81b3766f934cdecc..b45b368f9715ed33cf323d2e6f4f026127e23722 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -4263,6 +4263,7 @@ update_zone(isc_task_t *task, isc_event_t *event)
 cleanup:
 	if (inst != NULL) {
 		sync_concurr_limit_signal(inst->sctx);
+		sync_event_signal(inst->sctx, event);
 		if (dns_name_dynamic(&currname))
 			dns_name_free(&currname, inst->mctx);
 		if (dns_name_dynamic(&prevname))
@@ -4299,8 +4300,10 @@ update_config(isc_task_t * task, isc_event_t *event)
 	CHECK(ldap_parse_configentry(entry, inst));
 
 cleanup:
-	if (inst != NULL)
+	if (inst != NULL) {
 		sync_concurr_limit_signal(inst->sctx);
+		sync_event_signal(inst->sctx, event);
+	}
 	if (result != ISC_R_SUCCESS)
 		log_error_r("update_config (syncrepl) failed for '%s'. "
 			    "Configuration can be outdated, run `rndc reload`",
@@ -4621,6 +4624,7 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t *entry, int chgtype)
 	ldap_entryclass_t class = LDAP_ENTRYCLASS_NONE;
 	isc_result_t result = ISC_R_SUCCESS;
 	ldap_syncreplevent_t *pevent = NULL;
+	isc_event_t *wait_event = NULL;
 	dns_name_t entry_name;
 	dns_name_t entry_origin;
 	dns_name_t *zone_name = NULL;
@@ -4752,8 +4756,14 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t *entry, int chgtype)
 	pevent->prevdn = NULL;
 	pevent->chgtype = chgtype;
 	pevent->entry = entry;
+	wait_event = (isc_event_t *)pevent;
 	isc_task_send(task, (isc_event_t **)&pevent);
 
+	/* Lock syncrepl queue to prevent zone, config and resource records
+	 * from racing with each other. */
+	if (action == update_zone || action == update_config)
+		sync_event_wait(inst->sctx, wait_event);
+
 cleanup:
 	if (dns_name_dynamic(&entry_name))
 		dns_name_free(&entry_name, inst->mctx);
diff --git a/src/syncrepl.c b/src/syncrepl.c
index 0d916fe8396c574807fc7258a8d390bda6009a09..ba5cee19436a774da8c7594d8b4d3ec813d3a8cd 100644
--- a/src/syncrepl.c
+++ b/src/syncrepl.c
@@ -88,6 +88,7 @@ struct sync_ctx {
 	sync_state_t			state;
 	isc_task_t			*excl_task; /**< task used for transition
 							 barrier->finished */
+	isc_event_t			*last_ev; /**< Last processed event */
 	ISC_LIST(task_element_t)	tasks;	/**< list of tasks processing
 						     events from initial
 						     synchronization phase */
@@ -457,3 +458,33 @@ sync_concurr_limit_signal(sync_ctx_t *sctx) {
 
 	semaphore_signal(&sctx->concurr_limit);
 }
+
+/**
+ * Wait until given event ev is processed.
+ *
+ * End of event processing has to be signalled by
+ * sync_event_signal() call.
+ */
+void
+sync_event_wait(sync_ctx_t *sctx, isc_event_t *ev) {
+	REQUIRE(sctx != NULL);
+
+	LOCK(&sctx->mutex);
+	while (sctx->last_ev != ev)
+		WAIT(&sctx->cond, &sctx->mutex);
+	UNLOCK(&sctx->mutex);
+}
+
+/**
+ * Signal that given syncrepl event was processed.
+ */
+void
+sync_event_signal(sync_ctx_t *sctx, isc_event_t *ev) {
+	REQUIRE(sctx != NULL);
+	REQUIRE(ev != NULL);
+
+	LOCK(&sctx->mutex);
+	sctx->last_ev = ev;
+	BROADCAST(&sctx->cond);
+	UNLOCK(&sctx->mutex);
+}
diff --git a/src/syncrepl.h b/src/syncrepl.h
index 29fd29597628abec5b7cbaa75dfbec234a5d2d51..31d9d7f6e1725039a1743560aba2b243da91ab3c 100644
--- a/src/syncrepl.h
+++ b/src/syncrepl.h
@@ -66,4 +66,10 @@ sync_concurr_limit_wait(sync_ctx_t *sctx) ATTR_NONNULLS;
 void ATTR_NONNULLS
 sync_concurr_limit_signal(sync_ctx_t *sctx) ATTR_NONNULLS;
 
+void
+sync_event_wait(sync_ctx_t *sctx, isc_event_t *ev) ATTR_NONNULLS;
+
+void
+sync_event_signal(sync_ctx_t *sctx, isc_event_t *ev) ATTR_NONNULLS;
+
 #endif /* SYNCREPL_H_ */
-- 
1.9.3

From 4b81f913d5975c137af55d1d50d757066a160e3e Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Thu, 11 Sep 2014 11:17:47 +0200
Subject: [PATCH] Fix deadlocks on shutdown caused by sync_*_wait() calls.

sync_event_wait() and sync_concurr_limit_wait() calls were updated to
check inst->exiting variable. This is necessary to prevent deadlocks
on shutdown, e.g. in situation where syncrepl queue is full
(sync_concurr_limit_wait() is blocking) and BIND receives SIGINT.

Both synchronization calls now fail with ISC_R_SHUTTINGDOWN
if LDAP instance is marked as "exiting".

Signed-off-by: Petr Spacek <pspa...@redhat.com>
---
 src/ldap_helper.c | 22 ++++++++++++------
 src/ldap_helper.h |  2 ++
 src/semaphore.c   |  9 ++++----
 src/semaphore.h   |  6 +++--
 src/syncrepl.c    | 68 ++++++++++++++++++++++++++++++++++++++++++-------------
 src/syncrepl.h    | 12 +++++-----
 6 files changed, 84 insertions(+), 35 deletions(-)

diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index b45b368f9715ed33cf323d2e6f4f026127e23722..1eb8a23501d150cc930e01d1447150b6da3a60f7 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -404,8 +404,8 @@ validate_local_instance_settings(ldap_instance_t *inst, settings_set_t *set) {
 
 	/* Set timer for deadlock detection inside semaphore_wait_timed . */
 	CHECK(setting_get_uint("timeout", set, &uint));
-	if (semaphore_wait_timeout.seconds < uint*SEM_WAIT_TIMEOUT_MUL)
-		semaphore_wait_timeout.seconds = uint*SEM_WAIT_TIMEOUT_MUL;
+	if (conn_wait_timeout.seconds < uint*SEM_WAIT_TIMEOUT_MUL)
+		conn_wait_timeout.seconds = uint*SEM_WAIT_TIMEOUT_MUL;
 
 	CHECK(setting_get_uint("connections", set, &uint));
 	if (uint < 2) {
@@ -540,7 +540,7 @@ new_ldap_instance(isc_mem_t *mctx, const char *db_name,
 	ISC_LIST_INIT(ldap_inst->orig_global_forwarders.addrs);
 	ldap_inst->task = task;
 	ldap_inst->watcher = 0;
-	CHECK(sync_ctx_init(ldap_inst->mctx, task, &ldap_inst->sctx));
+	CHECK(sync_ctx_init(ldap_inst->mctx, ldap_inst, &ldap_inst->sctx));
 
 	isc_string_printf_truncate(settings_name, PRINT_BUFF_SIZE,
 				   SETTING_SET_NAME_LOCAL " for database %s",
@@ -4104,7 +4104,7 @@ ldap_pool_getconnection(ldap_pool_t *pool, ldap_connection_t ** conn)
 	REQUIRE(conn != NULL && *conn == NULL);
 	ldap_conn = *conn;
 
-	CHECK(semaphore_wait_timed(&pool->conn_semaphore));
+	CHECK(semaphore_wait_timed(&pool->conn_semaphore, &conn_wait_timeout));
 	/* Following assertion is necessary to convince clang static analyzer
 	 * that the loop is always entered. */
 	REQUIRE(pool->connections > 0);
@@ -4762,18 +4762,20 @@ syncrepl_update(ldap_instance_t *inst, ldap_entry_t *entry, int chgtype)
 	/* Lock syncrepl queue to prevent zone, config and resource records
 	 * from racing with each other. */
 	if (action == update_zone || action == update_config)
-		sync_event_wait(inst->sctx, wait_event);
+		CHECK(sync_event_wait(inst->sctx, wait_event));
 
 cleanup:
 	if (dns_name_dynamic(&entry_name))
 		dns_name_free(&entry_name, inst->mctx);
 	if (dns_name_dynamic(&entry_origin))
 		dns_name_free(&entry_origin, inst->mctx);
 	if (zone_ptr != NULL)
 		dns_zone_detach(&zone_ptr);
-	if (result != ISC_R_SUCCESS) {
+	if (result != ISC_R_SUCCESS)
 		log_error_r("syncrepl_update failed for object '%s'",
 			    entry->dn);
+	if (wait_event == NULL) {
+		/* Event was not sent */
 		sync_concurr_limit_signal(inst->sctx);
 
 		if (dbname != NULL)
@@ -4885,7 +4887,7 @@ int ldap_sync_search_entry (
 	if (inst->exiting)
 		return LDAP_SUCCESS;
 
-	sync_concurr_limit_wait(inst->sctx);
+	CHECK(sync_concurr_limit_wait(inst->sctx));
 	CHECK(ldap_entry_create(inst->mctx, ls->ls_ld, msg, &entry));
 	syncrepl_update(inst, entry, phase);
 #ifdef RBTDB_DEBUG
@@ -5157,3 +5159,9 @@ ldap_instance_gettask(ldap_instance_t *ldap_inst)
 {
 	return ldap_inst->task;
 }
+
+isc_boolean_t
+ldap_instance_isexiting(ldap_instance_t *ldap_inst)
+{
+	return ldap_inst->exiting;
+}
diff --git a/src/ldap_helper.h b/src/ldap_helper.h
index 304ab9d87c31e169d6668dc3ae4b13464a1a66ae..e6c77b9b2eb07054976c4911f7a4bd649c72d83b 100644
--- a/src/ldap_helper.h
+++ b/src/ldap_helper.h
@@ -108,4 +108,6 @@ isc_result_t activate_zones(isc_task_t *task, ldap_instance_t *inst) ATTR_NONNUL
 
 isc_task_t * ldap_instance_gettask(ldap_instance_t *ldap_inst);
 
+isc_boolean_t ldap_instance_isexiting(ldap_instance_t *ldap_inst) ATTR_NONNULLS ATTR_CHECKRESULT;
+
 #endif /* !_LD_LDAP_HELPER_H_ */
diff --git a/src/semaphore.c b/src/semaphore.c
index b6ad7bf090467c29d749d7165e7122d0075131d7..cda271d83840386e7b61a872e7ca7401011b5f6a 100644
--- a/src/semaphore.c
+++ b/src/semaphore.c
@@ -39,7 +39,7 @@
  *
  * Initial value can be useful in early phases of initialization.
  */
-isc_interval_t semaphore_wait_timeout = { 3, 0 };
+isc_interval_t conn_wait_timeout = { 3, 0 };
 
 /*
  * Initialize a semaphore.
@@ -103,18 +103,19 @@ semaphore_wait(semaphore_t *sem)
 /**
  * Wait on semaphore. This operation will try to acquire a lock on the
  * semaphore. If the semaphore is already acquired as many times at it allows,
- * the function will block until someone releases the lock OR timeout expire.
+ * the function will block until someone releases the lock OR timeout expires.
  *
  * @return ISC_R_SUCCESS or ISC_R_TIMEDOUT or other errors from ISC libs
  */
 isc_result_t
-semaphore_wait_timed(semaphore_t *sem)
+semaphore_wait_timed(semaphore_t *const sem,
+		     const isc_interval_t * const timeout)
 {
 	isc_result_t result;
 	isc_time_t abs_timeout;
 	REQUIRE(sem != NULL);
 
-	CHECK(isc_time_nowplusinterval(&abs_timeout, &semaphore_wait_timeout));
+	CHECK(isc_time_nowplusinterval(&abs_timeout, timeout));
 	LOCK(&sem->mutex);
 
 	while (sem->value <= 0)
diff --git a/src/semaphore.h b/src/semaphore.h
index bd9e710864d8ce9ee72acd8137a7303f8d737a8e..a792d3f574ed2123ae59ffd24c7e560336e5fcea 100644
--- a/src/semaphore.h
+++ b/src/semaphore.h
@@ -28,7 +28,7 @@
 
 /* Multiplier for to user-defined connection parameter 'timeout'. */
 #define SEM_WAIT_TIMEOUT_MUL 6 /* times */
-extern isc_interval_t semaphore_wait_timeout;
+extern isc_interval_t conn_wait_timeout;
 
 /*
  * Semaphore can be "acquired" multiple times. However, it has a maximum
@@ -47,7 +47,9 @@ typedef struct semaphore	semaphore_t;
 isc_result_t	semaphore_init(semaphore_t *sem, int value) ATTR_NONNULLS ATTR_CHECKRESULT;
 void		semaphore_destroy(semaphore_t *sem) ATTR_NONNULLS;
 void		semaphore_wait(semaphore_t *sem) ATTR_NONNULLS;
-isc_result_t	semaphore_wait_timed(semaphore_t *sem) ATTR_NONNULLS ATTR_CHECKRESULT;
+isc_result_t	semaphore_wait_timed(semaphore_t *sem,
+				     const isc_interval_t * const timeout)
+				     ATTR_NONNULLS ATTR_CHECKRESULT;
 void		semaphore_signal(semaphore_t *sem) ATTR_NONNULLS;
 
 #endif /* !_LD_SEMAPHORE_H_ */
diff --git a/src/syncrepl.c b/src/syncrepl.c
index ba5cee19436a774da8c7594d8b4d3ec813d3a8cd..098f9ffee234f98159c203ccfc78ee6009423c33 100644
--- a/src/syncrepl.c
+++ b/src/syncrepl.c
@@ -24,6 +24,7 @@
 #include <isc/event.h>
 #include <isc/mutex.h>
 #include <isc/task.h>
+#include <isc/time.h>
 #include <isc/util.h>
 
 #include "ldap_helper.h"
@@ -46,6 +47,13 @@ struct task_element {
 	ISC_LINK(task_element_t)	link;
 };
 
+/** Timeout for thread synchronization. Conditions are re-checked every three
+ * seconds to see if inst->exiting is true or not.
+ *
+ * The expectation is that no event will wait in queue for three seconds so
+ * polling will happen once only and only during BIND shutdown. */
+static const isc_interval_t shutdown_timeout = { 3, 0 };
+
 /**
  * @brief Synchronisation context.
  *
@@ -86,8 +94,7 @@ struct sync_ctx {
 	isc_mutex_t			mutex;	/**< guards rest of the structure */
 	isc_condition_t			cond;	/**< for signal when task_cnt == 0 */
 	sync_state_t			state;
-	isc_task_t			*excl_task; /**< task used for transition
-							 barrier->finished */
+	ldap_instance_t			*inst;
 	isc_event_t			*last_ev; /**< Last processed event */
 	ISC_LIST(task_element_t)	tasks;	/**< list of tasks processing
 						     events from initial
@@ -202,7 +209,7 @@ barrier_decrement(isc_task_t *task, isc_event_t *event) {
 		locked = ISC_TRUE;
 		CHECK(sync_finishev_create(bev->sctx, bev->dbname, &fev));
 		ev = (isc_event_t *)fev;
-		isc_task_send(bev->sctx->excl_task, &ev);
+		isc_task_send(ldap_instance_gettask(bev->sctx->inst), &ev);
 	}
 
 cleanup:
@@ -240,44 +247,42 @@ sync_barrierev_create(sync_ctx_t *sctx, const char *inst_name,
 /**
  * Initialize synchronization context.
  *
- * @param[in]	task	Task used for first synchronization events.
- * 			Typically the ldap_inst->task.
+ * @param[in]	inst	LDAP instance associated with this synchronization ctx.
  * @param[out]	sctxp	The new synchronization context.
  *
  * @post state == sync_init
  * @post task_cnt == 1
  * @post tasks list contains the task
  */
 isc_result_t
-sync_ctx_init(isc_mem_t *mctx, isc_task_t *task, sync_ctx_t **sctxp) {
+sync_ctx_init(isc_mem_t *mctx, ldap_instance_t *inst, sync_ctx_t **sctxp) {
 	isc_result_t result;
 	sync_ctx_t *sctx = NULL;
 	isc_boolean_t lock_ready = ISC_FALSE;
 	isc_boolean_t cond_ready = ISC_FALSE;
 	isc_boolean_t refcount_ready = ISC_FALSE;
 
 	REQUIRE(sctxp != NULL && *sctxp == NULL);
-	REQUIRE(ISCAPI_TASK_VALID(task));
 
 	CHECKED_MEM_GET_PTR(mctx, sctx);
 	ZERO_PTR(sctx);
 	isc_mem_attach(mctx, &sctx->mctx);
 
+	sctx->inst = inst;
+
 	CHECK(isc_mutex_init(&sctx->mutex));
 	lock_ready = ISC_TRUE;
 	CHECK(isc_condition_init(&sctx->cond));
 	cond_ready = ISC_TRUE;
 
 	/* refcount includes ldap_inst->task implicitly */
 	CHECK(isc_refcount_init(&sctx->task_cnt, 0));
 	refcount_ready = ISC_TRUE;
 
-	isc_task_attach(task, &sctx->excl_task);
-
 	ISC_LIST_INIT(sctx->tasks);
 
 	sctx->state = sync_init;
-	CHECK(sync_task_add(sctx, task));
+	CHECK(sync_task_add(sctx, ldap_instance_gettask(sctx->inst)));
 
 	CHECK(semaphore_init(&sctx->concurr_limit, LDAP_CONCURRENCY_LIMIT));
 
@@ -322,7 +327,6 @@ sync_ctx_free(sync_ctx_t **sctxp) {
 	}
 	isc_condition_destroy(&sctx->cond);
 	isc_refcount_destroy(&sctx->task_cnt);
-	isc_task_detach(&sctx->excl_task);
 	UNLOCK(&sctx->mutex);
 
 	isc_mutex_destroy(&(*sctxp)->mutex);
@@ -441,11 +445,28 @@ cleanup:
  * End of syncrepl event processing has to be signalled by
  * sync_concurr_limit_signal() call.
  */
-void
+isc_result_t
 sync_concurr_limit_wait(sync_ctx_t *sctx) {
+	isc_result_t result;
+	isc_time_t abs_timeout;
+
 	REQUIRE(sctx != NULL);
 
-	semaphore_wait(&sctx->concurr_limit);
+	while (ldap_instance_isexiting(sctx->inst) == ISC_FALSE) {
+		result = isc_time_nowplusinterval(&abs_timeout,
+						  &shutdown_timeout);
+		INSIST(result == ISC_R_SUCCESS);
+
+		result = semaphore_wait_timed(&sctx->concurr_limit,
+					      &shutdown_timeout);
+		if (result == ISC_R_SUCCESS)
+			goto cleanup;
+	}
+
+	result = ISC_R_SHUTTINGDOWN;
+
+cleanup:
+	return result;
 }
 
 /**
@@ -465,14 +486,29 @@ sync_concurr_limit_signal(sync_ctx_t *sctx) {
  * End of event processing has to be signalled by
  * sync_event_signal() call.
  */
-void
+isc_result_t
 sync_event_wait(sync_ctx_t *sctx, isc_event_t *ev) {
+	isc_result_t result;
+	isc_time_t abs_timeout;
+
 	REQUIRE(sctx != NULL);
 
 	LOCK(&sctx->mutex);
-	while (sctx->last_ev != ev)
-		WAIT(&sctx->cond, &sctx->mutex);
+	while (sctx->last_ev != ev) {
+		if (ldap_instance_isexiting(sctx->inst) == ISC_TRUE)
+			CLEANUP_WITH(ISC_R_SHUTTINGDOWN);
+
+		result = isc_time_nowplusinterval(&abs_timeout, &shutdown_timeout);
+		INSIST(result == ISC_R_SUCCESS);
+
+		WAITUNTIL(&sctx->cond, &sctx->mutex, &abs_timeout);
+	}
+
+	result = ISC_R_SUCCESS;
+
+cleanup:
 	UNLOCK(&sctx->mutex);
+	return result;
 }
 
 /**
diff --git a/src/syncrepl.h b/src/syncrepl.h
index 31d9d7f6e1725039a1743560aba2b243da91ab3c..4f534c0428b8ff80944f758f96678a19372550d3 100644
--- a/src/syncrepl.h
+++ b/src/syncrepl.h
@@ -43,7 +43,7 @@ enum sync_state {
 };
 
 isc_result_t
-sync_ctx_init(isc_mem_t *mctx, isc_task_t *task, sync_ctx_t **sctxp) ATTR_NONNULLS ATTR_CHECKRESULT;
+sync_ctx_init(isc_mem_t *mctx, ldap_instance_t *inst, sync_ctx_t **sctxp) ATTR_NONNULLS ATTR_CHECKRESULT;
 
 void
 sync_ctx_free(sync_ctx_t **statep);
@@ -60,14 +60,14 @@ sync_task_add(sync_ctx_t *sctx, isc_task_t *task) ATTR_NONNULLS ATTR_CHECKRESULT
 isc_result_t
 sync_barrier_wait(sync_ctx_t *sctx, const char *inst_name) ATTR_NONNULLS ATTR_CHECKRESULT;
 
-void ATTR_NONNULLS
-sync_concurr_limit_wait(sync_ctx_t *sctx) ATTR_NONNULLS;
+isc_result_t
+sync_concurr_limit_wait(sync_ctx_t *sctx) ATTR_NONNULLS ATTR_CHECKRESULT;
 
-void ATTR_NONNULLS
+void
 sync_concurr_limit_signal(sync_ctx_t *sctx) ATTR_NONNULLS;
 
-void
-sync_event_wait(sync_ctx_t *sctx, isc_event_t *ev) ATTR_NONNULLS;
+isc_result_t
+sync_event_wait(sync_ctx_t *sctx, isc_event_t *ev) ATTR_NONNULLS ATTR_CHECKRESULT;
 
 void
 sync_event_signal(sync_ctx_t *sctx, isc_event_t *ev) ATTR_NONNULLS;
-- 
1.9.3

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to