On 10/09/2012 03:49 PM, Petr Spacek wrote:
On 10/09/2012 01:21 PM, Adam Tkac wrote:
On Mon, Oct 08, 2012 at 04:46:54PM +0200, Petr Spacek wrote:
Hello,
Use automatic connection management in LDAP modification code to
prevent potential deadlock.
Without this patch the plugin will deadlock when modify_ldap_common()
is called with PTR synchronization enabled and only single
connection is available in the connection pool.
Nack
If I read the patch correctly, it leaves unused ldap_conn parameters in
ldap_modify_do() and modify_soa_record() functions.
Those params are always NULL so they can be safely removed. Please also remove
the "autoconn" variable from ldap_modify_do()
My intent was to keep the same connection management abilities as are in
ldap_query(): You can avoid repetitive ldap_pool_get/putconnection() calls by
passing connection via parameter.
I can remove it if it isn't worth. (Actually *_modify_*() functions do not use
this capability now.)
I forgot to send the patch after our discussion on IRC. Attached patch removes
unused parameters.
Petr^2 Spacek
From 7924717aab7d1dc343f21f6f459c1b4f21b373e1 Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Mon, 8 Oct 2012 16:41:40 +0200
Subject: [PATCH] Use automatic connection management in LDAP modification
code to prevent potential deadlock.
Without this patch the plugin will deadlock when modify_ldap_common()
is called with PTR synchronization enabled and only single
connection is available in the connection pool.
Signed-off-by: Petr Spacek <pspa...@redhat.com>
---
src/ldap_helper.c | 34 ++++++++++++++--------------------
1 file changed, 14 insertions(+), 20 deletions(-)
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index f8df1b29871c28a1eeecfa93d5d91edd1aee3944..c91aea44aab845854818373b8b9dd91fb0e059c9 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -306,8 +306,7 @@ static void ldap_query_free(isc_boolean_t prepare_reuse, ldap_qresult_t **ldap_q
/* Functions for writing to LDAP. */
static isc_result_t ldap_modify_do(ldap_instance_t *ldap_inst,
- ldap_connection_t *ldap_conn, const char *dn, LDAPMod **mods,
- isc_boolean_t delete_node);
+ const char *dn, LDAPMod **mods, isc_boolean_t delete_node);
static isc_result_t ldap_rdttl_to_ldapmod(isc_mem_t *mctx,
dns_rdatalist_t *rdlist, LDAPMod **changep);
static isc_result_t ldap_rdatalist_to_ldapmod(isc_mem_t *mctx,
@@ -2278,21 +2277,20 @@ reconnect:
}
static isc_result_t
-ldap_modify_do(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn,
- const char *dn, LDAPMod **mods, isc_boolean_t delete_node)
+ldap_modify_do(ldap_instance_t *ldap_inst, const char *dn, LDAPMod **mods,
+ isc_boolean_t delete_node)
{
int ret;
int err_code;
const char *operation_str;
isc_result_t result;
- isc_boolean_t autoconn = (ldap_conn == NULL);
+ ldap_connection_t *ldap_conn = NULL;
REQUIRE(dn != NULL);
REQUIRE(mods != NULL);
REQUIRE(ldap_inst != NULL);
- if (autoconn)
- CHECK(ldap_pool_getconnection(ldap_inst->pool, &ldap_conn));
+ CHECK(ldap_pool_getconnection(ldap_inst->pool, &ldap_conn));
if (ldap_conn->handle == NULL) {
/*
@@ -2375,8 +2373,7 @@ ldap_modify_do(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn,
result = ISC_R_FAILURE;
}
cleanup:
- if (autoconn)
- ldap_pool_putconnection(ldap_inst->pool, &ldap_conn);
+ ldap_pool_putconnection(ldap_inst->pool, &ldap_conn);
return result;
}
@@ -2544,8 +2541,8 @@ cleanup:
* refresh, retry, expire and minimum attributes for each SOA record.
*/
static isc_result_t
-modify_soa_record(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn,
- const char *zone_dn, dns_rdata_t *rdata)
+modify_soa_record(ldap_instance_t *ldap_inst, const char *zone_dn,
+ dns_rdata_t *rdata)
{
isc_result_t result;
dns_rdata_soa_t soa;
@@ -2578,7 +2575,7 @@ modify_soa_record(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn,
dns_rdata_freestruct((void *)&soa);
- result = ldap_modify_do(ldap_inst, ldap_conn, zone_dn, changep, ISC_FALSE);
+ result = ldap_modify_do(ldap_inst, zone_dn, changep, ISC_FALSE);
cleanup:
return result;
@@ -2593,7 +2590,6 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst,
{
isc_result_t result;
isc_mem_t *mctx = ldap_inst->mctx;
- ldap_connection_t *ldap_conn = NULL;
ld_string_t *owner_dn = NULL;
LDAPMod *change[3] = { NULL };
LDAPMod *change_ptr = NULL;
@@ -2630,8 +2626,6 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst,
CHECK(dn_to_dnsname(mctx, zone_dn, &zone_name, NULL));
- CHECK(ldap_pool_getconnection(ldap_inst->pool, &ldap_conn));
-
result = zr_get_zone_settings(ldap_inst->zone_register, &zone_name,
&zone_settings);
if (result != ISC_R_SUCCESS) {
@@ -2655,7 +2649,7 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst,
CHECK(discard_from_cache(cache, owner));
if (rdlist->type == dns_rdatatype_soa) {
- result = modify_soa_record(ldap_inst, ldap_conn, str_buf(owner_dn),
+ result = modify_soa_record(ldap_inst, str_buf(owner_dn),
HEAD(rdlist->rdata));
goto cleanup;
}
@@ -2666,7 +2660,7 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst,
CHECK(ldap_rdttl_to_ldapmod(mctx, rdlist, &change[1]));
}
- CHECK(ldap_modify_do(ldap_inst, ldap_conn, str_buf(owner_dn), change, delete_node));
+ CHECK(ldap_modify_do(ldap_inst, str_buf(owner_dn), change, delete_node));
/* Keep the PTR of corresponding A/AAAA record synchronized. */
if (rdlist->type == dns_rdatatype_a || rdlist->type == dns_rdatatype_aaaa) {
@@ -2824,13 +2818,13 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst,
change_ptr = NULL;
/* Modify PTR record. */
- CHECK(ldap_modify_do(ldap_inst, ldap_conn, str_buf(owner_dn_ptr), change, delete_node));
+ CHECK(ldap_modify_do(ldap_inst, str_buf(owner_dn_ptr),
+ change, delete_node));
(void) discard_from_cache(ldap_instance_getcache(ldap_inst),
dns_fixedname_name(&ptr_name));
}
cleanup:
- ldap_pool_putconnection(ldap_inst->pool, &ldap_conn);
str_destroy(&owner_dn_ptr);
str_destroy(&owner_dn);
str_destroy(&str_ptr);
@@ -3111,7 +3105,7 @@ soa_serial_increment(isc_mem_t *mctx, ldap_instance_t *inst,
dns_soa_setserial(new_serial, soa_rdata);
/* write the new serial back to DB */
- CHECK(modify_soa_record(inst, NULL, str_buf(zone_dn), soa_rdata));
+ CHECK(modify_soa_record(inst, str_buf(zone_dn), soa_rdata));
CHECK(discard_from_cache(ldap_instance_getcache(inst), zone_name));
/* put the new SOA to inst->cache and compare old and new serials */
--
1.7.11.4
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
