[Freeipa-devel] [PATCH 0078] Use automatic connection management in LDAP modification code to prevent potential deadlock

[Adam Tkac]

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()

Regards, Adam

> From 5ad686a95510b1584c85d672ec845b27504f746c Mon Sep 17 00:00:00 2001
> From: Petr Spacek <pspacek at 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 <pspacek at redhat.com>
> ---
>  src/ldap_helper.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index f8df1b29871c28a1eeecfa93d5d91edd1aee3944..03adb83bdc34593ec527facd3e3fc60755a4de22 100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -2593,7 +2593,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 +2629,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 +2652,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, NULL, str_buf(owner_dn),
>  					   HEAD(rdlist->rdata));
>  		goto cleanup;
>  	}
> @@ -2666,7 +2663,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, NULL, 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 +2821,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, NULL, 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);
> -- 
> 1.7.11.4
> 


-- 
Adam Tkac, Red Hat, Inc.