[Freeipa-devel] [PATCH 0078] Use automatic connection management in LDAP modification code to prevent potential deadlock
Adam Tkac
atkac at redhat.com
Mon Mar 25 10:17:05 UTC 2013
On Tue, Mar 05, 2013 at 05:24:26PM +0100, Petr Spacek wrote:
> On 15.10.2012 13:10, Petr Spacek wrote:
> >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.
>
> Patch rebased on top of master branch. No functional changes.
Ack.
> From 9af7dae883b6f014fdcb5aee8394ad5d8f87aab9 Mon Sep 17 00:00:00 2001
> From: Petr Spacek <pspacek at redhat.com>
> Date: Tue, 5 Mar 2013 17:10:06 +0100
> Subject: [PATCH] Use automatic connection management in LDAP modification
> code to prevent potential deadlock.
>
> Without this patch the plugin could 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 | 54 +++++++++++++++++++++++-------------------------------
> 1 file changed, 23 insertions(+), 31 deletions(-)
>
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index c5c8245ecd664f038781fc98f4b5756ceff87c2e..3ed4a44e043ce98a5503ad351f4e8a91116d5ac3 100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -312,8 +312,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,
> @@ -2472,31 +2471,19 @@ 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));
> -
> - if (ldap_conn->handle == NULL) {
> - /*
> - * handle can be NULL when the first connection to LDAP wasn't
> - * successful
> - * TODO: handle this case inside ldap_pool_getconnection()?
> - */
> - CHECK(ldap_connect(ldap_inst, ldap_conn, ISC_FALSE));
> - }
> -
> /* Any mod_op can be ORed with LDAP_MOD_BVALUES. */
> if ((mods[0]->mod_op & ~LDAP_MOD_BVALUES) == LDAP_MOD_ADD)
> operation_str = "modifying(add)";
> @@ -2507,7 +2494,17 @@ ldap_modify_do(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn,
> else {
> operation_str = "modifying(unknown operation)";
> log_bug("%s: 0x%x", operation_str, mods[0]->mod_op);
> - CHECK(ISC_R_NOTIMPLEMENTED);
> + CLEANUP_WITH(ISC_R_NOTIMPLEMENTED);
> + }
> +
> + CHECK(ldap_pool_getconnection(ldap_inst->pool, &ldap_conn));
> + if (ldap_conn->handle == NULL) {
> + /*
> + * handle can be NULL when the first connection to LDAP wasn't
> + * successful
> + * TODO: handle this case inside ldap_pool_getconnection()?
> + */
> + CHECK(ldap_connect(ldap_inst, ldap_conn, ISC_FALSE));
> }
>
> if (delete_node) {
> @@ -2569,8 +2566,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;
> }
> @@ -2738,8 +2734,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;
> @@ -2772,7 +2768,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;
> @@ -2787,7 +2783,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;
> @@ -2846,10 +2841,8 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst,
> CHECK(zr_get_zone_cache(ldap_inst->zone_register, owner, &cache));
> CHECK(discard_from_cache(cache, owner));
>
> - CHECK(ldap_pool_getconnection(ldap_inst->pool, &ldap_conn));
> -
> 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;
> }
> @@ -2860,7 +2853,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) {
> @@ -3018,16 +3011,15 @@ 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),
> + CHECK(ldap_modify_do(ldap_inst, str_buf(owner_dn_ptr),
> change, delete_node));
> cache = NULL;
> CHECK(zr_get_zone_cache(ldap_inst->zone_register,
> dns_fixedname_name(&ptr_name), &cache));
> CHECK(discard_from_cache(cache, 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);
> @@ -3303,7 +3295,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(zr_get_zone_cache(inst->zone_register, zone_name, &cache));
> CHECK(discard_from_cache(cache, zone_name));
>
> --
> 1.7.11.7
>
--
Adam Tkac, Red Hat, Inc.
More information about the Freeipa-devel
mailing list