Should libcontainer automatically reap runc:[1:CHILD]?

[LittleLightLittleFire]

This would mean there is no need to install a subreaper.

This avoids boobytrapping anyone trying to use os.Exec and would mean downstream projects like containerd no longer have to have a custom reaper to prevent race conditions with wait(2).

Something like this perhaps?

diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go
index 99cc02c..282d0c7 100644
--- a/libcontainer/init_linux.go
+++ b/libcontainer/init_linux.go
@@ -29,7 +29,8 @@ const (
 )
 
 type pid struct {
-	Pid int `json:"pid"`
+	Pid  int `json:"pid"`
+	Pid2 int `json:"pid2"`
 }
 
 // network is an internal struct used to setup container networks.
diff --git a/libcontainer/nsenter/nsexec.c b/libcontainer/nsenter/nsexec.c
index 0ad6883..01ef2a8 100644
--- a/libcontainer/nsenter/nsexec.c
+++ b/libcontainer/nsenter/nsexec.c
@@ -543,7 +543,7 @@ void nsexec(void)
 	 */
 	case JUMP_PARENT: {
 			int len;
-			pid_t child;
+			pid_t child, old_child = -1;
 			char buf[JSON_MAX];
 			bool ready = false;
 
@@ -607,18 +607,18 @@ void nsexec(void)
 					}
 					break;
 				case SYNC_RECVPID_PLS: {
-						pid_t old = child;
+						old_child = child;
 
 						/* Get the init_func pid. */
 						if (read(syncfd, &child, sizeof(child)) != sizeof(child)) {
-							kill(old, SIGKILL);
+							kill(old_child, SIGKILL);
 							bail("failed to sync with child: read(childpid)");
 						}
 
 						/* Send ACK. */
 						s = SYNC_RECVPID_ACK;
 						if (write(syncfd, &s, sizeof(s)) != sizeof(s)) {
-							kill(old, SIGKILL);
+							kill(old_child, SIGKILL);
 							kill(child, SIGKILL);
 							bail("failed to sync with child: write(SYNC_RECVPID_ACK)");
 						}
@@ -667,7 +667,7 @@ void nsexec(void)
 			}
 
 			/* Send the init_func pid back to our parent. */
-			len = snprintf(buf, JSON_MAX, "{\"pid\": %d}\n", child);
+			len = snprintf(buf, JSON_MAX, "{\"pid\": %d, \"pid2\": %d}\n", child, old_child);
 			if (len < 0) {
 				kill(child, SIGKILL);
 				bail("unable to generate JSON for child pid");
diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go
index bfe9955..aca2648 100644
--- a/libcontainer/process_linux.go
+++ b/libcontainer/process_linux.go
@@ -138,6 +138,14 @@ func (p *setnsProcess) execSetns() error {
 		p.cmd.Wait()
 		return newSystemErrorWithCause(err, "reading pid from init pipe")
 	}
+
+	// Clean up the zombie parent process if it exists
+	parentProcess, err := os.FindProcess(pid.Pid2)
+	if err != nil {
+		return err
+	}
+	_, _ = parentProcess.Wait()
+
 	process, err := os.FindProcess(pid.Pid)
 	if err != nil {
 		return err
@@ -221,6 +229,14 @@ func (p *initProcess) execSetns() error {
 		p.cmd.Wait()
 		return err
 	}
+
+	// Clean up the zombie parent process if it exists
+	parentProcess, err := os.FindProcess(pid.Pid2)
+	if err != nil {
+		return err
+	}
+	_, _ = parentProcess.Wait()
+
 	process, err := os.FindProcess(pid.Pid)
 	if err != nil {
 		return err
-- 
2.12.2

[cyphar]

This avoids boobytrapping anyone trying to use os.Exec and would mean downstream projects like containerd no longer have to have a custom reaper to prevent race conditions with wait(2).

To be clear, containerd would still need a reaper for the reasons I discussed with you offline (specifically when you do the equivalent of runc exec).

[LittleLightLittleFire]

To be clear, containerd would still need a reaper for the reasons I discussed with you offline (specifically when you do the equivalent of runc exec).

That is true it is still is wise for them to have to subreaper in case some zombies aren't cleaned up, but those would be fairly rare cases and is also conditional on how they use the init process.

[crosbymichael]

We already register a subreaper in containerd and have it running from boot. containerd should not require any changes.

[jordy25519]

I found @LittleLightLittleFire 's solution to be useful. I'm launching containers from a long-lived daemon process and the only examples of sub reaper implementations I found would reap every process. This caused problems if new processes were started during reaping.

[cyphar]

Yeah, I'll write a patch based on this in the coming days (I've been busy in the past few weeks with exams).

[LittleLightLittleFire]

Oh I can repurpose the patch into a pull request

…

On Wed, 28 Jun 2017 at 2:34 pm, Aleksa Sarai ***@***.***> wrote: Yeah, I'll write a patch based on this in the coming days (I've been busy in the past few weeks with exams). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1443 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABHKxpvbzTFCerMhohqX7l1DXHciwQ5cks5sIdfogaJpZM4NWFSs> .