New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deadlock in lib/krb5/mcache.c #432

Open
yasuoka opened this Issue Oct 4, 2018 · 0 comments

Comments

Projects
None yet
1 participant
@yasuoka

yasuoka commented Oct 4, 2018

lib/krb5/mcache.c has 2 types of mutex:

  • global mcc_mutex
  • mutex per struct krb5_mcache

Deadlocks happen since mcc_get_cache_first() and mcc_destroy() lock them in different order.

https://github.com/heimdal/heimdal/blob/master/lib/krb5/mcache.c#L421-L441

    421 static krb5_error_code KRB5_CALLCONV
    422 mcc_get_cache_first(krb5_context context, krb5_cc_cursor *cursor)
    423 {
    424     struct mcache_iter *iter;
    425 
    426     iter = calloc(1, sizeof(*iter));
    427     if (iter == NULL)
    428         return krb5_enomem(context);
    429 
    430     HEIMDAL_MUTEX_lock(&mcc_mutex);
    431     iter->cache = mcc_head;
    432     if (iter->cache) {
    433         HEIMDAL_MUTEX_lock(&(iter->cache->mutex));
    434         iter->cache->refcnt++;
    435         HEIMDAL_MUTEX_unlock(&(iter->cache->mutex));
    436     }
    437     HEIMDAL_MUTEX_unlock(&mcc_mutex);
    438 
    439     *cursor = iter;
    440     return 0;
    441 }

ORDER: mcc_mutext => cache->mutex

https://github.com/heimdal/heimdal/blob/master/lib/krb5/mcache.c#L245-L273

    245 static krb5_error_code KRB5_CALLCONV
    246 mcc_destroy(krb5_context context,
    247             krb5_ccache id)
    248 {
    249     krb5_mcache **n, *m = MCACHE(id);
    250 
    251     HEIMDAL_MUTEX_lock(&(m->mutex));
    252     if (m->refcnt == 0)
    253     {
    254         HEIMDAL_MUTEX_unlock(&(m->mutex));
    255         krb5_abortx(context, "mcc_destroy: refcnt already 0");
    256     }
    257 
    258     if (!MISDEAD(m)) {
    259         /* if this is an active mcache, remove it from the linked
    260            list, and free all data */
    261         HEIMDAL_MUTEX_lock(&mcc_mutex);
    262         for(n = &mcc_head; n && *n; n = &(*n)->next) {
    263             if(m == *n) {
    264                 *n = m->next;
    265                 break;
    266             }
    267         }
    268         HEIMDAL_MUTEX_unlock(&mcc_mutex);
    269         mcc_destroy_internal(context, m);
    270     }
    271     HEIMDAL_MUTEX_unlock(&(m->mutex));
    272     return 0;
    273 }

ORDER: cache->mutex => mcc_mutext

Deadlocks happen at L261 and L433.

The following diff will fix this.

diff --git a/lib/krb5/mcache.c b/lib/krb5/mcache.c
index 474cb3a2b..e45bc1b0a 100644
--- a/lib/krb5/mcache.c
+++ b/lib/krb5/mcache.c
@@ -248,27 +248,28 @@ mcc_destroy(krb5_context context,
 {
     krb5_mcache **n, *m = MCACHE(id);
 
+    HEIMDAL_MUTEX_lock(&mcc_mutex);
     HEIMDAL_MUTEX_lock(&(m->mutex));
     if (m->refcnt == 0)
     {
     	HEIMDAL_MUTEX_unlock(&(m->mutex));
+	HEIMDAL_MUTEX_unlock(&mcc_mutex);
     	krb5_abortx(context, "mcc_destroy: refcnt already 0");
     }
 
     if (!MISDEAD(m)) {
 	/* if this is an active mcache, remove it from the linked
            list, and free all data */
-	HEIMDAL_MUTEX_lock(&mcc_mutex);
 	for(n = &mcc_head; n && *n; n = &(*n)->next) {
 	    if(m == *n) {
 		*n = m->next;
 		break;
 	    }
 	}
-	HEIMDAL_MUTEX_unlock(&mcc_mutex);
 	mcc_destroy_internal(context, m);
     }
     HEIMDAL_MUTEX_unlock(&(m->mutex));
+    HEIMDAL_MUTEX_unlock(&mcc_mutex);
     return 0;
 }
 
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment