On 08/09/2016 11:26 AM, Jakub Hrozek wrote:
On Mon, Aug 08, 2016 at 09:46:55AM +0200, Petr Cech wrote:
> On 08/04/2016 05:01 PM, Petr Cech wrote:
> > On 08/04/2016 04:35 PM, Petr Cech wrote:
> > > Hi list,
> > >
> > > there is the first version of patch for [1]. I need
> > > to investigate if we have the same issue in other
> > > *_childs.
> > >
> > > I tested it with sleep() added into ldap_child code.
> > >
> > > [1] https://fedorahosted.org/sssd/ticket/3106
> > >
> >
> > I know, it is not perfect. For example 2 seconds between SIGTERM and
> > SIGKILL should be constant. I will send the second version with other
> > childs, but not today.
> >
> > Regards
>
> Hello all,
>
> there is fixed patch set. There is only one minor change -- constant for
> time between signals.
>
> Is there better name for global_ccname_file_dummy? I did not want to break
> the naming... but it looks really long.
I think this name is fine, but I would prefer to not use the variable in
the code, except for setting it after the temporary file is created and
then set the variable back to NULL after the temporary file is renamed
to the real one.
Addressed.
Thanks Jakub, it looks better. :-)
> I looked at code of others children. I saw the same schema at
> src/providers/ad/ad_gpo_child.c:361 (temporary file renamed to solid one).
>
> In my opinion we could fix ad_qpo_child in next patch (not the highest
> priority for now) and make SIGTERM/SIGKILL more general. Maybe all child
> processes could have it.
>
> Regards
>
> --
> Petr^4 Čech
--
Petr^4 Čech
>From 4a79f2911168e4cafe78bc201f9961eea846d15d Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@redhat.com>
Date: Thu, 4 Aug 2016 16:25:28 +0200
Subject: [PATCH 1/2] LDAP: Adding support for SIGTERM signal
We add support for handling SIGTERM signal. If ldap_child receives
SIGTERM signal it removes temporary file.
Resolves:
https://fedorahosted.org/sssd/ticket/3106
---
src/providers/ldap/ldap_child.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
index 52c271f36cb57b774e6091e7322e4b1394c78969..2edfc30ec7b4a3ff03d86ae41f086a6128451a9b 100644
--- a/src/providers/ldap/ldap_child.c
+++ b/src/providers/ldap/ldap_child.c
@@ -33,6 +33,30 @@
#include "providers/backend.h"
#include "providers/krb5/krb5_common.h"
+char *global_ccname_file_dummy = NULL;
+
+static void sig_term_handler(int sig)
+{
+ int ret;
+
+ DEBUG(SSSDBG_FATAL_FAILURE, "Received signal [%s] [%i], shutting down\n",
+ strsignal(sig), sig);
+
+ if (global_ccname_file_dummy != NULL) {
+ ret = unlink(global_ccname_file_dummy);
+ if (ret != 0) {
+ DEBUG(SSSDBG_FATAL_FAILURE, "Unlink file [%s] failed [%i][%s]\n",
+ global_ccname_file_dummy,
+ errno, strerror(errno));
+ } else {
+ DEBUG(SSSDBG_FATAL_FAILURE, "Unlink file [%s]\n",
+ global_ccname_file_dummy);
+ }
+ }
+
+ _exit(0);
+}
+
static krb5_context krb5_error_ctx;
#define LDAP_CHILD_DEBUG(level, error) KRB5_DEBUG(level, krb5_error_ctx, error)
@@ -405,6 +429,7 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
strerror(krberr), krberr);
goto done;
}
+ global_ccname_file_dummy = ccname_file_dummy;
ret = sss_unique_filename(tmp_ctx, ccname_file_dummy);
if (ret != EOK) {
@@ -490,6 +515,7 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
"rename failed [%d][%s].\n", ret, strerror(ret));
goto done;
}
+ global_ccname_file_dummy = NULL;
krberr = 0;
*ccname_out = talloc_steal(memctx, ccname);
@@ -631,6 +657,9 @@ int main(int argc, const char *argv[])
}
}
+ BlockSignals(false, SIGTERM);
+ CatchSignal(SIGTERM, sig_term_handler);
+
DEBUG(SSSDBG_TRACE_FUNC, "ldap_child started.\n");
main_ctx = talloc_new(NULL);
--
2.7.4
>From 221bf0bea9293a0192426144aaa7d39682300bb9 Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@redhat.com>
Date: Thu, 4 Aug 2016 16:27:40 +0200
Subject: [PATCH 2/2] LDAP: Adding SIGTERM signal before SIGKILL
We add better termination of ldap_child. If ldap_child reaches
the timeout for termination parent sents SIGTERM signal. Child
has 2 seconds for removing temporary file and exit.
If it is not sufficient there is SIGKILL send to the child.
Resolves:
https://fedorahosted.org/sssd/ticket/3106
---
src/providers/ldap/sdap_child_helpers.c | 39 +++++++++++++++++++++++++++++----
src/util/child_common.h | 2 ++
2 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/src/providers/ldap/sdap_child_helpers.c b/src/providers/ldap/sdap_child_helpers.c
index 92642e8e47e530e5aef5f78bf1c7a2427979275e..9f9c9b57a7ca5e1217d56ab560cc941432084df8 100644
--- a/src/providers/ldap/sdap_child_helpers.c
+++ b/src/providers/ldap/sdap_child_helpers.c
@@ -416,9 +416,7 @@ int sdap_get_tgt_recv(struct tevent_req *req,
return EOK;
}
-
-
-static void get_tgt_timeout_handler(struct tevent_context *ev,
+static void get_tgt_sigkill_handler(struct tevent_context *ev,
struct tevent_timer *te,
struct timeval tv, void *pvt)
{
@@ -428,7 +426,8 @@ static void get_tgt_timeout_handler(struct tevent_context *ev,
int ret;
DEBUG(SSSDBG_TRACE_ALL,
- "timeout for tgt child [%d] reached.\n", state->child->pid);
+ "timeout for sending SIGKILL to tgt child [%d] reached.\n",
+ state->child->pid);
ret = kill(state->child->pid, SIGKILL);
if (ret == -1) {
@@ -439,6 +438,38 @@ static void get_tgt_timeout_handler(struct tevent_context *ev,
tevent_req_error(req, ETIMEDOUT);
}
+static void get_tgt_timeout_handler(struct tevent_context *ev,
+ struct tevent_timer *te,
+ struct timeval tv, void *pvt)
+{
+ struct tevent_req *req = talloc_get_type(pvt, struct tevent_req);
+ struct sdap_get_tgt_state *state = tevent_req_data(req,
+ struct sdap_get_tgt_state);
+ int ret;
+
+ DEBUG(SSSDBG_TRACE_ALL,
+ "timeout for sending SIGTERM to tgt child [%d] reached.\n",
+ state->child->pid);
+
+ ret = kill(state->child->pid, SIGTERM);
+ if (ret == -1) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Sending SIGTERM failed [%d][%s].\n", errno, strerror(errno));
+ }
+
+ DEBUG(SSSDBG_TRACE_FUNC,
+ "Setting %d seconds timeout for sending SIGKILL to tgt child\n",
+ SIGTERM_TO_SIGKILL_TIME);
+
+ tv = tevent_timeval_current_ofs(SIGTERM_TO_SIGKILL_TIME, 0);
+
+ te = tevent_add_timer(ev, req, tv, get_tgt_sigkill_handler, req);
+ if (te == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "tevent_add_timer failed.\n");
+ tevent_req_error(req, ECANCELED);
+ }
+}
+
static errno_t set_tgt_child_timeout(struct tevent_req *req,
struct tevent_context *ev,
int timeout)
diff --git a/src/util/child_common.h b/src/util/child_common.h
index 2a62869034a466b465a481286950678af73667ab..eb7623987238d5d9e9032b9bbbbef49188f5b1d2 100644
--- a/src/util/child_common.h
+++ b/src/util/child_common.h
@@ -35,6 +35,8 @@
#define IN_BUF_SIZE 512
#define CHILD_MSG_CHUNK 256
+#define SIGTERM_TO_SIGKILL_TIME 2
+
struct response {
uint8_t *buf;
size_t size;
--
2.7.4
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
