Sio2jail leaves zombie processes

[otargowski]

This can be observed (seems like at least on master and all sandboxed releases from 1.1.0 above) by:

1. Compiling a simple executable, like main(){}
2. Adding sth like this:

--- a/src/sio2jail.cc
+++ b/src/sio2jail.cc
@@ -2,10 +2,14 @@
 #include "s2japp/Application.h"
 
 #include <iostream>
+#include <sys/wait.h>
 
 int main(int argc, const char* argv[]) {
     try {
-        return s2j::app::Application(argc, argv).main();
+        int ret = s2j::app::Application(argc, argv).main();
+        while (wait(NULL)>0)
+            std::cerr << "bruh";
+        return ret;
     }
     catch (const std::exception& ex) {
         std::cerr << "Exception occurred: " << ex.what() << std::endl;

4. Building and running sio2jail with arbitrary (?) args, for example --mount-namespace off ~/a; adding -t 1 gives the same result
5. Witnessing the bruh

Alternatively, one can add sleep(100) before the while and check htop or sth of this nature.

One way to fix this is executing the while from above silently, though that may be more of a workaround than an actual solution.

[Wolf480pl]

Is sio2jail reporting correctly the exit status of the child process?

If it's not, then this is a big issue and we'll need to look into the main loop around waitid to see what went wrong there.

But if it gets the exit code correctly, then that's sus, because usually one would expect that if sio2jail got the exit status, it did so by waiting for the process, and thus must've reaped it.

In principle, sio2jail should reap all of its direct children (of which there should be exactly one), but if it gets the exit code correctly, and leaves some zombies that get taken care of by init anyway, then I think it doesn't hurt much.
And the waitid loop is quite unintuitive and hard to get right, I'd rather not change it unless necessary.

[otargowski]

As far as I know, the exit codes are good.
I was surprised by it too, but one can get the code and leave the zombie by using WNOWAIT (which is used in 2 out of 3 waitid calls in the loop).
TraceExecutor::onExecuteEvent also doesn't use that flag, but doesn't seem to enter the particulat if clause.
If we were to change the waitid loop after all, I'd put add a non-WNOWAIT waitid around here:

sio2jail/src/executor/Executor.cc

Lines 160 to 168 in 7be4e0e

	if (event.exited || event.killed) {
	if (event.pid == childPid_) {
	if (event.exited) {
	outputBuilder_->setExitStatus(event.exitStatus);
	}
	if (event.killed) {
	outputBuilder_->setKillSignal(event.signal);
	}
	break;

As for the init, for example in a docker container with sth like a sioworkers process instead of a proper init, the zombies will accumulate.
Therefore we should at least add the cleanup loop somewhere.

[Wolf480pl]

As for the init, for example in a docker container with sth like a sioworkers process instead of a proper init, the zombies will accumulate.

Oh, ok... one way to solve it would be to wrap sioworkers in some sort of "minimal init" - I've seen some docker containers do a similar thing, and technically that'd be the correct thing to do. But most people making docker images won't do it...

I think if we could easily fix sio2jail to not leave zombies that'd be a good idea.
But we can't add it the place you suggested.
The break; in line 168 leads past the while loop and into code that calls the onPostExecute event:

sio2jail/src/executor/Executor.cc

Lines 190 to 192 in 2239909

	for (auto& listener: eventListeners_) {
	listener->onPostExecute();
	}

This event is used by a couple listeners to read the final measurements from the dying process:

sio2jail/src/perf/PerfListener.cc

Lines 154 to 158 in 95d9e34

	void PerfListener::onPostExecute() {
	TRACE();
	outputBuilder_->setCyclesUsed(getInstructionsUsed());
	}

sio2jail/src/limits/TimeLimitListener.cc

Lines 95 to 100 in 369317c

	void TimeLimitListener::onPostExecute() {
	// TODO: run this just after child exit
	auto time = getTimeUsage();
	outputBuilder_->setRealTimeMicroseconds(time->realTimeUs);
	verifyTimeUsage(move(time));
	}

sio2jail/src/limits/TimeLimitListener.cc

Lines 144 to 145 in 369317c

	TimeLimitListener::ProcessTimeUsage TimeLimitListener::getProcessTimeUsage() const {
	std::ifstream stat("/proc/" + std::to_string(childPid_) + "/stat");

So that non-WNOWAIT wait should be at the end of executeParent() IMO, after that onPostExecute loop.

And I think adding it there is unlikely to break anything, since the only thing after that is output dumping.