Fix concurrency issue in server socket handling

This commit is contained in:
Robbie Harwood 2017-10-27 11:18:27 -04:00
parent 02648264ab
commit 70190ed7bb
3 changed files with 91 additions and 1 deletions

View File

@ -0,0 +1,59 @@
From 99062c344b7dba58ab8db0fad5520a754d9a6841 Mon Sep 17 00:00:00 2001
From: Simo Sorce <simo@redhat.com>
Date: Thu, 26 Oct 2017 16:59:18 -0400
Subject: [PATCH] Do not call gpm_grab_sock() twice
In the gpm_get_ctx() call, we unnecessarily call gpm_grab_sock() which
would cause the lock to be held by one thread and never released. We
already call gpm_grab_sock() as the first thing after gpm_get_ctx() in
gpm_make_call(), plus gpm_make_call() properly releases the socket
once done.
This corrects the deadlock fix in
461a5fa9f91a2753ebeef6323a64239c35e2f250, which incorrectly released
the lock we wanted to grab. This caused the socket to not be locked
to our thread. Another thread could come along and change the global
ctx while we were still using the socket from another thread, causing
concurrency issues as only one request can be in flight on any given
socket at the same time.
In special cases where the "thread" uid/gid changes (like in
rpc.gssd), we end up closing the socket while we are still waiting for
an answer from the server, causing additional issues and confusion.
[rharwood@redhat.com: squashed 2 commits; minor edits accordingly]
Signed-off-by: Simo Sorce <simo@redhat.com>
Reviewed-by: Robbie Harwood <rharwood@redhat.com>
Merges: #218
(cherry picked from commit 8590c5dbc6fa07d0c366df23b982a4b6b9ffc259)
---
proxy/src/client/gpm_common.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/proxy/src/client/gpm_common.c b/proxy/src/client/gpm_common.c
index 994bd80..8837add 100644
--- a/proxy/src/client/gpm_common.c
+++ b/proxy/src/client/gpm_common.c
@@ -163,7 +163,9 @@ static int gpm_grab_sock(struct gpm_ctx *gpmctx)
ret = gpm_open_socket(gpmctx);
}
- pthread_mutex_unlock(&gpmctx->lock);
+ if (ret) {
+ pthread_mutex_unlock(&gpmctx->lock);
+ }
return ret;
}
@@ -512,11 +514,6 @@ static struct gpm_ctx *gpm_get_ctx(void)
pthread_once(&gpm_init_once_control, gpm_init_once);
- ret = gpm_grab_sock(&gpm_global_ctx);
- if (ret) {
- return NULL;
- }
-
return &gpm_global_ctx;
}

View File

@ -0,0 +1,26 @@
From d5f22a1c2ad70ff1e7922c91086a04f0dc31db58 Mon Sep 17 00:00:00 2001
From: Simo Sorce <simo@redhat.com>
Date: Thu, 26 Oct 2017 11:47:54 -0400
Subject: [PATCH] Emit debug on queue errors
Signed-off-by: Simo Sorce <simo@redhat.com>
Reviewed-by: Robbie Harwood <rharwood@redhat.com>
Merges: #218
(cherry picked from commit af666affbd4735ba437e3d89d9e22984a556ed16)
---
proxy/src/gp_workers.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/proxy/src/gp_workers.c b/proxy/src/gp_workers.c
index 2a33c21..18f38f6 100644
--- a/proxy/src/gp_workers.c
+++ b/proxy/src/gp_workers.c
@@ -314,6 +314,8 @@ static void gp_handle_reply(verto_ctx *vctx, verto_ev *ev)
case GP_QUERY_IN:
/* ?! fallback and kill client conn */
case GP_QUERY_ERR:
+ GPDEBUGN(3, "[status] Handling query error, terminating CID %d.\n",
+ gp_conn_get_cid(q->conn));
gp_conn_free(q->conn);
gp_query_free(q, true);
break;

View File

@ -1,6 +1,6 @@
Name: gssproxy
Version: 0.7.0
Release: 21%{?dist}
Release: 22%{?dist}
Summary: GSSAPI Proxy
Group: System Environment/Libraries
@ -35,6 +35,8 @@ Patch17: Fix-error-handling-in-gpm_send_buffer-gpm_recv_buffe.patch
Patch18: Handle-outdated-encrypted-ccaches.patch
Patch19: Fix-error-handling-in-gp_config_from_dir.patch
Patch20: Fix-silent-crash-with-duplicate-config-sections.patch
Patch21: Emit-debug-on-queue-errors.patch
Patch22: Do-not-call-gpm_grab_sock-twice.patch
### Dependencies ###
Requires: krb5-libs >= 1.12.0
@ -132,6 +134,9 @@ rm -rf %{buildroot}
%systemd_postun_with_restart gssproxy.service
%changelog
* Fri Oct 27 2017 Robbie Harwood <rharwood@redhat.com> - 0.7.0-22
- Fix concurrency issue in server socket handling
* Mon Oct 02 2017 Robbie Harwood <rharwood@redhat.com> - 0.7.0-21
- Off-by-one error fix in selinux-policy version