[Freeipa-devel] [PATCH 0060] Fix zone delete in ldap_zone_delete2()
Adam Tkac
atkac at redhat.com
Mon Sep 24 13:00:36 UTC 2012
On Thu, Sep 13, 2012 at 01:36:32PM +0200, Petr Spacek wrote:
> Hello,
>
> Fix zone delete in ldap_zone_delete2(). This fixes two race
> conditions during BIND reload:
>
> - failing assert in destroy_ldap_connection() DESTROYLOCK:
> ((pthread_mutex_destroy(&ldap_conn->lock) == 0) ? 0 : 34) == 0
>
> - use-after-free in call:
> ldap_cache_enabled(cache=0xdededededededede)
Ack.
When pushing, please consider if "race-condition" is good description. From my
point of view better is "fix extraction of zone FQDN when destroing LDAP
instance" or something like that.
Regards, Adam
> From dc017b4d7250289eb5938262dbb43632126f9329 Mon Sep 17 00:00:00 2001
> From: Petr Spacek <pspacek at redhat.com>
> Date: Thu, 13 Sep 2012 13:02:19 +0200
> Subject: [PATCH] Fix zone delete in ldap_zone_delete2(). This fixes two race
> conditions during BIND reload:
>
> - failing assert in destroy_ldap_connection() DESTROYLOCK:
> ((pthread_mutex_destroy(&ldap_conn->lock) == 0) ? 0 : 34) == 0
>
> - use-after-free in call:
> ldap_cache_enabled(cache=0xdededededededede)
>
> Signed-off-by: Petr Spacek <pspacek at redhat.com>
> ---
> src/ldap_helper.c | 64 +++++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 48 insertions(+), 16 deletions(-)
>
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index 67a64b79159983c83cb1bfc73c4b02a9bce986a7..3b49de809738fef18cae10c38fd3d9c33eef5141 100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -517,45 +517,68 @@ destroy_ldap_instance(ldap_instance_t **ldap_instp)
> ldap_instance_t *ldap_inst;
> dns_rbtnodechain_t chain;
> dns_rbt_t *rbt;
> - isc_result_t result;
> + isc_result_t result = ISC_R_SUCCESS;
> + const char *db_name;
>
> REQUIRE(ldap_instp != NULL && *ldap_instp != NULL);
>
> ldap_inst = *ldap_instp;
> + db_name = ldap_inst->db_name; /* points to DB instance: outside ldap_inst */
>
> /*
> * Unregister all zones already registered in BIND.
> *
> * NOTE: This should be probably done in zone_register.c
> */
> - dns_rbtnodechain_init(&chain, ldap_inst->mctx);
> rbt = zr_get_rbt(ldap_inst->zone_register);
>
> /* Potentially ISC_R_NOSPACE can occur. Destroy codepath has no way to
> * return errors, so kill BIND.
> * DNS_R_NAMETOOLONG should never happen, because all names were checked
> * while loading. */
> - result = dns_rbtnodechain_first(&chain, rbt, NULL, NULL);
> - RUNTIME_CHECK(result == ISC_R_SUCCESS || result == DNS_R_NEWORIGIN
> - || result == ISC_R_NOTFOUND);
>
> + dns_rbtnodechain_init(&chain, ldap_inst->mctx);
> while (result != ISC_R_NOMORE && result != ISC_R_NOTFOUND) {
> dns_fixedname_t name;
> + dns_fixedname_t origin;
> + dns_fixedname_t concat;
> dns_fixedname_init(&name);
> - result = dns_rbtnodechain_current(&chain, NULL,
> - dns_fixedname_name(&name),
> - NULL);
> - RUNTIME_CHECK(result == ISC_R_SUCCESS);
> + dns_fixedname_init(&origin);
> + dns_fixedname_init(&concat);
> +
> + dns_rbtnodechain_reset(&chain);
> + result = dns_rbtnodechain_first(&chain, rbt, NULL, NULL);
> + RUNTIME_CHECK(result == ISC_R_SUCCESS || result == DNS_R_NEWORIGIN ||
> + result == ISC_R_NOTFOUND);
> +
> + /* Search for first zone in zone register and omit auxiliary nodes. */
> + while (result != ISC_R_NOMORE && result != ISC_R_NOTFOUND) {
> + dns_rbtnode_t *node = NULL;
> +
> + result = dns_rbtnodechain_current(&chain, dns_fixedname_name(&name),
> + dns_fixedname_name(&origin), &node);
> + RUNTIME_CHECK(result == ISC_R_SUCCESS);
> +
> + if (node->data != NULL) { /* Auxiliary nodes have data == NULL. */
> + result = dns_name_concatenate(dns_fixedname_name(&name),
> + dns_fixedname_name(&origin),
> + dns_fixedname_name(&concat),
> + NULL);
> + RUNTIME_CHECK(result == ISC_R_SUCCESS);
> + break;
> + }
> +
> + result = dns_rbtnodechain_next(&chain, NULL, NULL);
> + RUNTIME_CHECK(result == ISC_R_SUCCESS || result == DNS_R_NEWORIGIN ||
> + result == ISC_R_NOMORE);
> + }
> + if (result == ISC_R_NOMORE || result == ISC_R_NOTFOUND)
> + break;
>
> result = ldap_delete_zone2(ldap_inst,
> - dns_fixedname_name(&name),
> + dns_fixedname_name(&concat),
> ISC_TRUE);
> - RUNTIME_CHECK(result == ISC_R_SUCCESS);
> -
> - result = dns_rbtnodechain_next(&chain, NULL, NULL);
> - RUNTIME_CHECK(result == ISC_R_SUCCESS ||
> - result == DNS_R_NEWORIGIN ||
> - result == ISC_R_NOMORE);
> + RUNTIME_CHECK(result == ISC_R_SUCCESS);
> }
>
> dns_rbtnodechain_invalidate(&chain);
> @@ -606,6 +629,7 @@ destroy_ldap_instance(ldap_instance_t **ldap_instp)
> isc_mem_putanddetach(&ldap_inst->mctx, ldap_inst, sizeof(ldap_instance_t));
>
> *ldap_instp = NULL;
> + log_debug(1, "LDAP instance '%s' destroyed", db_name);
> }
>
> static isc_result_t
> @@ -776,7 +800,10 @@ ldap_delete_zone2(ldap_instance_t *inst, dns_name_t *name, isc_boolean_t lock)
> isc_boolean_t freeze = ISC_FALSE;
> dns_zone_t *zone = NULL;
> dns_zone_t *foundzone = NULL;
> + char zone_name_char[DNS_NAME_FORMATSIZE];
>
> + dns_name_format(name, zone_name_char, DNS_NAME_FORMATSIZE);
> + log_debug(1, "deleting zone '%s'", zone_name_char);
> if (lock) {
> result = isc_task_beginexclusive(inst->task);
> RUNTIME_CHECK(result == ISC_R_SUCCESS ||
> @@ -790,6 +817,7 @@ ldap_delete_zone2(ldap_instance_t *inst, dns_name_t *name, isc_boolean_t lock)
>
> result = zr_get_zone_ptr(inst->zone_register, name, &zone);
> if (result == ISC_R_NOTFOUND) {
> + log_debug(1, "zone '%s' not found in zone register", zone_name_char);
> result = ISC_R_SUCCESS;
> goto cleanup;
> } else if (result != ISC_R_SUCCESS)
> @@ -810,7 +838,11 @@ ldap_delete_zone2(ldap_instance_t *inst, dns_name_t *name, isc_boolean_t lock)
> if (dns_zone_getdb(zone, &dbp) == ISC_R_SUCCESS) {
> dns_db_detach(&dbp); /* dns_zone_getdb() attaches DB implicitly */
> dns_zone_unload(zone);
> + log_debug(1, "zone '%s' unloaded", zone_name_char);
> + } else {
> + log_debug(1, "zone '%s' not loaded - unload skipped", zone_name_char);
> }
> +
> CHECK(dns_zt_unmount(inst->view->zonetable, zone));
> CHECK(zr_del_zone(inst->zone_register, name));
> dns_zonemgr_releasezone(inst->zmgr, zone);
> --
> 1.7.11.4
>
--
Adam Tkac, Red Hat, Inc.
More information about the Freeipa-devel
mailing list