Hello,

Fix race condition during write to internal RBTDB.

RBTDB implementation allows to open only one RBTDB instance
for writing at the same time. This patch adds mutex to
newversion() implementation in ldap_driver.c.

See comments around ldapdb_t, newversion() and closeversion().

--
Petr^2 Spacek
From fe1ba56a48baca2d506b3a0468303ed8fbe0231f Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Thu, 31 Oct 2013 15:31:32 +0100
Subject: [PATCH] Fix race condition during write to internal RBTDB.

RBTDB implementation allows to open only one RBTDB instance
for writing at the same time. This patch adds mutex to
newversion() implementation in ldap_driver.c.

See comments around ldapdb_t, newversion() and closeversion().

Signed-off-by: Petr Spacek <pspa...@redhat.com>
---
 src/ldap_driver.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 src/ldap_helper.c | 13 +++++-----
 2 files changed, 80 insertions(+), 8 deletions(-)

diff --git a/src/ldap_driver.c b/src/ldap_driver.c
index 9de574a6179f2966b0e169a0c97e537a9a11e256..11ade10a12edc89c4bd0e713788cc56b8afee83d 100644
--- a/src/ldap_driver.c
+++ b/src/ldap_driver.c
@@ -70,7 +70,28 @@ typedef struct {
 	dns_db_t			common;
 	isc_refcount_t			refs;
 	ldap_instance_t			*ldap_inst;
+
+	/**
+	 * Internal RBT database implementation provided by BIND 9.
+	 * Most of read only dns_db_*  calls (find(), createiterator(), etc.)
+	 * are blindly forwarded to this RBTDB.
+	 * Data modification calls (like addrdataset() etc.) are intercepted
+	 * by this driver, data manipulation is done in LDAP
+	 * and then the same modification is done in internal RBTDB. */
 	dns_db_t			*rbtdb;
+
+	/**
+	 * Guard for newversion. Only one new version can be open at any time.
+	 * newversion(ldapdb, newver) locks the lock
+	 * and closeversion(ldapdb, newver) unlocks the lock. */
+	isc_mutex_t			newversion_lock;
+
+	/**
+	 * Upcoming RBTDB version. It is automatically updated
+	 * by newversion(ldapdb) and closeversion(ldapdb).
+	 * The purpose is to detect moment when the new version is closed.
+	 * That is the right time for unlocking newversion_lock. */
+	dns_dbversion_t			*newversion;
 } ldapdb_t;
 
 dns_db_t * ATTR_NONNULLS
@@ -147,6 +168,8 @@ cleanup:
 #endif
 	dns_db_detach(&ldapdb->rbtdb);
 	dns_name_free(&ldapdb->common.origin, ldapdb->common.mctx);
+	RUNTIME_CHECK(isc_mutex_destroy(&ldapdb->newversion_lock)
+		      == ISC_R_SUCCESS);
 	isc_mem_putanddetach(&ldapdb->common.mctx, ldapdb, sizeof(*ldapdb));
 }
 
@@ -241,14 +264,46 @@ currentversion(dns_db_t *db, dns_dbversion_t **versionp)
 	dns_db_currentversion(ldapdb->rbtdb, versionp);
 }
 
+/**
+ * @brief Allocate and open new RBTDB version.
+ *
+ * New version is compatible with LDAPDB and RBTDB.
+ * Only one new version can be open at any time. This limitation is enforced
+ * by ldapdb->newversion_lock.
+ *
+ * @warning This function has to be used for all newversion() calls for LDAPDB
+ *          AND also internal RBTDB (ldapdb->rbtdb). This ensures proper
+ *          serialization and prevents assertion failures in newversion().
+ *
+ * How to work with internal RBTDB versions in safe way (note ldapdb vs. rbtdb):
+ * @verbatim
+	CHECK(dns_db_newversion(ldapdb, &newversion));
+	// do whatevent you need with the newversion, e.g.:
+	CHECK(dns_diff_apply(diff, rbtdb, newversion));
+
+cleanup:
+	if (newversion != NULL)
+		dns_db_closeversion(ldapdb, &newversion, ISC_TRUE);
+   @endverbatim
+ */
 static isc_result_t
 newversion(dns_db_t *db, dns_dbversion_t **versionp)
 {
 	ldapdb_t *ldapdb = (ldapdb_t *)db;
+	isc_result_t result;
 
 	REQUIRE(VALID_LDAPDB(ldapdb));
 
-	return dns_db_newversion(ldapdb->rbtdb, versionp);
+	LOCK(&ldapdb->newversion_lock);
+	result = dns_db_newversion(ldapdb->rbtdb, versionp);
+	if (result == ISC_R_SUCCESS) {
+		INSIST(*versionp != NULL);
+		ldapdb->newversion = *versionp;
+	} else {
+		INSIST(*versionp == NULL);
+		UNLOCK(&ldapdb->newversion_lock);
+	}
+	return result;
 }
 
 static void
@@ -262,13 +317,24 @@ attachversion(dns_db_t *db, dns_dbversion_t *source,
 	dns_db_attachversion(ldapdb->rbtdb, source, targetp);
 }
 
+/**
+ * @brief Close LDAPDB and internal RBTDB version.
+ *
+ * @see newversion for related warnings and examples.
+ */
 static void
 closeversion(dns_db_t *db, dns_dbversion_t **versionp, isc_boolean_t commit)
 {
 	ldapdb_t *ldapdb = (ldapdb_t *)db;
+	dns_dbversion_t *closed_version = *versionp;
 
 	REQUIRE(VALID_LDAPDB(ldapdb));
+
 	dns_db_closeversion(ldapdb->rbtdb, versionp, commit);
+	if (closed_version == ldapdb->newversion) {
+		ldapdb->newversion = NULL;
+		UNLOCK(&ldapdb->newversion_lock);
+	}
 }
 
 static isc_result_t
@@ -926,6 +992,7 @@ ldapdb_create(isc_mem_t *mctx, dns_name_t *name, dns_dbtype_t type,
 {
 	ldapdb_t *ldapdb = NULL;
 	isc_result_t result;
+	isc_boolean_t lock_ready = ISC_FALSE;
 
 	UNUSED(driverarg); /* Currently we don't need any data */
 
@@ -940,7 +1007,8 @@ ldapdb_create(isc_mem_t *mctx, dns_name_t *name, dns_dbtype_t type,
 	ZERO_PTR(ldapdb);
 
 	isc_mem_attach(mctx, &ldapdb->common.mctx);
-
+	CHECK(isc_mutex_init(&ldapdb->newversion_lock));
+	lock_ready = ISC_TRUE;
 	dns_name_init(&ldapdb->common.origin, NULL);
 	isc_ondestroy_init(&ldapdb->common.ondest);
 
@@ -966,6 +1034,9 @@ ldapdb_create(isc_mem_t *mctx, dns_name_t *name, dns_dbtype_t type,
 
 cleanup:
 	if (ldapdb != NULL) {
+		if (lock_ready == ISC_TRUE)
+			RUNTIME_CHECK(isc_mutex_destroy(&ldapdb->newversion_lock)
+				      == ISC_R_SUCCESS);
 		if (dns_name_dynamic(&ldapdb->common.origin))
 			dns_name_free(&ldapdb->common.origin, mctx);
 
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 8000da782a42b77c2ebade5e3ef20f5e4c415d86..9d4a2f62d4a6d29d56d6ce611b686f43161f3dcb 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -1767,7 +1767,7 @@ ldap_parse_master_zoneentry(ldap_entry_t *entry, ldap_instance_t *inst)
 	CHECK(dns_zone_getserial2(zone, &curr_serial));
 
 	CHECK(zr_get_zone_dbs(inst->zone_register, &name, &ldapdb, &rbtdb));
-	CHECK(dns_db_newversion(rbtdb, &version));
+	CHECK(dns_db_newversion(ldapdb, &version));
 	CHECK(dns_db_getoriginnode(rbtdb, &node));
 	result = dns_db_allrdatasets(rbtdb, node, version, 0,
 				     &rbt_rds_iterator);
@@ -1859,7 +1859,7 @@ ldap_parse_master_zoneentry(ldap_entry_t *entry, ldap_instance_t *inst)
 		CHECK(dns_diff_apply(&diff, rbtdb, version));
 		if (sync_state == sync_finished)
 			dns_journal_destroy(&journal);
-		dns_db_closeversion(rbtdb, &version, ISC_TRUE);
+		dns_db_closeversion(ldapdb, &version, ISC_TRUE);
 	}
 
 	CHECK(dns_zone_getserial2(zone, &curr_serial));
@@ -1877,7 +1877,7 @@ cleanup:
 	if (node != NULL)
 		dns_db_detachnode(rbtdb, &node);
 	if (rbtdb != NULL && version != NULL)
-		dns_db_closeversion(rbtdb, &version, ISC_FALSE); /* rollback */
+		dns_db_closeversion(ldapdb, &version, ISC_FALSE); /* rollback */
 	if (rbtdb != NULL)
 		dns_db_detach(&rbtdb);
 	if (ldapdb != NULL)
@@ -3822,6 +3822,7 @@ update_record(isc_task_t *task, isc_event_t *event)
 	dns_name_init(&origin, NULL);
 	dns_name_init(&prevname, NULL);
 	dns_name_init(&prevorigin, NULL);
+
 	CHECK(manager_get_ldap_instance(pevent->dbname, &inst));
 	CHECK(dn_to_dnsname(mctx, pevent->dn, &name, &origin));
 	CHECK(zr_get_zone_ptr(inst->zone_register, &origin, &zone_ptr));
@@ -3832,7 +3833,7 @@ update_restart:
 	ldapdb = NULL;
 	journal = NULL;
 	CHECK(zr_get_zone_dbs(inst->zone_register, &name, &ldapdb, &rbtdb));
-	CHECK(dns_db_newversion(rbtdb, &version));
+	CHECK(dns_db_newversion(ldapdb, &version));
 
 	CHECK(dns_db_findnode(rbtdb, &name, ISC_TRUE, &node));
 	result = dns_db_allrdatasets(rbtdb, node, version, 0, &rbt_rds_iterator);
@@ -3935,7 +3936,7 @@ update_restart:
 		CHECK(dns_diff_apply(&diff, rbtdb, version));
 		if (sync_state == sync_finished)
 			dns_journal_destroy(&journal);
-		dns_db_closeversion(rbtdb, &version, ISC_TRUE);
+		dns_db_closeversion(ldapdb, &version, ISC_TRUE);
 	}
 
 cleanup:
@@ -3953,7 +3954,7 @@ cleanup:
 		dns_db_detachnode(rbtdb, &node);
 	/* rollback */
 	if (rbtdb != NULL && version != NULL)
-		dns_db_closeversion(rbtdb, &version, ISC_FALSE);
+		dns_db_closeversion(ldapdb, &version, ISC_FALSE);
 	if (journal != NULL)
 		dns_journal_destroy(&journal);
 	if (rbtdb != NULL)
-- 
1.8.3.1

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

Reply via email to