Re: [Freeipa-devel] [PATCH 0060] Fix zone delete in ldap_zone_delete2()

2012-09-24 Thread Adam Tkac
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 pspa...@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 pspa...@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),
 + 

Re: [Freeipa-devel] [PATCH 0060] Fix zone delete in ldap_zone_delete2()

2012-09-24 Thread Petr Spacek

On 09/24/2012 03:00 PM, Adam Tkac wrote:

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


Pushed with amended commit message:

e6286d061d12a604fa8c4c3eb282df7ec17b4cbe

Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel