Avoid race when opening exec fifo by craigfurman · Pull Request #1698 · opencontainers/runc

craigfurman · 2018-01-22T16:13:58Z

When starting a container with runc start or runc run, the stub
process (runc[2:INIT]) opens a fifo for writing. Its parent runc process
will open the same fifo for reading. In this way, they synchronize.

If the stub process exits at the wrong time, the parent runc process
will block forever.

This can happen when racing 2 runc operations against each other: runc run/start, and runc delete. It could also happen for other reasons,
e.g. the kernel's OOM killer may select the stub process.

This commit resolves this race by racing the opening of the exec fifo
from the runc parent process against the stub process exiting. If the
stub process exits before we open the fifo, we return an error.

Another solution is to wait on the stub process. However, it seems it
would require more refactoring to avoid calling wait multiple times on
the same process, which is an error.

Note: We aren't really sure how to integration test this in a sane way. In Garden, we wrote a test but it involves patching in:

diff --git a/libcontainer/standard_init_linux.go b/libcontainer/standard_init_linux.go
index 8a544ed5..84cd8765 100644
--- a/libcontainer/standard_init_linux.go
+++ b/libcontainer/standard_init_linux.go
@@ -6,7 +6,9 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	"strings"
 	"syscall" //only for Exec
+	"time"
 
 	"github.com/opencontainers/runc/libcontainer/apparmor"
 	"github.com/opencontainers/runc/libcontainer/configs"
@@ -169,6 +171,9 @@ func (l *linuxStandardInit) Init() error {
 	// user process. We open it through /proc/self/fd/$fd, because the fd that
 	// was given to us was an O_PATH fd to the fifo itself. Linux allows us to
 	// re-open an O_PATH fd through /proc.
+	if !strings.Contains(name, "init") {
+		time.Sleep(time.Hour)
+	}
 	fd, err := unix.Open(fmt.Sprintf("/proc/self/fd/%d", l.fifoFd), unix.O_WRONLY|unix.O_CLOEXEC, 0)
 	if err != nil {
 		return newSystemErrorWithCause(err, "open exec fifo")

to expose the race condition, and then performing runc run and runc delete operations. Hopefully someone has a better idea of how to get a more sensible test into runc.

[Fixes: #1697]

Cheers,
@williammartin & Craig

crosbymichael · 2018-01-22T16:18:26Z

libcontainer/container_linux.go

+		if err := readFromExecFifo(f); err != nil {
+			return err
+		}
+		return os.Remove(path)


should the file be closed first or does it not matter?

Not sure why this should matter. I suppose the fd will point to an inaccessible location on the filesystem for some amount of time, but in terms of code cleanliness, the defer seems better?

crosbymichael · 2018-01-22T16:20:52Z

libcontainer/container_linux.go

+
+func awaitProcessExit(pid int) <-chan struct{} {
+	isDead := make(chan struct{})
+	go func() {


What happens when the exec fifo is opened successfully? Will this go routine live forever?

Good spot, this is definitely a problem in the attach case. Think we've fixed it.

crosbymichael · 2018-01-22T16:28:43Z

libcontainer/container_linux.go

+func awaitFifoOpen(path string) <-chan openResult {
+	fifoOpened := make(chan openResult)
+	go func() {
+		f, err := os.OpenFile(path, os.O_RDONLY, 0)


same issue here, if the process dies, how do we unblock this goroutine?

Not sure this is in an issue like the other one, if the process dies we error out https://github.com/cloudfoundry-incubator/runc/blob/exec-fifo-race/libcontainer/container_linux.go#L275 and then cleanup happens as it would in any other case.

When starting a container with `runc start` or `runc run`, the stub process (runc[2:INIT]) opens a fifo for writing. Its parent runc process will open the same fifo for reading. In this way, they synchronize. If the stub process exits at the wrong time, the parent runc process will block forever. This can happen when racing 2 runc operations against each other: `runc run/start`, and `runc delete`. It could also happen for other reasons, e.g. the kernel's OOM killer may select the stub process. This commit resolves this race by racing the opening of the exec fifo from the runc parent process against the stub process exiting. If the stub process exits before we open the fifo, we return an error. Another solution is to wait on the stub process. However, it seems it would require more refactoring to avoid calling wait multiple times on the same process, which is an error. Signed-off-by: Craig Furman <cfurman@pivotal.io>

crosbymichael · 2018-01-22T18:14:58Z

LGTM

crosbymichael · 2018-01-22T21:53:53Z

ping @mrunalp @hqhq

mrunalp · 2018-01-22T21:55:23Z

Looking

mrunalp · 2018-01-22T22:12:58Z

LGTM

hqhq · 2018-01-23T02:59:24Z

libcontainer/container_linux.go

+	go func() {
+		f, err := os.OpenFile(path, os.O_RDONLY, 0)
+		if err != nil {
+			fifoOpened <- openResult{err: newSystemErrorWithCause(err, "open exec fifo for reading")}


It might not affect cleanup, but I think we better return here, because consumer only read it once.

We agree from a code cleanliness perspective, although you're right that it doesn't affect cleanup. We've added a return after that line.

hqhq · 2018-01-23T02:59:51Z

One minor tip, otherwise LGTM to me.

phsiao · 2018-01-23T03:30:19Z

I was able to test this patch, and have updated moby/moby#36010 with my finding so far. In short, it does appear to resolve the issue I was having.

Signed-off-by: Craig Furman <cfurman@pivotal.io>

craigfurman · 2018-01-23T10:48:32Z

We added another commit to address a comment. If you'd like us to rebase and squash let us know.

hqhq · 2018-01-23T11:10:30Z

LGTM, thanks.

crosbymichael · 2018-01-23T14:57:31Z

LGTM

Sample Falco alert: ``` File below / or /root opened for writing (user=<NA> command=runc:[1:CHILD] init parent=docker-runc-cur file=/exec.fifo program=runc:[1:CHILD] CID1 image=<NA>) ``` This github issue provides some context: opencontainers/runc#1698 Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

craigfurman force-pushed the exec-fifo-race branch from 626258e to 6b7ed4e Compare January 22, 2018 16:14

crosbymichael reviewed Jan 22, 2018

View reviewed changes

crosbymichael mentioned this pull request Jan 22, 2018

docker-runc does not terminate and leave docker-shim hanging in 17.12 moby/moby#36010

Closed

craigfurman force-pushed the exec-fifo-race branch from 6b7ed4e to 8d3e6c9 Compare January 22, 2018 17:03

hqhq reviewed Jan 23, 2018

View reviewed changes

Return from goroutine when it should terminate

5c0af14

Signed-off-by: Craig Furman <cfurman@pivotal.io>

crosbymichael merged commit 9f9c962 into opencontainers:master Jan 23, 2018

runcom mentioned this pull request Jan 23, 2018

test: Bump up runc to 9f9c96235cc97674e935002fc3d78361b696a69e cri-o/cri-o#1270

Merged

stevvooe mentioned this pull request Jan 23, 2018

Update runc to 9f9c96235cc97674e935002fc3d78361b69 containerd/containerd#2048

Merged

This was referenced Jan 23, 2018

Docker builds hanging / stuck moby/moby#36067

Closed

Zombie tasks on 17.11 moby/moby#35594

Closed

crosbymichael mentioned this pull request Jan 29, 2018

release: prepare 1.0.2-rc.0 containerd/containerd#2074

Merged

cpuguy83 mentioned this pull request Jan 30, 2018

Can't stop docker container moby/moby#35933

Closed

andrewhsu mentioned this pull request Feb 1, 2018

[17.06] backport: Avoid race when opening exec fifo docker-archive/runc#4

Closed

pires mentioned this pull request Feb 2, 2018

v1.0 discussion #1709

Closed

caniszczyk added this to the 1.0.0 milestone Feb 21, 2018

cyphar mentioned this pull request Feb 24, 2018

VERSION: release v1.0.0-rc5 #1739

Merged

thaJeztah mentioned this pull request Mar 8, 2018

Pods Terminating forever due to Docker 17.09-ce Bug kubernetes-retired/kube-aws#1135

Closed

lucab mentioned this pull request Apr 11, 2018

Docker does not catch container exit coreos/bugs#2306

Closed

cpuguy83 mentioned this pull request May 18, 2018

dockerd seems not to close file descriptors to /var/run/docker.sock and hits "too many open files" error moby/moby#37061

Closed

liggitt mentioned this pull request Dec 18, 2019

Fix race checking for process exit and waiting for exec fifo #2185

Merged

kolyshkin mentioned this pull request Sep 30, 2021

[1.13.1] fix init race projectatomic/runc#56

Merged

Conversation

craigfurman commented Jan 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented Jan 22, 2018 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crosbymichael commented Jan 22, 2018

Uh oh!

mrunalp commented Jan 22, 2018

Uh oh!

mrunalp commented Jan 22, 2018 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhq commented Jan 23, 2018

Uh oh!

phsiao commented Jan 23, 2018

Uh oh!

craigfurman commented Jan 23, 2018

Uh oh!

hqhq commented Jan 23, 2018 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crosbymichael commented Jan 23, 2018 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

crosbymichael commented Jan 22, 2018 •

edited by caniszczyk

Loading

mrunalp commented Jan 22, 2018 •

edited by caniszczyk

Loading

hqhq commented Jan 23, 2018 •

edited by caniszczyk

Loading

crosbymichael commented Jan 23, 2018 •

edited by caniszczyk

Loading