On 28.5.2014 13:26, Tomas Hozza wrote:
On 05/27/2014 03:59 PM, Petr Spacek wrote:
On 27.5.2014 15:54, Petr Spacek wrote:
Fix race condition during zone loading.

DNS zone has to be added to DNS view before dns_zone_load() is called.
It is necessary to prevent dns_zone_load() from racing with
dns_zone_setview().

This race condition sometimes prevents zone from being signed.
Now the unsigned zone is visible until signing process is complete. This
mimics BIND behavior for in-line signed zones.

https://fedorahosted.org/bind-dyndb-ldap/ticket/56

And here is the patch...


Hi.

When I use bind-dyndb-ldap plugin with this patch, named
does not start due to:

rbt.c:1379: REQUIRE(name->buffer != ((void *)0)) failed, back trace

(gdb) bt
#0  0x00007f3963924c39 in raise () from /lib64/libc.so.6
#1  0x00007f3963926348 in abort () from /lib64/libc.so.6
#2  0x00007f3966979aee in assertion_failed ()
#3  0x00007f3964b6917a in isc_assertion_failed () from /lib64/libisc.so.95
#4  0x00007f39661ca9da in dns_rbt_fullnamefromnode () from
/lib64/libdns.so.100
#5  0x00007f396011824b in rbt_iter_getnodename (iter=<optimized out>,
nodename=nodename@entry=0x7f39625f8bf0) at rbt_helper.c:46
#6  0x00007f396011839b in rbt_iter_next
(iterp=iterp@entry=0x7f39625f8b90,
nodename=nodename@entry=0x7f39625f8bf0) at rbt_helper.c:144
#7  0x00007f3960112d32 in activate_zones
(task=task@entry=0x7f39668f5790, inst=0x7f39668e4160) at ldap_helper.c:1164
#8  0x00007f396011a20d in barrier_decrement (task=0x7f39668f5790,
event=0x7f396005b010) at syncrepl.c:138
#9  0x00007f3964b8b836 in run () from /lib64/libisc.so.95
#10 0x00007f396473ff33 in start_thread () from /lib64/libpthread.so.0
#11 0x00007f39639e3ded in clone () from /lib64/libc.so.6


It looks like you should use INIT_BUFFERED_NAME(name); used in the
original code instead of dns_name_init(&name, NULL). The macro
initializes the buffer in "name", which is missing in the new code.

Oh yes, it didn't happened on my machine because I have had only single zone defined in LDAP at the time of testing. Thank you for catching this!

I'm attaching fixed patch. dns_name_reset() is good enough in this case.

--
Petr^2 Spacek
From 2be7e3201f11e1322309534ab6762d637c8642c1 Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Tue, 27 May 2014 15:41:59 +0200
Subject: [PATCH] Fix race condition during zone loading.

DNS zone has to be added to DNS view before dns_zone_load() is called.
It is necessary to prevent dns_zone_load() from racing with dns_zone_setview().

This race condition sometimes prevents zone from being signed.
Now the unsigned zone is visible until signing process is complete. This
mimics BIND behavior for in-line signed zones.

https://fedorahosted.org/bind-dyndb-ldap/ticket/56

Signed-off-by: Petr Spacek <pspa...@redhat.com>
---
 src/ldap_helper.c | 97 ++++++++++++++++++++++++++++++-------------------------
 src/ldap_helper.h |  2 +-
 2 files changed, 54 insertions(+), 45 deletions(-)

diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 3150b56f118b6270bb79a8bc2491c472b98477dc..b285d548d9a4ccbfb008ff024d4b6281315c629f 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -1100,65 +1100,74 @@ cleanup:
 }
 
 /**
+ * Add zone to view and call dns_zone_load().
+ */
+static isc_result_t ATTR_NONNULLS ATTR_CHECKRESULT
+activate_zone(isc_task_t *task, ldap_instance_t *inst, dns_name_t *name) {
+	isc_result_t result;
+	dns_zone_t *raw = NULL;
+	dns_zone_t *secure = NULL;
+	dns_zone_t *toview = NULL;
+	settings_set_t *zone_settings = NULL;
+
+	CHECK(zr_get_zone_ptr(inst->zone_register, name, &raw, &secure));
+
+	/* Load only "secure" zone if inline-signing is active.
+	 * It will not work if raw zone is loaded explicitly
+	 * - dns_zone_load() will fail magically. */
+	toview = (secure != NULL) ? secure : raw;
+
+	/*
+	 * Zone has to be published *before* zone load
+	 * otherwise it will race with zone->view != NULL check
+	 * in zone_maintenance() in zone.c.
+	 */
+	result = publish_zone(task, inst, toview);
+	if (result != ISC_R_SUCCESS) {
+		dns_zone_log(toview, ISC_LOG_ERROR,
+			     "cannot add zone to view: %s",
+			     dns_result_totext(result));
+		goto cleanup;
+	}
+
+	CHECK(load_zone(toview));
+	if (secure != NULL) {
+		CHECK(zr_get_zone_settings(inst->zone_register, name,
+					   &zone_settings));
+		CHECK(zone_master_reconfigure_nsec3param(zone_settings,
+							 secure));
+	}
+
+cleanup:
+	if (raw != NULL)
+		dns_zone_detach(&raw);
+	if (secure != NULL)
+		dns_zone_detach(&secure);
+	return result;
+}
+
+/**
  * Add all zones in zone register to DNS view specified in inst->view
  * and load zones.
  */
 isc_result_t
 activate_zones(isc_task_t *task, ldap_instance_t *inst) {
 	isc_result_t result;
-	isc_boolean_t loaded;
 	rbt_iterator_t *iter = NULL;
-	dns_zone_t *raw = NULL;
-	dns_zone_t *secure = NULL;
-	dns_zone_t *toview = NULL;
 	DECLARE_BUFFERED_NAME(name);
 	unsigned int published_cnt = 0;
 	unsigned int total_cnt = 0;
-	settings_set_t *zone_settings = NULL;
 
 	INIT_BUFFERED_NAME(name);
-	CHECK(zr_rbt_iter_init(inst->zone_register, &iter, &name));
-	do {
+	for(result = zr_rbt_iter_init(inst->zone_register, &iter, &name);
+	    result == ISC_R_SUCCESS;
+	    dns_name_reset(&name), result = rbt_iter_next(&iter, &name)) {
 		++total_cnt;
-		CHECK(zr_get_zone_ptr(inst->zone_register, &name, &raw, &secure));
-		/* Load only "secure" zone if inline-signing is active.
-		 * It will not work if raw zone is loaded explicitly. */
-		toview = (secure != NULL) ? secure : raw;
-		result = load_zone(toview);
-		loaded = (result == ISC_R_SUCCESS);
-		if (loaded == ISC_FALSE)
-			dns_zone_log(raw, ISC_LOG_ERROR,
-				     "unable to load zone: %s",
-				     dns_result_totext(result));
-		else if (secure != NULL) {
-			zone_settings = NULL;
-			CHECK(zr_get_zone_settings(inst->zone_register,
-						   &name, &zone_settings));
-			CHECK(zone_master_reconfigure_nsec3param(zone_settings,
-								 secure));
-		}
-
-		/*
-		 * Don't bother if load fails, server will return
-		 * SERVFAIL for queries beneath this zone. This is
-		 * admin's problem.
-		 */
-		result = publish_zone(task, inst, toview);
-		if (result != ISC_R_SUCCESS)
-			dns_zone_log(toview, ISC_LOG_ERROR,
-				     "cannot add zone to view: %s",
-				     dns_result_totext(result));
-		else if (loaded == ISC_TRUE)
+		result = activate_zone(task, inst, &name);
+		if (result == ISC_R_SUCCESS)
 			++published_cnt;
-		dns_zone_detach(&raw);
-		if (secure != NULL)
-			dns_zone_detach(&secure);
+	};
 
-		INIT_BUFFERED_NAME(name);
-		CHECK(rbt_iter_next(&iter, &name));
-	} while (result == ISC_R_SUCCESS);
-
-cleanup:
 	log_info("%u zones from LDAP instance '%s' loaded (%u zones defined)",
 		 published_cnt, inst->db_name, total_cnt);
 	return result;
diff --git a/src/ldap_helper.h b/src/ldap_helper.h
index bcc5e39083dc645b9522e4de80e72e05e956e80a..e72e97a032ba90e7974302bc92604640a1c2bb1a 100644
--- a/src/ldap_helper.h
+++ b/src/ldap_helper.h
@@ -103,6 +103,6 @@ const char * ldap_instance_getdbname(ldap_instance_t *ldap_inst) ATTR_NONNULLS;
 
 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);
+isc_result_t activate_zones(isc_task_t *task, ldap_instance_t *inst) ATTR_NONNULLS;
 
 #endif /* !_LD_LDAP_HELPER_H_ */
-- 
1.9.0

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

Reply via email to