From 91f6eabe49553bef060b243161b7008a3ae387a8 Mon Sep 17 00:00:00 2001 From: KLuka Date: Fri, 6 Mar 2015 13:41:39 +0100 Subject: [PATCH] Issue #103, #203: Child process termination (partial fix) When browser tab/window is closed during active session, child process stays alive forever (even if shellinaboxd is terminated). This fix works only if shellinaboxd is started without root privileges. Droping them at runtime doesn't help either. Issue is related to PAM session management process. If we start shellinaboxd with root priviliges this fix will not affect anything. * When session timeouts cleanup procedure is triggered. Procedure is executed in launcher process, because this is parent of child (service) process. There we execute checks, if we have correct child pid (stored in session) and than we can terminate process. * Added debug information about cleaning up child process https://code.google.com/p/shellinabox/issues/detail?id=103 https://code.google.com/p/shellinabox/issues/detail?id=203 --- shellinabox/launcher.c | 50 ++++++++++++++++++++++++++++++++++++-- shellinabox/launcher.h | 12 +++++---- shellinabox/session.c | 2 ++ shellinabox/session.h | 2 ++ shellinabox/shellinaboxd.c | 6 ++++- 5 files changed, 64 insertions(+), 8 deletions(-) diff --git a/shellinabox/launcher.c b/shellinabox/launcher.c index 69e29e6..835969e 100644 --- a/shellinabox/launcher.c +++ b/shellinabox/launcher.c @@ -520,6 +520,7 @@ int launchChild(int service, struct Session *session, const char *url) { ssize_t len = sizeof(struct LaunchRequest) + strlen(u) + 1; check(request = calloc(len, 1)); request->service = service; + request->terminate = -1; request->width = session->width; request->height = session->height; strncat(request->peerName, httpGetPeerName(session->http), @@ -547,6 +548,7 @@ int launchChild(int service, struct Session *session, const char *url) { return -1; } check(bytes == sizeof(pid)); + check(session->pid = pid); struct cmsghdr *cmsg = CMSG_FIRSTHDR(&msg); check(cmsg); check(cmsg->cmsg_level == SOL_SOCKET); @@ -555,6 +557,34 @@ int launchChild(int service, struct Session *session, const char *url) { return pid; } +int terminateChild(struct Session *session) { + if (launcher < 0) { + errno = EINVAL; + return -1; + } + + if (session->pid < 1) { + debug("Child pid for termination not valid!"); + return -1; + } + + // Send terminate request to launcher process + struct LaunchRequest *request; + ssize_t len = sizeof(struct LaunchRequest); + check(request = calloc(len, 1)); + request->terminate = session->pid; + if (NOINTR(write(launcher, request, len)) != len) { + debug("Child %d termination request failed!", request->terminate); + free(request); + return -1; + } + + free(request); + session->pid = 0; + session->cleanup = 0; + return 0; +} + struct Utmp { const char pid[32]; int pty; @@ -1616,7 +1646,7 @@ static void launcherDaemon(int fd) { int status; pid_t pid; while (NOINTR(pid = waitpid(-1, &status, WNOHANG)) > 0) { - debug("Child %d exited with exit code %d\n", pid, WEXITSTATUS(status)); + debug("Child %d exited with exit code %d", pid, WEXITSTATUS(status)); if (WIFEXITED(status) || WIFSIGNALED(status)) { char key[32]; snprintf(&key[0], sizeof(key), "%d", pid); @@ -1627,6 +1657,21 @@ static void launcherDaemon(int fd) { continue; } + // Check if we received terminate request from parent process and + // try to terminate child, if child is still running + if (request.terminate > 0) { + errno = 0; + NOINTR(pid = waitpid(request.terminate, &status, WNOHANG)); + if (pid == 0 && errno == 0) { + if (kill(request.terminate, SIGTERM) == 0) { + debug("Terminating child %d (kill)", request.terminate); + } else { + debug("Terminating child failed [%s]", strerror(errno)); + } + } + continue; + } + char *url; check(url = calloc(request.urlLength + 1, 1)); readURL: @@ -1637,7 +1682,7 @@ static void launcherDaemon(int fd) { break; } while (NOINTR(pid = waitpid(-1, &status, WNOHANG)) > 0) { - debug("Child %d exited with exit code %d\n", pid, WEXITSTATUS(status)); + debug("Child %d exited with exit code %d", pid, WEXITSTATUS(status)); if (WIFEXITED(status) || WIFSIGNALED(status)) { char key[32]; snprintf(&key[0], sizeof(key), "%d", pid); @@ -1682,6 +1727,7 @@ static void launcherDaemon(int fd) { childProcesses = newHashMap(destroyUtmpHashEntry, NULL); } addToHashMap(childProcesses, utmp->pid, (char *)utmp); + debug("Child %d launched", pid); } else { int fds[2]; if (!pipe(fds)) { diff --git a/shellinabox/launcher.h b/shellinabox/launcher.h index 945bccc..2d7d145 100644 --- a/shellinabox/launcher.h +++ b/shellinabox/launcher.h @@ -52,15 +52,17 @@ struct LaunchRequest { - int service; - int width, height; - char peerName[128]; - int urlLength; - char url[0]; + int service; + int width, height; + pid_t terminate; + char peerName[128]; + int urlLength; + char url[0]; }; int supportsPAM(void); int launchChild(int service, struct Session *session, const char *url); +int terminateChild(struct Session *session); void setWindowSize(int pty, int width, int height); int forkLauncher(void); void terminateLauncher(void); diff --git a/shellinabox/session.c b/shellinabox/session.c index f18203f..25f5513 100644 --- a/shellinabox/session.c +++ b/shellinabox/session.c @@ -121,6 +121,8 @@ void initSession(struct Session *session, const char *sessionKey, session->height = 0; session->buffered = NULL; session->len = 0; + session->pid = 0; + session->cleanup = 0; } struct Session *newSession(const char *sessionKey, Server *server, URL *url, diff --git a/shellinabox/session.h b/shellinabox/session.h index bd54963..11d2281 100644 --- a/shellinabox/session.h +++ b/shellinabox/session.h @@ -63,6 +63,8 @@ struct Session { int height; char *buffered; int len; + pid_t pid; + int cleanup; }; void addToGraveyard(struct Session *session); diff --git a/shellinabox/shellinaboxd.c b/shellinabox/shellinaboxd.c index d0d787b..ba734e1 100644 --- a/shellinabox/shellinaboxd.c +++ b/shellinabox/shellinaboxd.c @@ -274,8 +274,11 @@ static int completePendingRequest(struct Session *session, } static void sessionDone(void *arg) { - debug("Child terminated"); struct Session *session = (struct Session *)arg; + debug("Session %s done", session->sessionKey); + if (session->cleanup) { + terminateChild(session); + } session->done = 1; addToGraveyard(session); completePendingRequest(session, "", 0, INT_MAX); @@ -301,6 +304,7 @@ static int handleSession(struct ServerConnection *connection, void *arg, if (bytes || timedOut) { if (!session->http && timedOut) { debug("Timeout. Closing session."); + session->cleanup = 1; return 0; } check(!session->done);