[nfs-utils PATCH] gssd: clean up threads on shutdown

[nfs-utils PATCH] gssd: clean up threads on shutdown

[Scott Mayhew]

Valgrind reports leaks such as the following:

==1781== 736 bytes in 2 blocks are possibly lost in loss record 831 of 926
==1781==    at 0x487F36B: calloc (vg_replace_malloc.c:1616)
==1781==    by 0x403A75F: calloc (rtld-malloc.h:44)
==1781==    by 0x403A75F: allocate_dtv (dl-tls.c:477)
==1781==    by 0x403B2E5: _dl_allocate_tls (dl-tls.c:734)
==1781==    by 0x4B6579E: pthread_create@@GLIBC_2.34 (in /usr/lib64/libc.so.6)
==1781==    by 0x400E531: start_upcall_thread.constprop.0 (gssd_proc.c:971)
==1781==    by 0x400EC34: handle_gssd_upcall (gssd_proc.c:1117)
==1781==    by 0x48B9D9B: event_persist_closure (event.c:1623)
==1781==    by 0x48B9D9B: event_process_active_single_queue (event.c:1682)
==1781==    by 0x48BA5BE: event_process_active (event.c:1783)
==1781==    by 0x48BA5BE: event_base_loop (event.c:2006)
==1781==    by 0x400A431: main (gssd.c:1292)
==1781==

This is due to the thread-local storage allocated by pthread_create() as
well as the upcall_thread_info struct allocated by
start_upcall_thread().  Those do eventually get freed by the watchdog
thread in scan_active_thread_list().  The thread-local storage gets
freed when pthread_tryjoin_np() succeeds and the upcall_thread_info gets
freed via TAILQ_REMOVE() + free().  But since the watchdog thread sleeps
between iterations (30 seconds by default), from the perspective of
tools like valgrind there’s a leak.

Add logic to clean up the threads on shutdown to make tools like
valgrind happy.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 utils/gssd/gssd.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 6a41fc5d..e381c0f2 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -98,6 +98,7 @@ static struct event_base *evbase = NULL;
 
 int upcall_timeout = DEF_UPCALL_TIMEOUT;
 static bool cancel_timed_out_upcalls = false;
+pthread_t watchdog_tid;
 
 TAILQ_HEAD(topdir_list_head, topdir) topdir_list;
 
@@ -498,6 +499,61 @@ gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data)
 	handle_krb5_upcall(clp);
 }
 
+static void
+cleanup_active_thread_list(void)
+{
+	struct upcall_thread_info *info;
+	bool cancelled = false;
+	void *tret, *saveprev;
+	int err;
+
+	pthread_mutex_lock(&active_thread_list_lock);
+	TAILQ_FOREACH(info, &active_thread_list, list) {
+		err = pthread_tryjoin_np(info->tid, &tret);
+		switch (err) {
+		case 0:
+			saveprev = info->list.tqe_prev;
+			TAILQ_REMOVE(&active_thread_list, info, list);
+			free(info);
+			info = saveprev;
+			break;
+		case EBUSY:
+			pthread_cancel(info->tid);
+			do_error_downcall(info->fd, info->uid, -ETIMEDOUT);
+			cancelled = true;
+			break;
+		default:
+			/* EDEADLK, EINVAL, and ESRCH... none of which should happen! */
+			printerr(0, "watchdog: attempt to join thread id 0x%lx returned %d (%s)!\n",
+					info->tid, err, strerror(err));
+			break;
+		}
+	}
+	if (cancelled) {
+		TAILQ_FOREACH(info, &active_thread_list, list) {
+			err = pthread_tryjoin_np(info->tid, &tret);
+			switch (err) {
+			case EBUSY:
+				printerr(0, "watchdog: thread id 0x%lx still busy on shutdown\n",
+					 info->tid);
+				/* fall through */
+			default:
+				/* EDEADLK, EINVAL, and ESRCH... none of which should happen! */
+				printerr(0, "watchdog: attempt to join thread id 0x%lx returned %d (%s)!\n",
+						info->tid, err, strerror(err));
+				/* fall through */
+			case 0:
+				saveprev = info->list.tqe_prev;
+				TAILQ_REMOVE(&active_thread_list, info, list);
+				free(info);
+				info = saveprev;
+				break;
+			}
+		}
+	}
+	pthread_mutex_unlock(&active_thread_list_lock);
+}
+
 /*
  * scan_active_thread_list:
  *
@@ -628,6 +684,7 @@ start_watchdog_thread(void)
 		printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
 			 ret, strerror(errno));
 	}
+	watchdog_tid = th;
 	return ret;
 }
 
@@ -1290,6 +1347,9 @@ main(int argc, char *argv[])
 
 	printerr(0, "event_dispatch() returned %i!\n", rc);
 
+	pthread_cancel(watchdog_tid);
+	cleanup_active_thread_list();
+
 	gssd_destroy_krb5_principals(root_uses_machine_creds);
 
 	while (!TAILQ_EMPTY(&topdir_list)) {
-- 
2.55.0

Re: [nfs-utils PATCH] gssd: clean up threads on shutdown

[Steve Dickson]

On 8/31/26 7:02 PM, Scott Mayhew wrote:
> Valgrind reports leaks such as the following:
> 
> ==1781== 736 bytes in 2 blocks are possibly lost in loss record 831 of 926
> ==1781==    at 0x487F36B: calloc (vg_replace_malloc.c:1616)
> ==1781==    by 0x403A75F: calloc (rtld-malloc.h:44)
> ==1781==    by 0x403A75F: allocate_dtv (dl-tls.c:477)
> ==1781==    by 0x403B2E5: _dl_allocate_tls (dl-tls.c:734)
> ==1781==    by 0x4B6579E: pthread_create@@GLIBC_2.34 (in /usr/lib64/libc.so.6)
> ==1781==    by 0x400E531: start_upcall_thread.constprop.0 (gssd_proc.c:971)
> ==1781==    by 0x400EC34: handle_gssd_upcall (gssd_proc.c:1117)
> ==1781==    by 0x48B9D9B: event_persist_closure (event.c:1623)
> ==1781==    by 0x48B9D9B: event_process_active_single_queue (event.c:1682)
> ==1781==    by 0x48BA5BE: event_process_active (event.c:1783)
> ==1781==    by 0x48BA5BE: event_base_loop (event.c:2006)
> ==1781==    by 0x400A431: main (gssd.c:1292)
> ==1781==
> 
> This is due to the thread-local storage allocated by pthread_create() as
> well as the upcall_thread_info struct allocated by
> start_upcall_thread().  Those do eventually get freed by the watchdog
> thread in scan_active_thread_list().  The thread-local storage gets
> freed when pthread_tryjoin_np() succeeds and the upcall_thread_info gets
> freed via TAILQ_REMOVE() + free().  But since the watchdog thread sleeps
> between iterations (30 seconds by default), from the perspective of
> tools like valgrind there’s a leak.
> 
> Add logic to clean up the threads on shutdown to make tools like
> valgrind happy.
> 
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
Committed... (tag: nfs-utils-2-9-3-rc5)

steved.
> ---
>   utils/gssd/gssd.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 60 insertions(+)
> 
> diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
> index 6a41fc5d..e381c0f2 100644
> --- a/utils/gssd/gssd.c
> +++ b/utils/gssd/gssd.c
> @@ -98,6 +98,7 @@ static struct event_base *evbase = NULL;
>   
>   int upcall_timeout = DEF_UPCALL_TIMEOUT;
>   static bool cancel_timed_out_upcalls = false;
> +pthread_t watchdog_tid;
>   
>   TAILQ_HEAD(topdir_list_head, topdir) topdir_list;
>   
> @@ -498,6 +499,61 @@ gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data)
>   	handle_krb5_upcall(clp);
>   }
>   
> +static void
> +cleanup_active_thread_list(void)
> +{
> +	struct upcall_thread_info *info;
> +	bool cancelled = false;
> +	void *tret, *saveprev;
> +	int err;
> +
> +	pthread_mutex_lock(&active_thread_list_lock);
> +	TAILQ_FOREACH(info, &active_thread_list, list) {
> +		err = pthread_tryjoin_np(info->tid, &tret);
> +		switch (err) {
> +		case 0:
> +			saveprev = info->list.tqe_prev;
> +			TAILQ_REMOVE(&active_thread_list, info, list);
> +			free(info);
> +			info = saveprev;
> +			break;
> +		case EBUSY:
> +			pthread_cancel(info->tid);
> +			do_error_downcall(info->fd, info->uid, -ETIMEDOUT);
> +			cancelled = true;
> +			break;
> +		default:
> +			/* EDEADLK, EINVAL, and ESRCH... none of which should happen! */
> +			printerr(0, "watchdog: attempt to join thread id 0x%lx returned %d (%s)!\n",
> +					info->tid, err, strerror(err));
> +			break;
> +		}
> +	}
> +	if (cancelled) {
> +		TAILQ_FOREACH(info, &active_thread_list, list) {
> +			err = pthread_tryjoin_np(info->tid, &tret);
> +			switch (err) {
> +			case EBUSY:
> +				printerr(0, "watchdog: thread id 0x%lx still busy on shutdown\n",
> +					 info->tid);
> +				/* fall through */
> +			default:
> +				/* EDEADLK, EINVAL, and ESRCH... none of which should happen! */
> +				printerr(0, "watchdog: attempt to join thread id 0x%lx returned %d (%s)!\n",
> +						info->tid, err, strerror(err));
> +				/* fall through */
> +			case 0:
> +				saveprev = info->list.tqe_prev;
> +				TAILQ_REMOVE(&active_thread_list, info, list);
> +				free(info);
> +				info = saveprev;
> +				break;
> +			}
> +		}
> +	}
> +	pthread_mutex_unlock(&active_thread_list_lock);
> +}
> +
>   /*
>    * scan_active_thread_list:
>    *
> @@ -628,6 +684,7 @@ start_watchdog_thread(void)
>   		printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
>   			 ret, strerror(errno));
>   	}
> +	watchdog_tid = th;
>   	return ret;
>   }
>   
> @@ -1290,6 +1347,9 @@ main(int argc, char *argv[])
>   
>   	printerr(0, "event_dispatch() returned %i!\n", rc);
>   
> +	pthread_cancel(watchdog_tid);
> +	cleanup_active_thread_list();
> +
>   	gssd_destroy_krb5_principals(root_uses_machine_creds);
>   
>   	while (!TAILQ_EMPTY(&topdir_list)) {