On Mon, 2012-06-18 at 09:33 -0400, Stephen Gallagher wrote: > On Mon, 2012-06-18 at 06:30 -0700, Shantanu Goel wrote: > > Hi Stephen, > > > > > > Please feel free to modify the patch in any way or shape you deem > > necessary for inclusion. We are just glad that you agree there is a > > real problem which needs fixing. One thing I ask is if you expect to > > have rhel 5 or 6 test RPMs that we could test with the ultimate fix > > any time soon, please drop me a note and we will gladly install them > > on some of our problematic machines here to see if they address the > > problems we have seen. > > Sure, once this is done I'm going to be committing it upstream for the > master branch (future 1.9), the sssd-1-8 branch (our current LTM > release) and the sssd-1-5 branch (our previous LTM release). > > You should be able to pull the patches from the sssd-1-5 branch and > build them for your systems once they're ready.
Ok, new patches attached. Shantanu, these are currently designed for the master branch. We'll get them committed there first and tested out for a little while, then we'll backport them. Patch 0001: Return the correct errno value. Previously it could have been reset by closing the socket. Patch 0002: Add some additional debugging to the client_destructor() Patch 0003: On systems that support MSG_NOSIGNAL, we should use it. This way, if a client app isn't configured to listen for SIGPIPE, it will not crash. Patch 0004: Add a timer to each client context. If sixty seconds pass (configurable in the patch 0005) without either read or write activity, we will free the client context and close the socket. The client code is already written to be tolerant of this and will reconnect on the next request. This will help us avoid resource exhaustion if we have clients that hang on to NSS and PAM file descriptors indefinitely (like 'su' and 'login' do for PAM). Patch 0005: Make the client idle timeout value configurable and add it to the manpages and config API.
From 488bdb948f35fd155a478fc8634ad1d77eb259d2 Mon Sep 17 00:00:00 2001
From: Shantanu Goel <sg...@trade4.test-jc.tower-research.com>
Date: Mon, 18 Jun 2012 08:45:32 -0400
Subject: [PATCH 1/5] Set return errno to the value prior to calling close().
---
src/sss_client/common.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/sss_client/common.c b/src/sss_client/common.c
index 28adb4424dfc72dab79ca00fc905152a7cff3e8a..90713d5b25785a137f1d0f03689c8877ede12d28 100644
--- a/src/sss_client/common.c
+++ b/src/sss_client/common.c
@@ -154,7 +154,7 @@ static enum sss_status sss_cli_send_req(enum sss_cli_command cmd,
/* Write failed */
sss_cli_close_socket();
- *errnop = errno;
+ *errnop = error;
return SSS_STATUS_UNAVAIL;
}
@@ -264,7 +264,7 @@ static enum sss_status sss_cli_recv_rep(enum sss_cli_command cmd,
* through. */
sss_cli_close_socket();
- *errnop = errno;
+ *errnop = error;
ret = SSS_STATUS_UNAVAIL;
goto failed;
}
--
1.7.10.2
From 4bb4d571cb597d7b444ae75f517d3dc57306c398 Mon Sep 17 00:00:00 2001
From: Shantanu Goel <sg...@trade4.test-jc.tower-research.com>
Date: Mon, 18 Jun 2012 08:49:10 -0400
Subject: [PATCH 2/5] Log message if close() fails in destructor.
---
src/responder/common/responder_common.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index 2e3e98a9cceec66cdac200d93274c6f3cea41f0c..5d5248809d095420e8a4d11ce80fc13e1949c873 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -87,7 +87,18 @@ static errno_t set_close_on_exec(int fd)
static int client_destructor(struct cli_ctx *ctx)
{
- if (ctx->cfd > 0) close(ctx->cfd);
+ errno_t ret;
+
+ if ((ctx->cfd > 0) && close(ctx->cfd) < 0) {
+ ret = errno;
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Failed to close fd [%d]: [%s]\n",
+ ctx->cfd, strerror(ret)));
+ }
+
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ ("Terminated client [%p][%d]\n",
+ ctx, ctx->cfd));
return 0;
}
--
1.7.10.2
From db6ae22f6dba8b812cf6ea9c3d86d2ff35acd416 Mon Sep 17 00:00:00 2001
From: Shantanu Goel <sg...@trade4.test-jc.tower-research.com>
Date: Mon, 18 Jun 2012 09:26:45 -0400
Subject: [PATCH 3/5] Do not send SIGPIPE on disconnection
Note we set MSG_NOSIGNAL to avoid
having to fiddle with signal masks
but also do not want to die in case
SIGPIPE gets raised and the application
does not handle it.
---
src/sss_client/common.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/src/sss_client/common.c b/src/sss_client/common.c
index 90713d5b25785a137f1d0f03689c8877ede12d28..80265e301590dee6be081984f203dce25ed594e7 100644
--- a/src/sss_client/common.c
+++ b/src/sss_client/common.c
@@ -49,6 +49,19 @@
#include <pthread.h>
#endif
+/*
+* Note we set MSG_NOSIGNAL to avoid
+* having to fiddle with signal masks
+* but also do not want to die in case
+* SIGPIPE gets raised and the application
+* does not handle it.
+*/
+#ifdef MSG_NOSIGNAL
+#define sss_write(sockfd, buf, len) send(sockfd, buf, len, MSG_NOSIGNAL)
+#else
+#define sss_write(sockfd, buf, len) write(sockfd, buf, len)
+#endif
+
/* common functions */
int sss_cli_sd = -1; /* the sss client socket descriptor */
@@ -133,14 +146,14 @@ static enum sss_status sss_cli_send_req(enum sss_cli_command cmd,
errno = 0;
if (datasent < SSS_NSS_HEADER_SIZE) {
- res = write(sss_cli_sd,
- (char *)header + datasent,
- SSS_NSS_HEADER_SIZE - datasent);
+ res = sss_write(sss_cli_sd,
+ (char *)header + datasent,
+ SSS_NSS_HEADER_SIZE - datasent);
} else {
rdsent = datasent - SSS_NSS_HEADER_SIZE;
- res = write(sss_cli_sd,
- (const char *)rd->data + rdsent,
- rd->len - rdsent);
+ res = sss_write(sss_cli_sd,
+ (const char *)rd->data + rdsent,
+ rd->len - rdsent);
}
error = errno;
--
1.7.10.2
From 11d7586ebfacf05d74441b8e355cda14cea1fd00 Mon Sep 17 00:00:00 2001
From: Shantanu Goel <sg...@trade4.test-jc.tower-research.com>
Date: Mon, 18 Jun 2012 10:54:44 -0400
Subject: [PATCH 4/5] Add support for terminating idle connections
---
src/responder/common/responder.h | 2 +
src/responder/common/responder_common.c | 67 ++++++++++++++++++++++++++++++-
2 files changed, 67 insertions(+), 2 deletions(-)
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 2cc85445c6d802ba2de275c143c8aeccd2eb5ece..67884ed7659d3d99ac8d693ad083b4f7e293bf71 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -126,6 +126,8 @@ struct cli_ctx {
int netgrent_cur;
char *automntmap_name;
+
+ struct tevent_timer *idle;
};
struct sss_cmd_table {
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index 5d5248809d095420e8a4d11ce80fc13e1949c873..3c269725256c88cfad65a0fc9f1b0d4ac4009797 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -219,12 +219,24 @@ static void client_recv(struct cli_ctx *cctx)
return;
}
+static errno_t reset_idle_timer(struct cli_ctx *cctx);
+
static void client_fd_handler(struct tevent_context *ev,
struct tevent_fd *fde,
uint16_t flags, void *ptr)
{
+ errno_t ret;
struct cli_ctx *cctx = talloc_get_type(ptr, struct cli_ctx);
+ /* Always reset the idle timer on any activity */
+ ret = reset_idle_timer(cctx);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Could not create idle timer for client. "
+ "This connection may not auto-terminate\n"));
+ /* Non-fatal, continue */
+ }
+
if (flags & TEVENT_FD_READ) {
client_recv(cctx);
return;
@@ -240,6 +252,11 @@ struct accept_fd_ctx {
bool is_private;
};
+static void idle_handler(struct tevent_context *ev,
+ struct tevent_timer *te,
+ struct timeval current_time,
+ void *data);
+
static void accept_fd_handler(struct tevent_context *ev,
struct tevent_fd *fde,
uint16_t flags, void *ptr)
@@ -317,12 +334,58 @@ static void accept_fd_handler(struct tevent_context *ev,
talloc_set_destructor(cctx, client_destructor);
- DEBUG(4, ("Client connected%s!\n",
- accept_ctx->is_private ? " to privileged pipe" : ""));
+ /* Set up the idle timer */
+ ret = reset_idle_timer(cctx);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Could not create idle timer for client. "
+ "This connection may not auto-terminate\n"));
+ /* Non-fatal, continue */
+ }
+
+ DEBUG(SSSDBG_TRACE_FUNC,
+ ("Client connected%s!\n",
+ accept_ctx->is_private ? " to privileged pipe" : ""));
return;
}
+static errno_t reset_idle_timer(struct cli_ctx *cctx)
+{
+ struct timeval tv;
+
+ /* TODO: make this configurable */
+ tv = tevent_timeval_current_ofs(60, 0);
+
+ talloc_zfree(cctx->idle);
+
+ cctx->idle = tevent_add_timer(cctx->ev, cctx, tv, idle_handler, cctx);
+ if (!cctx->idle) return ENOMEM;
+
+ DEBUG(SSSDBG_TRACE_ALL,
+ ("Idle timer re-set for client [%p][%d]\n",
+ cctx, cctx->cfd));
+
+ return EOK;
+}
+
+static void idle_handler(struct tevent_context *ev,
+ struct tevent_timer *te,
+ struct timeval current_time,
+ void *data)
+{
+ /* This connection is idle. Terminate it */
+ struct cli_ctx *cctx =
+ talloc_get_type(data, struct cli_ctx);
+
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ ("Terminating idle client [%p][%d]\n",
+ cctx, cctx->cfd));
+
+ /* The cli_ctx destructor will handle the rest */
+ talloc_free(cctx);
+}
+
static int sss_dp_init(struct resp_ctx *rctx,
struct sbus_interface *intf,
const char *cli_name,
--
1.7.10.2
From 86367ed64e7e83ba15a19cbc9e1c41773a029b1b Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Mon, 18 Jun 2012 11:23:04 -0400
Subject: [PATCH 5/5] Make the client idle timeout configurable
---
src/confdb/confdb.h | 2 ++
src/config/SSSDConfig/__init__.py.in | 1 +
src/config/SSSDConfigTest.py | 3 ++-
src/config/etc/sssd.api.conf | 1 +
src/man/sssd.conf.5.xml | 15 +++++++++++++++
src/responder/common/responder.h | 1 +
src/responder/common/responder_common.c | 22 ++++++++++++++++++----
7 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
index db247b184d2dbecdd281d16d7b5bc9659bb1cfc3..3fa8b0377d701548e737ae413925358fda6b6884 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -72,6 +72,8 @@
/* Responders */
#define CONFDB_RESPONDER_GET_DOMAINS_TIMEOUT "get_domains_timeout"
+#define CONFDB_RESPONDER_CLI_IDLE_TIMEOUT "client_idle_timeout"
+#define CONFDB_RESPONDER_CLI_IDLE_DEFAULT_TIMEOUT 60
/* NSS */
#define CONFDB_NSS_CONF_ENTRY "config/nss"
diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
index d7895b49af9e6c9a3b48bb282679c61d6ebbfa6a..b90f505d110bf293e8b65964a38c7d620e779a11 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -45,6 +45,7 @@ option_strings = {
'command' : _('Command to start service'),
'reconnection_retries' : _('Number of times to attempt connection to Data Providers'),
'fd_limit' : _('The number of file descriptors that may be opened by this responder'),
+ 'client_idle_timeout' : _('Idle time before automatic disconnection of a client'),
# [sssd]
'services' : _('SSSD Services to start'),
diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py
index 1e1fe98e6850f7539579db00fc0bd96d2a865c9b..ef696e98c0b1ccce3dccda8ead7a3bda10049cf8 100755
--- a/src/config/SSSDConfigTest.py
+++ b/src/config/SSSDConfigTest.py
@@ -274,7 +274,8 @@ class SSSDConfigTestSSSDService(unittest.TestCase):
'debug_to_files',
'command',
'reconnection_retries',
- 'fd_limit']
+ 'fd_limit',
+ 'client_idle_timeout']
self.assertTrue(type(options) == dict,
"Options should be a dictionary")
diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
index e09a8bf033f38144cd9824e2592728c2898e3db2..c3d6fa8119213c874c5f9f1d5f77746a0e1fc88f 100644
--- a/src/config/etc/sssd.api.conf
+++ b/src/config/etc/sssd.api.conf
@@ -10,6 +10,7 @@ debug_to_files = bool, None, false
command = str, None, false
reconnection_retries = int, None, false
fd_limit = int, None, false
+client_idle_timeout = int, None, false
[sssd]
# Monitor service
diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index 1dae3ccbcbef6ac73c2bfae82a1ab04251e84259..bdf2543b7900aed7b4e75999a4088f91985fe69b 100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -303,6 +303,21 @@
</para>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>client_idle_timeout</term>
+ <listitem>
+ <para>
+ This option specifies the number of seconds that
+ a client of an SSSD process can hold onto a file
+ descriptor without communicating on it. This value
+ is limited in order to avoid resource exhasution
+ on the system.
+ </para>
+ <para>
+ Default: 60
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</refsect2>
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 67884ed7659d3d99ac8d693ad083b4f7e293bf71..43a4fa0236c313c3cdeb2a0b181ffd4c96167bb7 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -87,6 +87,7 @@ struct resp_ctx {
struct sss_domain_info *domains;
int domains_timeout;
+ int client_idle_timeout;
struct sysdb_ctx_list *db_list;
struct sss_cmd_table *sss_cmds;
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index 3c269725256c88cfad65a0fc9f1b0d4ac4009797..242fae996634469cb6a4e29c137613fb4bf19a03 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -352,10 +352,8 @@ static void accept_fd_handler(struct tevent_context *ev,
static errno_t reset_idle_timer(struct cli_ctx *cctx)
{
- struct timeval tv;
-
- /* TODO: make this configurable */
- tv = tevent_timeval_current_ofs(60, 0);
+ struct timeval tv =
+ tevent_timeval_current_ofs(cctx->rctx->client_idle_timeout, 0);
talloc_zfree(cctx->idle);
@@ -620,6 +618,22 @@ int sss_process_init(TALLOC_CTX *mem_ctx,
rctx->confdb_service_path = confdb_service_path;
ret = confdb_get_int(rctx->cdb, rctx->confdb_service_path,
+ CONFDB_RESPONDER_CLI_IDLE_TIMEOUT,
+ CONFDB_RESPONDER_CLI_IDLE_DEFAULT_TIMEOUT,
+ &rctx->client_idle_timeout);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ ("Cannot get the client idle timeout [%d]: %s\n",
+ ret, strerror(ret)));
+ return ret;
+ }
+
+ /* Ensure that the client timeout is at least ten seconds */
+ if (rctx->client_idle_timeout < 10) {
+ rctx->client_idle_timeout = 10;
+ }
+
+ ret = confdb_get_int(rctx->cdb, rctx->confdb_service_path,
CONFDB_RESPONDER_GET_DOMAINS_TIMEOUT,
GET_DOMAINS_DEFAULT_TIMEOUT, &rctx->domains_timeout);
if (ret != EOK) {
--
1.7.10.2
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
