From 8ef49081e8a42bfa05bb63265cd4f951e2b23413 Mon Sep 17 00:00:00 2001 From: Doug Nazar Date: Fri, 26 Jun 2020 16:02:04 -0400 Subject: [PATCH] gssd: Refcount struct clnt_info to protect multithread usage Struct clnt_info is shared with the various upcall threads so we need to ensure that it stays around even if the client dir gets removed. Reported-by: Sebastian Kraus Signed-off-by: Doug Nazar --- utils/gssd/gssd.c | 67 ++++++++++++++++++++++++++++++++---------- utils/gssd/gssd.h | 5 ++-- utils/gssd/gssd_proc.c | 4 +-- 3 files changed, 55 insertions(+), 21 deletions(-) diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c index 588da0fb..b40c3220 100644 --- a/utils/gssd/gssd.c +++ b/utils/gssd/gssd.c @@ -90,9 +90,7 @@ char *ccachedir = NULL; /* Avoid DNS reverse lookups on server names */ static bool avoid_dns = true; static bool use_gssproxy = false; -int thread_started = false; -pthread_mutex_t pmutex = PTHREAD_MUTEX_INITIALIZER; -pthread_cond_t pcond = PTHREAD_COND_INITIALIZER; +pthread_mutex_t clp_lock = PTHREAD_MUTEX_INITIALIZER; TAILQ_HEAD(topdir_list_head, topdir) topdir_list; @@ -359,20 +357,28 @@ out: free(port); } +/* Actually frees clp and fields that might be used from other + * threads if was last reference. + */ static void -gssd_destroy_client(struct clnt_info *clp) +gssd_free_client(struct clnt_info *clp) { - if (clp->krb5_fd >= 0) { + int refcnt; + + pthread_mutex_lock(&clp_lock); + refcnt = --clp->refcount; + pthread_mutex_unlock(&clp_lock); + if (refcnt > 0) + return; + + printerr(3, "freeing client %s\n", clp->relpath); + + if (clp->krb5_fd >= 0) close(clp->krb5_fd); - event_del(&clp->krb5_ev); - } - if (clp->gssd_fd >= 0) { + if (clp->gssd_fd >= 0) close(clp->gssd_fd); - event_del(&clp->gssd_ev); - } - inotify_rm_watch(inotify_fd, clp->wd); free(clp->relpath); free(clp->servicename); free(clp->servername); @@ -380,6 +386,24 @@ gssd_destroy_client(struct clnt_info *clp) free(clp); } +/* Called when removing from clnt_list to tear down event handling. + * Will then free clp if was last reference. + */ +static void +gssd_destroy_client(struct clnt_info *clp) +{ + printerr(3, "destroying client %s\n", clp->relpath); + + if (clp->krb5_fd >= 0) + event_del(&clp->krb5_ev); + + if (clp->gssd_fd >= 0) + event_del(&clp->gssd_ev); + + inotify_rm_watch(inotify_fd, clp->wd); + gssd_free_client(clp); +} + static void gssd_scan(void); static int @@ -416,11 +440,21 @@ static struct clnt_upcall_info *alloc_upcall_info(struct clnt_info *clp) info = malloc(sizeof(struct clnt_upcall_info)); if (info == NULL) return NULL; + + pthread_mutex_lock(&clp_lock); + clp->refcount++; + pthread_mutex_unlock(&clp_lock); info->clp = clp; return info; } +void free_upcall_info(struct clnt_upcall_info *info) +{ + gssd_free_client(info->clp); + free(info); +} + /* For each upcall read the upcall info into the buffer, then create a * thread in a detached state so that resources are released back into * the system without the need for a join. @@ -438,13 +472,13 @@ gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data) info->lbuflen = read(clp->gssd_fd, info->lbuf, sizeof(info->lbuf)); if (info->lbuflen <= 0 || info->lbuf[info->lbuflen-1] != '\n') { printerr(0, "WARNING: %s: failed reading request\n", __func__); - free(info); + free_upcall_info(info); return; } info->lbuf[info->lbuflen-1] = 0; if (start_upcall_thread(handle_gssd_upcall, info)) - free(info); + free_upcall_info(info); } static void @@ -461,12 +495,12 @@ gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data) sizeof(info->uid)) < (ssize_t)sizeof(info->uid)) { printerr(0, "WARNING: %s: failed reading uid from krb5 " "upcall pipe: %s\n", __func__, strerror(errno)); - free(info); + free_upcall_info(info); return; } if (start_upcall_thread(handle_krb5_upcall, info)) - free(info); + free_upcall_info(info); } static struct clnt_info * @@ -501,6 +535,7 @@ gssd_get_clnt(struct topdir *tdi, const char *name) clp->name = clp->relpath + strlen(tdi->name) + 1; clp->krb5_fd = -1; clp->gssd_fd = -1; + clp->refcount = 1; TAILQ_INSERT_HEAD(&tdi->clnt_list, clp, list); return clp; @@ -651,7 +686,7 @@ gssd_scan_topdir(const char *name) if (clp->scanned) continue; - printerr(3, "destroying client %s\n", clp->relpath); + printerr(3, "orphaned client %s\n", clp->relpath); saveprev = clp->list.tqe_prev; TAILQ_REMOVE(&tdi->clnt_list, clp, list); gssd_destroy_client(clp); diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h index f4f59754..d33e4dff 100644 --- a/utils/gssd/gssd.h +++ b/utils/gssd/gssd.h @@ -63,12 +63,10 @@ extern unsigned int context_timeout; extern unsigned int rpc_timeout; extern char *preferred_realm; extern pthread_mutex_t ple_lock; -extern pthread_cond_t pcond; -extern pthread_mutex_t pmutex; -extern int thread_started; struct clnt_info { TAILQ_ENTRY(clnt_info) list; + int refcount; int wd; bool scanned; char *name; @@ -94,6 +92,7 @@ struct clnt_upcall_info { void handle_krb5_upcall(struct clnt_upcall_info *clp); void handle_gssd_upcall(struct clnt_upcall_info *clp); +void free_upcall_info(struct clnt_upcall_info *info); #endif /* _RPC_GSSD_H_ */ diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c index 8fe6605b..05c1da64 100644 --- a/utils/gssd/gssd_proc.c +++ b/utils/gssd/gssd_proc.c @@ -730,7 +730,7 @@ handle_krb5_upcall(struct clnt_upcall_info *info) printerr(2, "\n%s: uid %d (%s)\n", __func__, info->uid, clp->relpath); process_krb5_upcall(clp, info->uid, clp->krb5_fd, NULL, NULL, NULL); - free(info); + free_upcall_info(info); } void @@ -830,6 +830,6 @@ handle_gssd_upcall(struct clnt_upcall_info *info) out: free(upcall_str); out_nomem: - free(info); + free_upcall_info(info); return; } -- 2.26.2