[17.06] backport: Avoid race when opening exec fifo

.travis.yml

@@ -4,6 +4,8 @@ go:
   - 1.8.x
   - tip
 
+go_import_path: github.com/opencontainers/runc
+
 matrix:
   allow_failures:
     - go: tip
@@ -22,7 +24,7 @@ before_install:
   - sudo apt-get install -y libseccomp-dev libapparmor-dev
   - go get -u github.com/golang/lint/golint
   - go get -u github.com/vbatts/git-validation
-  - go get -u github.com/mvdan/sh/cmd/shfmt
+#  - go get -u mvdan.cc/sh/cmd/shfmt 	# disabled because Dockerfile uses older version of shfmt
   - env | grep TRAVIS_
 
 script:

Dockerfile

@@ -20,6 +20,7 @@ RUN apt-get update && apt-get install -y \
     protobuf-c-compiler \
     protobuf-compiler \
     python-minimal \
+    kmod \
     --no-install-recommends \
     && apt-get clean
 

Makefile

@@ -85,13 +85,13 @@ localtest:
 	make localunittest localintegration localrootlessintegration
 
 unittest: runcimage
-	docker run -e TESTFLAGS -t --privileged --rm -v $(CURDIR):/go/src/$(PROJECT) $(RUNC_IMAGE) make localunittest
+	docker run -e TESTFLAGS -t --privileged --rm -v /lib/modules:/lib/modules:ro -v $(CURDIR):/go/src/$(PROJECT) $(RUNC_IMAGE) make localunittest
 
 localunittest: all
 	go test -timeout 3m -tags "$(BUILDTAGS)" ${TESTFLAGS} -v $(allpackages)
 
 integration: runcimage
-	docker run -e TESTFLAGS -t --privileged --rm -v $(CURDIR):/go/src/$(PROJECT) $(RUNC_IMAGE) make localintegration
+	docker run -e TESTFLAGS -t --privileged --rm -v /lib/modules:/lib/modules:ro -v $(CURDIR):/go/src/$(PROJECT) $(RUNC_IMAGE) make localintegration
 
 localintegration: all
 	bats -t tests/integration${TESTFLAGS}
@@ -133,7 +133,7 @@ clean:
 
 validate:
 	script/validate-gofmt
-	script/validate-shfmt
+	[[ -z "$(TRAVIS)" ]] && script/validate-shfmt || true
 	go vet $(allpackages)
 
 ci: validate localtest

contrib/cmd/recvtty/recvtty.go

@@ -24,6 +24,7 @@ import (
 	"os"
 	"strings"
 
+	"github.com/opencontainers/runc/libcontainer"
 	"github.com/opencontainers/runc/libcontainer/utils"
 	"github.com/urfave/cli"
 )
@@ -100,6 +101,9 @@ func handleSingle(path string) error {
 	if err != nil {
 		return err
 	}
+	if err = libcontainer.SaneTerminal(master); err != nil {
+		return err
+	}
 
 	// Copy from our stdio to the master fd.
 	quitChan := make(chan struct{})

libcontainer/console_linux.go

@@ -130,12 +130,12 @@ func ptsname(f *os.File) (string, error) {
 	return fmt.Sprintf("/dev/pts/%d", n), nil
 }
 
-// saneTerminal sets the necessary tty_ioctl(4)s to ensure that a pty pair
+// SaneTerminal sets the necessary tty_ioctl(4)s to ensure that a pty pair
 // created by us acts normally. In particular, a not-very-well-known default of
 // Linux unix98 ptys is that they have +onlcr by default. While this isn't a
 // problem for terminal emulators, because we relay data from the terminal we
 // also relay that funky line discipline.
-func saneTerminal(terminal *os.File) error {
+func SaneTerminal(terminal *os.File) error {
 	// Go doesn't have a wrapper for any of the termios ioctls.
 	var termios unix.Termios
 

libcontainer/container_linux.go

@@ -5,13 +5,15 @@ package libcontainer
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"reflect"
+	"strconv"
 	"strings"
 	"sync"
 	"syscall"
@@ -233,20 +235,71 @@ func (c *linuxContainer) Exec() error {
 
 func (c *linuxContainer) exec() error {
 	path := filepath.Join(c.root, execFifoFilename)
-	f, err := os.OpenFile(path, os.O_RDONLY, 0)
-	if err != nil {
-		return newSystemErrorWithCause(err, "open exec fifo for reading")
+
+	fifoOpen := make(chan struct{})
+	select {
+	case <-awaitProcessExit(c.initProcess.pid(), fifoOpen):
+		return errors.New("container process is already dead")
+	case result := <-awaitFifoOpen(path):
+		close(fifoOpen)
+		if result.err != nil {
+			return result.err
+		}
+		f := result.file
+		defer f.Close()
+		if err := readFromExecFifo(f); err != nil {
+			return err
+		}
+		return os.Remove(path)
 	}
-	defer f.Close()
-	data, err := ioutil.ReadAll(f)
+}
+
+func readFromExecFifo(execFifo io.Reader) error {
+	data, err := ioutil.ReadAll(execFifo)
 	if err != nil {
 		return err
 	}
-	if len(data) > 0 {
-		os.Remove(path)
-		return nil
+	if len(data) <= 0 {
+		return fmt.Errorf("cannot start an already running container")
 	}
-	return fmt.Errorf("cannot start an already running container")
+	return nil
+}
+
+func awaitProcessExit(pid int, exit <-chan struct{}) <-chan struct{} {
+	isDead := make(chan struct{})
+	go func() {
+		for {
+			select {
+			case <-exit:
+				return
+			case <-time.After(time.Millisecond * 100):
+				// if process became zombie, signal death
+				if state, err := pidState(pid); err != nil || state == 'Z' {
+					close(isDead)
+					return
+				}
+			}
+		}
+	}()
+	return isDead
+}
+
+func awaitFifoOpen(path string) <-chan openResult {
+	fifoOpened := make(chan openResult)
+	go func() {
+		f, err := os.OpenFile(path, os.O_RDONLY, 0)
+		if err != nil {
+			fifoOpened <- openResult{err: newSystemErrorWithCause(err, "open exec fifo for reading")}
+			return
+		}
+		fifoOpened <- openResult{file: f}
+	}()
+	return fifoOpened
+}
+
+type openResult struct {
+	file *os.File
+	err  error
 }
 
 func (c *linuxContainer) start(process *Process, isInit bool) error {
@@ -1569,3 +1622,41 @@ func (c *linuxContainer) bootstrapData(cloneFlags uintptr, nsMaps map[configs.Na
 
 	return bytes.NewReader(r.Serialize()), nil
 }
+
+// pidState returns the state code as specified in /proc/$pid/stat
+// This function was created only to backport https://github.com/opencontainers/runc/pull/1698
+// without having to also backport https://github.com/opencontainers/runc/pull/1489
+func pidState(pid int) (int, error) {
+	bytes, err := ioutil.ReadFile(filepath.Join("/proc", strconv.Itoa(pid), "stat"))
+	if err != nil {
+		return 0, err
+	}
+
+	return parseState(string(bytes))
+}
+
+// parseState parses the state code from a string in the format of /proc/$pid/stat
+// This function was created only to backport https://github.com/opencontainers/runc/pull/1698
+// without having to also backport https://github.com/opencontainers/runc/pull/1489
+func parseState(data string) (int, error) {
+	// From proc(5), field 2 could contain space and is inside `(` and `)`.
+	// The following is an example:
+	// 89653 (gunicorn: maste) S 89630 89653 89653 0 -1 4194560 29689 28896 0 3 146 32 76 19 20 0 1 0 2971844 52965376 3920 18446744073709551615 1 1 0 0 0 0 0 16781312 137447943 0 0 0 17 1 0 0 0 0 0 0 0 0 0 0 0 0 0
+	i := strings.LastIndex(data, ")")
+	if i <= 2 || i >= len(data)-1 {
+		return 0, fmt.Errorf("invalid stat data: %q", data)
+	}
+
+	parts := strings.SplitN(data[:i], "(", 2)
+	if len(parts) != 2 {
+		return 0, fmt.Errorf("invalid stat data: %q", data)
+	}
+
+	// parts indexes should be offset by 3 from the field number given
+	// proc(5), because parts is zero-indexed and we've removed fields
+	// one (PID) and two (Name) in the paren-split.
+	parts = strings.SplitN(data[i+2:], " ", 2)
+	var state int
+	fmt.Sscanf(parts[3-3], "%c", &state)
+	return state, nil
+}

libcontainer/container_linux_test.go

@@ -216,3 +216,21 @@ func TestGetContainerState(t *testing.T) {
 		}
 	}
 }
+
+func TestParseState(t *testing.T) {
+	data := map[string]int{
+		"4902 (gunicorn: maste) S 4885 4902 4902 0 -1 4194560 29683 29929 61 83 78 16 96 17 20 0 1 0 9126532 52965376 1903 18446744073709551615 4194304 7461796 140733928751520 140733928698072 139816984959091 0 0 16781312 137447943 1 0 0 17 3 0 0 9 0 0 9559488 10071156 33050624 140733928758775 140733928758945 140733928758945 140733928759264 0": 'S',
+		"9534 (cat) R 9323 9534 9323 34828 9534 4194304 95 0 0 0 0 0 0 0 20 0 1 0 9214966 7626752 168 18446744073709551615 4194304 4240332 140732237651568 140732237650920 140570710391216 0 0 0 0 0 0 0 17 1 0 0 0 0 0 6340112 6341364 21553152 140732237653865 140732237653885 140732237653885 140732237656047 0":                                      'R',
+
+		"24767 (irq/44-mei_me) S 2 0 0 0 -1 2129984 0 0 0 0 0 0 0 0 -51 0 1 0 8722075 0 0 18446744073709551615 0 0 0 0 0 0 0 2147483647 0 0 0 0 17 1 50 1 0 0 0 0 0 0 0 0 0 0 0": 'S',
+	}
+	for line, expected := range data {
+		state, err := parseState(line)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if state != expected {
+			t.Fatalf("expected state %q but received %q", expected, state)
+		}
+	}
+}

libcontainer/integration/execin_test.go

@@ -299,6 +299,7 @@ func TestExecInTTY(t *testing.T) {
 				err: err,
 			}
 		}
+		libcontainer.SaneTerminal(f)
 		dc <- &cdata{
 			c: libcontainer.ConsoleFromFile(f),
 		}

tty.go

@@ -74,6 +74,9 @@ func (t *tty) recvtty(process *libcontainer.Process, socket *os.File) error {
 	if err != nil {
 		return err
 	}
+	if err = libcontainer.SaneTerminal(f); err != nil {
+		return err
+	}
 	console := libcontainer.ConsoleFromFile(f)
 	go io.Copy(console, os.Stdin)
 	t.wg.Add(1)