diff mbox series

[2/2] thread safe clnt destruction.

Message ID 20220726141906.69023-2-attila.kovacs@cfa.harvard.edu (mailing list archive)
State New, archived
Headers show
Series [1/2] clnt_dg_freeres() uncleared set active state may deadlock. | expand

Commit Message

Attila Kovacs July 26, 2022, 2:19 p.m. UTC 
From: Attila Kovacs <attipaci@gmail.com>

If clnt_dg_destroy() or clnt_vc_destroy() is awoken with other blocked 
operations pending (such as clnt_*_call(), clnt_*_control(), or 
clnt_*_freeres()) but no active operation currently being executed, then the 
client gets destroyed. Then, as the other blocked operations get subsequently 
awoken, they will try operate on an invalid client handle, potentially causing 
unpredictable behavior and stack corruption. 

Signed-off-by: Attila Kovacs <attipaci@gmail.com>
---
 src/clnt_dg.c       | 13 ++++++++++++-
 src/clnt_fd_locks.h |  2 ++
 src/clnt_vc.c       | 13 ++++++++++++-
 3 files changed, 26 insertions(+), 2 deletions(-)

Comments

Steve Dickson July 28, 2022, 1:17 p.m. UTC | #1 
On 7/26/22 10:19 AM, Attila Kovacs wrote:
> From: Attila Kovacs <attipaci@gmail.com>
> 
> If clnt_dg_destroy() or clnt_vc_destroy() is awoken with other blocked
> operations pending (such as clnt_*_call(), clnt_*_control(), or
> clnt_*_freeres()) but no active operation currently being executed, then the
> client gets destroyed. Then, as the other blocked operations get subsequently
> awoken, they will try operate on an invalid client handle, potentially causing
> unpredictable behavior and stack corruption.
> 
> Signed-off-by: Attila Kovacs <attipaci@gmail.com>
Committed (tag: libtirpc-1-3-3-rc4)

steved.
> ---
>   src/clnt_dg.c       | 13 ++++++++++++-
>   src/clnt_fd_locks.h |  2 ++
>   src/clnt_vc.c       | 13 ++++++++++++-
>   3 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/src/clnt_dg.c b/src/clnt_dg.c
> index b2043ac..166af63 100644
> --- a/src/clnt_dg.c
> +++ b/src/clnt_dg.c
> @@ -101,6 +101,7 @@ extern mutex_t clnt_fd_lock;
>   #define	release_fd_lock(fd_lock, mask) {	\
>   	mutex_lock(&clnt_fd_lock);	\
>   	fd_lock->active = FALSE;	\
> +	fd_lock->pending--;		\
>   	thr_sigsetmask(SIG_SETMASK, &(mask), NULL); \
>   	cond_signal(&fd_lock->cv);	\
>   	mutex_unlock(&clnt_fd_lock);    \
> @@ -311,6 +312,7 @@ clnt_dg_call(cl, proc, xargs, argsp, xresults, resultsp, utimeout)
>   	sigfillset(&newmask);
>   	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
>   	mutex_lock(&clnt_fd_lock);
> +	cu->cu_fd_lock->pending++;
>   	while (cu->cu_fd_lock->active)
>   		cond_wait(&cu->cu_fd_lock->cv, &clnt_fd_lock);
>   	cu->cu_fd_lock->active = TRUE;
> @@ -571,10 +573,12 @@ clnt_dg_freeres(cl, xdr_res, res_ptr)
>   	sigfillset(&newmask);
>   	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
>   	mutex_lock(&clnt_fd_lock);
> +	cu->cu_fd_lock->pending++;
>   	while (cu->cu_fd_lock->active)
>   		cond_wait(&cu->cu_fd_lock->cv, &clnt_fd_lock);
>   	xdrs->x_op = XDR_FREE;
>   	dummy = (*xdr_res)(xdrs, res_ptr);
> +	cu->cu_fd_lock->pending--;
>   	thr_sigsetmask(SIG_SETMASK, &mask, NULL);
>   	cond_signal(&cu->cu_fd_lock->cv);
>   	mutex_unlock(&clnt_fd_lock);
> @@ -602,6 +606,7 @@ clnt_dg_control(cl, request, info)
>   	sigfillset(&newmask);
>   	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
>   	mutex_lock(&clnt_fd_lock);
> +	cu->cu_fd_lock->pending++;
>   	while (cu->cu_fd_lock->active)
>   		cond_wait(&cu->cu_fd_lock->cv, &clnt_fd_lock);
>   	cu->cu_fd_lock->active = TRUE;
> @@ -742,8 +747,14 @@ clnt_dg_destroy(cl)
>   	sigfillset(&newmask);
>   	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
>   	mutex_lock(&clnt_fd_lock);
> -	while (cu_fd_lock->active)
> +	/* wait until all pending operations on client are completed. */
> +	while (cu_fd_lock->pending > 0) {
> +		/* If a blocked operation can be awakened, then do it. */
> +		if (cu_fd_lock->active == FALSE)
> +			cond_signal(&cu_fd_lock->cv);
> +		/* keep waiting... */
>   		cond_wait(&cu_fd_lock->cv, &clnt_fd_lock);
> +	}
>   	if (cu->cu_closeit)
>   		(void)close(cu_fd);
>   	XDR_DESTROY(&(cu->cu_outxdrs));
> diff --git a/src/clnt_fd_locks.h b/src/clnt_fd_locks.h
> index 359f995..6ba62cb 100644
> --- a/src/clnt_fd_locks.h
> +++ b/src/clnt_fd_locks.h
> @@ -50,6 +50,7 @@ static unsigned int fd_locks_prealloc = 0;
>   /* per-fd lock */
>   struct fd_lock_t {
>   	bool_t active;
> +	int pending;        /* Number of pending operations on fd */
>   	cond_t cv;
>   };
>   typedef struct fd_lock_t fd_lock_t;
> @@ -180,6 +181,7 @@ fd_lock_t* fd_lock_create(int fd, fd_locks_t *fd_locks) {
>   		item->fd = fd;
>   		item->refs = 1;
>   		item->fd_lock.active = FALSE;
> +		item->fd_lock.pending = 0;
>   		cond_init(&item->fd_lock.cv, 0, (void *) 0);
>   		TAILQ_INSERT_HEAD(list, item, link);
>   	} else {
> diff --git a/src/clnt_vc.c b/src/clnt_vc.c
> index 3c73e65..5bbc78b 100644
> --- a/src/clnt_vc.c
> +++ b/src/clnt_vc.c
> @@ -153,6 +153,7 @@ extern mutex_t  clnt_fd_lock;
>   #define release_fd_lock(fd_lock, mask) {	\
>   	mutex_lock(&clnt_fd_lock);	\
>   	fd_lock->active = FALSE;	\
> +	fd_lock->pending--;		\
>   	thr_sigsetmask(SIG_SETMASK, &(mask), (sigset_t *) NULL);	\
>   	cond_signal(&fd_lock->cv);	\
>   	mutex_unlock(&clnt_fd_lock);    \
> @@ -357,6 +358,7 @@ clnt_vc_call(cl, proc, xdr_args, args_ptr, xdr_results, results_ptr, timeout)
>   	sigfillset(&newmask);
>   	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
>   	mutex_lock(&clnt_fd_lock);
> +	ct->ct_fd_lock->pending++;
>   	while (ct->ct_fd_lock->active)
>   		cond_wait(&ct->ct_fd_lock->cv, &clnt_fd_lock);
>   	ct->ct_fd_lock->active = TRUE;
> @@ -495,10 +497,12 @@ clnt_vc_freeres(cl, xdr_res, res_ptr)
>   	sigfillset(&newmask);
>   	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
>   	mutex_lock(&clnt_fd_lock);
> +	ct->ct_fd_lock->pending++;
>   	while (ct->ct_fd_lock->active)
>   		cond_wait(&ct->ct_fd_lock->cv, &clnt_fd_lock);
>   	xdrs->x_op = XDR_FREE;
>   	dummy = (*xdr_res)(xdrs, res_ptr);
> +	ct->ct_fd_lock->pending--;
>   	thr_sigsetmask(SIG_SETMASK, &(mask), NULL);
>   	cond_signal(&ct->ct_fd_lock->cv);
>   	mutex_unlock(&clnt_fd_lock);
> @@ -533,6 +537,7 @@ clnt_vc_control(cl, request, info)
>   	sigfillset(&newmask);
>   	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
>   	mutex_lock(&clnt_fd_lock);
> +	ct->ct_fd_lock->pending++;
>   	while (ct->ct_fd_lock->active)
>   		cond_wait(&ct->ct_fd_lock->cv, &clnt_fd_lock);
>   	ct->ct_fd_lock->active = TRUE;
> @@ -655,8 +660,14 @@ clnt_vc_destroy(cl)
>   	sigfillset(&newmask);
>   	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
>   	mutex_lock(&clnt_fd_lock);
> -	while (ct_fd_lock->active)
> +	/* wait until all pending operations on client are completed. */
> +	while (ct_fd_lock->pending > 0) {
> +		/* If a blocked operation can be awakened, then do it. */
> +		if (ct_fd_lock->active == FALSE)
> +			cond_signal(&ct_fd_lock->cv);
> +		/* keep waiting... */
>   		cond_wait(&ct_fd_lock->cv, &clnt_fd_lock);
> +	}
>   	if (ct->ct_closeit && ct->ct_fd != -1) {
>   		(void)close(ct->ct_fd);
>   	}
diff mbox series

Patch

diff --git a/src/clnt_dg.c b/src/clnt_dg.c
index b2043ac..166af63 100644
--- a/src/clnt_dg.c
+++ b/src/clnt_dg.c
@@ -101,6 +101,7 @@  extern mutex_t clnt_fd_lock;
 #define	release_fd_lock(fd_lock, mask) {	\
 	mutex_lock(&clnt_fd_lock);	\
 	fd_lock->active = FALSE;	\
+	fd_lock->pending--;		\
 	thr_sigsetmask(SIG_SETMASK, &(mask), NULL); \
 	cond_signal(&fd_lock->cv);	\
 	mutex_unlock(&clnt_fd_lock);    \
@@ -311,6 +312,7 @@  clnt_dg_call(cl, proc, xargs, argsp, xresults, resultsp, utimeout)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
+	cu->cu_fd_lock->pending++;
 	while (cu->cu_fd_lock->active)
 		cond_wait(&cu->cu_fd_lock->cv, &clnt_fd_lock);
 	cu->cu_fd_lock->active = TRUE;
@@ -571,10 +573,12 @@  clnt_dg_freeres(cl, xdr_res, res_ptr)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
+	cu->cu_fd_lock->pending++;
 	while (cu->cu_fd_lock->active)
 		cond_wait(&cu->cu_fd_lock->cv, &clnt_fd_lock);
 	xdrs->x_op = XDR_FREE;
 	dummy = (*xdr_res)(xdrs, res_ptr);
+	cu->cu_fd_lock->pending--;
 	thr_sigsetmask(SIG_SETMASK, &mask, NULL);
 	cond_signal(&cu->cu_fd_lock->cv);
 	mutex_unlock(&clnt_fd_lock);
@@ -602,6 +606,7 @@  clnt_dg_control(cl, request, info)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
+	cu->cu_fd_lock->pending++;
 	while (cu->cu_fd_lock->active)
 		cond_wait(&cu->cu_fd_lock->cv, &clnt_fd_lock);
 	cu->cu_fd_lock->active = TRUE;
@@ -742,8 +747,14 @@  clnt_dg_destroy(cl)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
-	while (cu_fd_lock->active)
+	/* wait until all pending operations on client are completed. */
+	while (cu_fd_lock->pending > 0) {
+		/* If a blocked operation can be awakened, then do it. */
+		if (cu_fd_lock->active == FALSE)
+			cond_signal(&cu_fd_lock->cv);
+		/* keep waiting... */
 		cond_wait(&cu_fd_lock->cv, &clnt_fd_lock);
+	}
 	if (cu->cu_closeit)
 		(void)close(cu_fd);
 	XDR_DESTROY(&(cu->cu_outxdrs));
diff --git a/src/clnt_fd_locks.h b/src/clnt_fd_locks.h
index 359f995..6ba62cb 100644
--- a/src/clnt_fd_locks.h
+++ b/src/clnt_fd_locks.h
@@ -50,6 +50,7 @@  static unsigned int fd_locks_prealloc = 0;
 /* per-fd lock */
 struct fd_lock_t {
 	bool_t active;
+	int pending;        /* Number of pending operations on fd */
 	cond_t cv;
 };
 typedef struct fd_lock_t fd_lock_t;
@@ -180,6 +181,7 @@  fd_lock_t* fd_lock_create(int fd, fd_locks_t *fd_locks) {
 		item->fd = fd;
 		item->refs = 1;
 		item->fd_lock.active = FALSE;
+		item->fd_lock.pending = 0;
 		cond_init(&item->fd_lock.cv, 0, (void *) 0);
 		TAILQ_INSERT_HEAD(list, item, link);
 	} else {
diff --git a/src/clnt_vc.c b/src/clnt_vc.c
index 3c73e65..5bbc78b 100644
--- a/src/clnt_vc.c
+++ b/src/clnt_vc.c
@@ -153,6 +153,7 @@  extern mutex_t  clnt_fd_lock;
 #define release_fd_lock(fd_lock, mask) {	\
 	mutex_lock(&clnt_fd_lock);	\
 	fd_lock->active = FALSE;	\
+	fd_lock->pending--;		\
 	thr_sigsetmask(SIG_SETMASK, &(mask), (sigset_t *) NULL);	\
 	cond_signal(&fd_lock->cv);	\
 	mutex_unlock(&clnt_fd_lock);    \
@@ -357,6 +358,7 @@  clnt_vc_call(cl, proc, xdr_args, args_ptr, xdr_results, results_ptr, timeout)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
+	ct->ct_fd_lock->pending++;
 	while (ct->ct_fd_lock->active)
 		cond_wait(&ct->ct_fd_lock->cv, &clnt_fd_lock);
 	ct->ct_fd_lock->active = TRUE;
@@ -495,10 +497,12 @@  clnt_vc_freeres(cl, xdr_res, res_ptr)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
+	ct->ct_fd_lock->pending++;
 	while (ct->ct_fd_lock->active)
 		cond_wait(&ct->ct_fd_lock->cv, &clnt_fd_lock);
 	xdrs->x_op = XDR_FREE;
 	dummy = (*xdr_res)(xdrs, res_ptr);
+	ct->ct_fd_lock->pending--;
 	thr_sigsetmask(SIG_SETMASK, &(mask), NULL);
 	cond_signal(&ct->ct_fd_lock->cv);
 	mutex_unlock(&clnt_fd_lock);
@@ -533,6 +537,7 @@  clnt_vc_control(cl, request, info)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
+	ct->ct_fd_lock->pending++;
 	while (ct->ct_fd_lock->active)
 		cond_wait(&ct->ct_fd_lock->cv, &clnt_fd_lock);
 	ct->ct_fd_lock->active = TRUE;
@@ -655,8 +660,14 @@  clnt_vc_destroy(cl)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
-	while (ct_fd_lock->active)
+	/* wait until all pending operations on client are completed. */
+	while (ct_fd_lock->pending > 0) {
+		/* If a blocked operation can be awakened, then do it. */
+		if (ct_fd_lock->active == FALSE)
+			cond_signal(&ct_fd_lock->cv);
+		/* keep waiting... */
 		cond_wait(&ct_fd_lock->cv, &clnt_fd_lock);
+	}
 	if (ct->ct_closeit && ct->ct_fd != -1) {
 		(void)close(ct->ct_fd);
 	}