On 11.11.2013 13:03, Petr Spacek wrote:
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().
I'm attaching rebased patch.
This patch should go to master branch.
--
Petr^2 Spacek
From 618a6c4b5aed18b5c4d36e9351a93a60f67abae6 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 ee12fdfa7ca1d6fdecb679da47fac00baea5ffc4..56db0bf6595d3a8815504d05eb2b509ea03f6c62 100644
--- a/src/ldap_driver.c
+++ b/src/ldap_driver.c
@@ -69,7 +69,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
@@ -146,6 +167,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));
}
@@ -240,14 +263,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
@@ -261,13 +316,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
@@ -925,6 +991,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 */
@@ -939,7 +1006,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);
@@ -965,6 +1033,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 52cbfb3e3c04c517b976d5235f83f42423c7b4e6..b086272c555dc1ca9e0151c3b481e06f2b6d93b4 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -1764,7 +1764,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);
@@ -1849,7 +1849,7 @@ ldap_parse_master_zoneentry(ldap_entry_t *entry, ldap_instance_t *inst)
/* commit */
CHECK(dns_diff_apply(&diff, rbtdb, version));
- dns_db_closeversion(rbtdb, &version, ISC_TRUE);
+ dns_db_closeversion(ldapdb, &version, ISC_TRUE);
}
CHECK(dns_zone_getserial2(zone, &curr_serial));
@@ -1867,7 +1867,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 (journal != NULL)
@@ -3831,6 +3831,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));
@@ -3847,7 +3848,7 @@ update_restart:
journal = NULL;
ldapdb_rdatalist_destroy(mctx, &rdatalist);
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);
@@ -3944,7 +3945,7 @@ update_restart:
/* commit */
CHECK(dns_diff_apply(&diff, rbtdb, version));
- dns_db_closeversion(rbtdb, &version, ISC_TRUE);
+ dns_db_closeversion(ldapdb, &version, ISC_TRUE);
}
/* Check if the zone is loaded or not.
@@ -3966,7 +3967,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 (rbtdb != NULL)
dns_db_detach(&rbtdb);
if (journal != NULL)
--
1.8.3.1
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
