2022-07-26 14:22:48

by Attila Kovacs

Subject: [PATCH 1/2] clnt_dg_freeres() uncleared set active state may deadlock.

From: Attila Kovacs <[email protected]>

In clnt_dg.c in clnt_dg_freeres(), cu_fd_lock->active is set to TRUE, with no
corresponding clearing when the operation (*xdr_res() call) is completed. This
would leave other waiting operations blocked indefinitely, effectively
deadlocking the client. For comparison, clnt_vd_freeres() in clnt_vc.c does not
set the active state to TRUE. I believe the vc behavior is correct, while the
dg behavior is a bug.

Signed-off-by: Attila Kovacs <[email protected]>
---
src/clnt_dg.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/src/clnt_dg.c b/src/clnt_dg.c
index 7c5d22e..b2043ac 100644
--- a/src/clnt_dg.c
+++ b/src/clnt_dg.c
@@ -573,7 +573,6 @@ clnt_dg_freeres(cl, xdr_res, res_ptr)
mutex_lock(&clnt_fd_lock);
while (cu->cu_fd_lock->active)
cond_wait(&cu->cu_fd_lock->cv, &clnt_fd_lock);
- cu->cu_fd_lock->active = TRUE;
xdrs->x_op = XDR_FREE;
dummy = (*xdr_res)(xdrs, res_ptr);
thr_sigsetmask(SIG_SETMASK, &mask, NULL);
--
2.37.1

2022-07-26 14:23:27

by Attila Kovacs

Subject: [PATCH 2/2] thread safe clnt destruction.

From: Attila Kovacs <[email protected]>

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 <[email protected]>
---
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);
}
--
2.37.1

2022-07-28 13:27:49

by Steve Dickson

Subject: Re: [PATCH 2/2] thread safe clnt destruction.

On 7/26/22 10:19 AM, Attila Kovacs wrote:
> From: Attila Kovacs <[email protected]>
>
> 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 <[email protected]>
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);
> }

2022-07-28 13:28:37

by Steve Dickson

Subject: Re: [PATCH 1/2] clnt_dg_freeres() uncleared set active state may deadlock.

On 7/26/22 10:19 AM, Attila Kovacs wrote:
> From: Attila Kovacs <[email protected]>
>
> In clnt_dg.c in clnt_dg_freeres(), cu_fd_lock->active is set to TRUE, with no
> corresponding clearing when the operation (*xdr_res() call) is completed. This
> would leave other waiting operations blocked indefinitely, effectively
> deadlocking the client. For comparison, clnt_vd_freeres() in clnt_vc.c does not
> set the active state to TRUE. I believe the vc behavior is correct, while the
> dg behavior is a bug.
>
> Signed-off-by: Attila Kovacs <[email protected]>
Committed (tag: libtirpc-1-3-3-rc4)

steved.
> ---
> src/clnt_dg.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/src/clnt_dg.c b/src/clnt_dg.c
> index 7c5d22e..b2043ac 100644
> --- a/src/clnt_dg.c
> +++ b/src/clnt_dg.c
> @@ -573,7 +573,6 @@ clnt_dg_freeres(cl, xdr_res, res_ptr)
> mutex_lock(&clnt_fd_lock);
> while (cu->cu_fd_lock->active)
> cond_wait(&cu->cu_fd_lock->cv, &clnt_fd_lock);
> - cu->cu_fd_lock->active = TRUE;
> xdrs->x_op = XDR_FREE;
> dummy = (*xdr_res)(xdrs, res_ptr);
> thr_sigsetmask(SIG_SETMASK, &mask, NULL);