From bb4e77686dfdae72dbaf1fa626519759f03de431 Mon Sep 17 00:00:00 2001
From: Wiktor Garbacz <wiktorg@google.com>
Date: Fri, 27 Jul 2018 13:33:39 +0200
Subject: [PATCH] subproc: reap processes after killing

Always try to release resources if possible.

Fixes #69
---
 nsjail.cc  |  4 +--
 subproc.cc | 72 +++++++++++++++++++++++++++++++-----------------------
 subproc.h  |  2 +-
 3 files changed, 44 insertions(+), 34 deletions(-)

diff --git a/nsjail.cc b/nsjail.cc
index 830f554..0b57033 100644
--- a/nsjail.cc
+++ b/nsjail.cc
@@ -122,7 +122,7 @@ static int listenMode(nsjconf_t* nsjconf) {
 	}
 	for (;;) {
 		if (sigFatal > 0) {
-			subproc::killAll(nsjconf);
+			subproc::killAndReapAll(nsjconf);
 			logs::logStop(sigFatal);
 			close(listenfd);
 			return EXIT_SUCCESS;
@@ -159,7 +159,7 @@ static int standaloneMode(nsjconf_t* nsjconf) {
 				subproc::displayProc(nsjconf);
 			}
 			if (sigFatal > 0) {
-				subproc::killAll(nsjconf);
+				subproc::killAndReapAll(nsjconf);
 				logs::logStop(sigFatal);
 				return (128 + sigFatal);
 			}
diff --git a/subproc.cc b/subproc.cc
index 7da5c2c..1502026 100644
--- a/subproc.cc
+++ b/subproc.cc
@@ -288,8 +288,39 @@ static void seccompViolation(nsjconf_t* nsjconf, siginfo_t* si) {
 	}
 }
 
-int reapProc(nsjconf_t* nsjconf) {
+static int reapProc(nsjconf_t* nsjconf, pid_t pid, bool should_wait = false) {
 	int status;
+
+	if (wait4(pid, &status, should_wait ? 0 : WNOHANG, NULL) == pid) {
+		cgroup::finishFromParent(nsjconf, pid);
+
+		std::string remote_txt = "[UNKNOWN]";
+		const pids_t* elem = getPidElem(nsjconf, pid);
+		if (elem) {
+			remote_txt = elem->remote_txt;
+		}
+
+		if (WIFEXITED(status)) {
+			LOG_I("PID: %d (%s) exited with status: %d, (PIDs left: %d)",
+			    pid, remote_txt.c_str(), WEXITSTATUS(status),
+			    countProc(nsjconf) - 1);
+			removeProc(nsjconf, pid);
+			return WEXITSTATUS(status);
+		}
+		if (WIFSIGNALED(status)) {
+			LOG_I(
+			    "PID: %d (%s) terminated with signal: %s (%d), (PIDs left: %d)",
+			    pid, remote_txt.c_str(),
+			    util::sigName(WTERMSIG(status)).c_str(), WTERMSIG(status),
+			    countProc(nsjconf) - 1);
+			removeProc(nsjconf, pid);
+			return 128 + WTERMSIG(status);
+		}
+	}
+	return 0;
+}
+
+int reapProc(nsjconf_t* nsjconf) {
 	int rv = 0;
 	siginfo_t si;
 
@@ -304,33 +335,7 @@ int reapProc(nsjconf_t* nsjconf) {
 		if (si.si_code == CLD_KILLED && si.si_status == SIGSYS) {
 			seccompViolation(nsjconf, &si);
 		}
-
-		if (wait4(si.si_pid, &status, WNOHANG, NULL) == si.si_pid) {
-			cgroup::finishFromParent(nsjconf, si.si_pid);
-
-			std::string remote_txt = "[UNKNOWN]";
-			const pids_t* elem = getPidElem(nsjconf, si.si_pid);
-			if (elem) {
-				remote_txt = elem->remote_txt;
-			}
-
-			if (WIFEXITED(status)) {
-				LOG_I("PID: %d (%s) exited with status: %d, (PIDs left: %d)",
-				    si.si_pid, remote_txt.c_str(), WEXITSTATUS(status),
-				    countProc(nsjconf) - 1);
-				removeProc(nsjconf, si.si_pid);
-				rv = WEXITSTATUS(status);
-			}
-			if (WIFSIGNALED(status)) {
-				LOG_I(
-				    "PID: %d (%s) terminated with signal: %s (%d), (PIDs left: %d)",
-				    si.si_pid, remote_txt.c_str(),
-				    util::sigName(WTERMSIG(status)).c_str(), WTERMSIG(status),
-				    countProc(nsjconf) - 1);
-				removeProc(nsjconf, si.si_pid);
-				rv = 128 + WTERMSIG(status);
-			}
-		}
+		rv = reapProc(nsjconf, si.si_pid);
 	}
 
 	time_t now = time(NULL);
@@ -357,9 +362,14 @@ int reapProc(nsjconf_t* nsjconf) {
 	return rv;
 }
 
-void killAll(nsjconf_t* nsjconf) {
-	for (const auto& p : nsjconf->pids) {
-		kill(p.pid, SIGKILL);
+void killAndReapAll(nsjconf_t* nsjconf) {
+	while (!nsjconf->pids.empty()) {
+		pid_t pid = nsjconf->pids.front().pid;
+		if (kill(pid, SIGKILL) == 0) {
+			reapProc(nsjconf, pid, true);
+		} else {
+			removeProc(nsjconf, pid);
+		}
 	}
 }
 
diff --git a/subproc.h b/subproc.h
index c09240f..33e2b5c 100644
--- a/subproc.h
+++ b/subproc.h
@@ -36,7 +36,7 @@ namespace subproc {
 bool runChild(nsjconf_t* nsjconf, int fd_in, int fd_out, int fd_err);
 int countProc(nsjconf_t* nsjconf);
 void displayProc(nsjconf_t* nsjconf);
-void killAll(nsjconf_t* nsjconf);
+void killAndReapAll(nsjconf_t* nsjconf);
 /* Returns the exit code of the first failing subprocess, or 0 if none fail */
 int reapProc(nsjconf_t* nsjconf);
 int systemExe(const std::vector<std::string>& args, char** env);