URL: https://github.com/SSSD/sssd/pull/107
Title: #107: WATCHDOG: Avoid non async-signal-safe from the signal_handler
lslebodn commented:
"""
I checked the man page and API on linux and BSD and setpgrp has a different API
on these platforms.
But there is similar function which do the same and has the same API on both
platforms. `setpgid()`.
It would be good if you could change it. It will reduce some platform specific
ifdefs. And we needn't change SELinux policy twice from setpgrp -> setpgid.
ATM; there is a comment that we should fail after failure in setpgrp after
fixed SElinux policy.
But it would cause a problems on older distributions where we do not plan to
rebase sssd and therefore SELinux policy would not be updated. Upstream version
would not work there.
I think we should just scream to log file and inform users that it can leak
some child processes in rare cases. And we should use -getpid() in watchdog
handler. It will kill whole process groups if setpgrp/setpgid passed and only
the process if it such process group does not exist.
Here is a diff; feel free to rephrase some comments
```
diff --git a/src/util/server.c b/src/util/server.c
index 60019a7d3..9d1af6a0a 100644
--- a/src/util/server.c
+++ b/src/util/server.c
@@ -460,16 +460,16 @@ int server_setup(const char *name, int flags,
struct logrotate_ctx *lctx;
char *locale;
int watchdog_interval;
+ pid_t my_pid;
- ret = setpgrp();
+ my_pid = getpid();
+ ret = setpgid(my_pid, my_pid);
if (ret != EOK) {
ret = errno;
DEBUG(SSSDBG_MINOR_FAILURE,
- "Failed setting process group: %s[ %d]\n",
+ "Failed setting process group: %s[ %d]. "
+ "We might leak processes in case of failure\n",
sss_strerror(ret), ret);
-
- /* Do not abort here till SELinux policies are fixed,
- * otherwise SSSD won't start up. */
}
ret = chown_debug_file(NULL, uid, gid);
diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c
index 044e87f6e..68f055f3b 100644
--- a/src/util/util_watchdog.c
+++ b/src/util/util_watchdog.c
@@ -50,7 +50,12 @@ static void watchdog_detect_timeshift(void)
if (cur_time < prev_time) {
/* Time shift detected. We need to restart watchdog. */
if (write(watchdog_ctx.pipefd[1], "1", 1) != 1) {
- kill(-getpgrp(), SIGTERM);
+ /* getpid() should return the same value as getpgrp()
+ * If they are not the same then setpgrp()/setpgid() failed
+ * and we should not kill whole process group but only current
+ * process.
+ */
+ kill(-getpid(), SIGTERM);
}
}
}
```
"""
See the full comment at
https://github.com/SSSD/sssd/pull/107#issuecomment-271523487
_______________________________________________
sssd-devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]
