From: Mark Andrews Date: Wed, 24 Jan 2018 07:55:56 +0000 (+1100) Subject: 4876. [bug] Address deadlock with accessing a keytable. [RT #47000] X-Git-Tag: v9.11.3b1~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=635e4351b04fd61ca6d853bdac6268c090b55129;p=thirdparty%2Fbind9.git 4876. [bug] Address deadlock with accessing a keytable. [RT #47000] (cherry picked from commit b575c4ec42ab9aefa86e97b691e0e887c1748670) --- diff --git a/CHANGES b/CHANGES index 4a38f38316b..30ae6ef2882 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ +4876. [bug] Address deadlock with accessing a keytable. [RT #47000] + 4875. [bug] Address compile failures on older systems. [RT #47015] 4874. [bug] Wrong time display when reporting new keywarntime. diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index 042e9bb5bf5..7ac7fabce74 100644 --- a/lib/dns/keytable.c +++ b/lib/dns/keytable.c @@ -14,6 +14,7 @@ #include #include +#include #include #include /* Required for HP/UX (and others?) */ #include @@ -33,12 +34,10 @@ struct dns_keytable { /* Unlocked. */ unsigned int magic; isc_mem_t *mctx; - isc_mutex_t lock; + isc_refcount_t active_nodes; + isc_refcount_t references; isc_rwlock_t rwlock; - /* Locked by lock. */ - isc_uint32_t active_nodes; /* Locked by rwlock. */ - isc_uint32_t references; dns_rbt_t *table; }; @@ -70,38 +69,48 @@ dns_keytable_create(isc_mem_t *mctx, dns_keytable_t **keytablep) { REQUIRE(keytablep != NULL && *keytablep == NULL); keytable = isc_mem_get(mctx, sizeof(*keytable)); - if (keytable == NULL) + if (keytable == NULL) { return (ISC_R_NOMEMORY); + } keytable->table = NULL; result = dns_rbt_create(mctx, free_keynode, mctx, &keytable->table); - if (result != ISC_R_SUCCESS) + if (result != ISC_R_SUCCESS) { goto cleanup_keytable; + } - result = isc_mutex_init(&keytable->lock); - if (result != ISC_R_SUCCESS) + result = isc_rwlock_init(&keytable->rwlock, 0, 0); + if (result != ISC_R_SUCCESS) { goto cleanup_rbt; + } - result = isc_rwlock_init(&keytable->rwlock, 0, 0); - if (result != ISC_R_SUCCESS) - goto cleanup_lock; + result = isc_refcount_init(&keytable->active_nodes, 0); + if (result != ISC_R_SUCCESS) { + goto cleanup_rwlock; + } + + result = isc_refcount_init(&keytable->references, 1); + if (result != ISC_R_SUCCESS) { + goto cleanup_active_nodes; + } keytable->mctx = NULL; isc_mem_attach(mctx, &keytable->mctx); - keytable->active_nodes = 0; - keytable->references = 1; keytable->magic = KEYTABLE_MAGIC; *keytablep = keytable; return (ISC_R_SUCCESS); - cleanup_lock: - DESTROYLOCK(&keytable->lock); + cleanup_active_nodes: + isc_refcount_destroy(&keytable->active_nodes); - cleanup_rbt: + cleanup_rwlock: + isc_rwlock_destroy(&keytable->rwlock); + + cleanup_rbt: dns_rbt_destroy(&keytable->table); - cleanup_keytable: + cleanup_keytable: isc_mem_putanddetach(&mctx, keytable, sizeof(*keytable)); return (result); @@ -117,21 +126,15 @@ dns_keytable_attach(dns_keytable_t *source, dns_keytable_t **targetp) { REQUIRE(VALID_KEYTABLE(source)); REQUIRE(targetp != NULL && *targetp == NULL); - RWLOCK(&source->rwlock, isc_rwlocktype_write); - - INSIST(source->references > 0); - source->references++; - INSIST(source->references != 0); - - RWUNLOCK(&source->rwlock, isc_rwlocktype_write); + isc_refcount_increment(&source->references, NULL); *targetp = source; } void dns_keytable_detach(dns_keytable_t **keytablep) { - isc_boolean_t destroy = ISC_FALSE; dns_keytable_t *keytable; + unsigned int refs; /* * Detach *keytablep from its keytable. @@ -140,28 +143,19 @@ dns_keytable_detach(dns_keytable_t **keytablep) { REQUIRE(keytablep != NULL && VALID_KEYTABLE(*keytablep)); keytable = *keytablep; + *keytablep = NULL; - RWLOCK(&keytable->rwlock, isc_rwlocktype_write); - - INSIST(keytable->references > 0); - keytable->references--; - LOCK(&keytable->lock); - if (keytable->references == 0 && keytable->active_nodes == 0) - destroy = ISC_TRUE; - UNLOCK(&keytable->lock); - - RWUNLOCK(&keytable->rwlock, isc_rwlocktype_write); - - if (destroy) { + isc_refcount_decrement(&keytable->references, &refs); + if (refs == 0) { + INSIST(isc_refcount_current(&keytable->active_nodes) == 0); + isc_refcount_destroy(&keytable->active_nodes); + isc_refcount_destroy(&keytable->references); dns_rbt_destroy(&keytable->table); isc_rwlock_destroy(&keytable->rwlock); - DESTROYLOCK(&keytable->lock); keytable->magic = 0; isc_mem_putanddetach(&keytable->mctx, keytable, sizeof(*keytable)); } - - *keytablep = NULL; } static isc_result_t @@ -346,9 +340,7 @@ dns_keytable_find(dns_keytable_t *keytable, dns_name_t *keyname, DNS_RBTFIND_NOOPTIONS, NULL, NULL); if (result == ISC_R_SUCCESS) { if (node->data != NULL) { - LOCK(&keytable->lock); - keytable->active_nodes++; - UNLOCK(&keytable->lock); + isc_refcount_increment0(&keytable->active_nodes, NULL); dns_keynode_attach(node->data, keynodep); } else result = ISC_R_NOTFOUND; @@ -376,9 +368,7 @@ dns_keytable_nextkeynode(dns_keytable_t *keytable, dns_keynode_t *keynode, return (ISC_R_NOTFOUND); dns_keynode_attach(keynode->next, nextnodep); - LOCK(&keytable->lock); - keytable->active_nodes++; - UNLOCK(&keytable->lock); + isc_refcount_increment(&keytable->active_nodes, NULL); return (ISC_R_SUCCESS); } @@ -426,9 +416,7 @@ dns_keytable_findkeynode(dns_keytable_t *keytable, dns_name_t *name, break; } if (knode != NULL) { - LOCK(&keytable->lock); - keytable->active_nodes++; - UNLOCK(&keytable->lock); + isc_refcount_increment0(&keytable->active_nodes, NULL); dns_keynode_attach(knode, keynodep); } else result = DNS_R_PARTIALMATCH; @@ -466,9 +454,7 @@ dns_keytable_findnextkeynode(dns_keytable_t *keytable, dns_keynode_t *keynode, break; } if (knode != NULL) { - LOCK(&keytable->lock); - keytable->active_nodes++; - UNLOCK(&keytable->lock); + isc_refcount_increment(&keytable->active_nodes, NULL); result = ISC_R_SUCCESS; dns_keynode_attach(knode, nextnodep); } else @@ -517,9 +503,7 @@ dns_keytable_attachkeynode(dns_keytable_t *keytable, dns_keynode_t *source, REQUIRE(VALID_KEYNODE(source)); REQUIRE(target != NULL && *target == NULL); - LOCK(&keytable->lock); - keytable->active_nodes++; - UNLOCK(&keytable->lock); + isc_refcount_increment(&keytable->active_nodes, NULL); dns_keynode_attach(source, target); } @@ -534,11 +518,7 @@ dns_keytable_detachkeynode(dns_keytable_t *keytable, dns_keynode_t **keynodep) REQUIRE(VALID_KEYTABLE(keytable)); REQUIRE(keynodep != NULL && VALID_KEYNODE(*keynodep)); - LOCK(&keytable->lock); - INSIST(keytable->active_nodes > 0); - keytable->active_nodes--; - UNLOCK(&keytable->lock); - + isc_refcount_decrement(&keytable->active_nodes, NULL); dns_keynode_detach(keytable->mctx, keynodep); } @@ -683,6 +663,7 @@ dns_keytable_forall(dns_keytable_t *keytable, result = ISC_R_SUCCESS; goto cleanup; } + isc_refcount_increment0(&keytable->active_nodes, NULL); for (;;) { dns_rbtnodechain_current(&chain, NULL, NULL, &node); if (node->data != NULL) @@ -694,6 +675,7 @@ dns_keytable_forall(dns_keytable_t *keytable, break; } } + isc_refcount_decrement(&keytable->active_nodes, NULL); cleanup: dns_rbtnodechain_invalidate(&chain); @@ -726,7 +708,7 @@ dns_keynode_managed(dns_keynode_t *keynode) { isc_result_t dns_keynode_create(isc_mem_t *mctx, dns_keynode_t **target) { isc_result_t result; - dns_keynode_t *knode = NULL; + dns_keynode_t *knode; REQUIRE(target != NULL && *target == NULL);