From 7e65e11136e81a834effe40a9f926416fb1eea78 Mon Sep 17 00:00:00 2001 From: Will Martin Date: Mon, 22 Jan 2018 17:03:02 +0000 Subject: [PATCH 50/94] runc: Avoid race when opening exec fifo [Changelog]: Avoid race when opening exec fifo 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. This is cherry-picked from runc upstream: https://github.com/opencontainers/runc/pull/1698 [Author]:Shukui Yang Change-Id: Id0ba756349b59463f7ee19ad63a6f243bee4a729 Signed-off-by: Craig Furman Signed-off-by: dengguangxing --- libcontainer/container_linux.go | 69 +++++++++++++++++++++++++++++++++++------ libcontainer/system/proc.go | 59 +++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 9 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 74b82c5..278f597 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -5,6 +5,7 @@ package libcontainer import ( "bytes" "encoding/json" + "errors" "fmt" "io" "io/ioutil" @@ -239,20 +240,70 @@ 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): + stat, err := system.GetProcessState(pid) + if err != nil || stat == system.Zombie { + 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")} + } + fifoOpened <- openResult{file: f} + }() + return fifoOpened +} + +type openResult struct { + file *os.File + err error } func (c *linuxContainer) start(process *Process, isInit bool) error { diff --git a/libcontainer/system/proc.go b/libcontainer/system/proc.go index a0e9637..03fd940 100644 --- a/libcontainer/system/proc.go +++ b/libcontainer/system/proc.go @@ -1,12 +1,48 @@ package system import ( + "fmt" "io/ioutil" "path/filepath" "strconv" "strings" ) +type State rune + +const ( // Only values for Linux 3.14 and later are listed here + Dead State = 'X' + DiskSleep State = 'D' + Running State = 'R' + Sleeping State = 'S' + Stopped State = 'T' + TracingStop State = 't' + Zombie State = 'Z' +) + +// String forms of the state from proc(5)'s documentation for +// /proc/[pid]/status' "State" field. +func (s State) String() string { + switch s { + case Dead: + return "dead" + case DiskSleep: + return "disk sleep" + case Running: + return "running" + case Sleeping: + return "sleeping" + case Stopped: + return "stopped" + case TracingStop: + return "tracing stop" + case Zombie: + return "zombie" + default: + return fmt.Sprintf("unknown (%c)", s) + } +} + // look in /proc to find the process start time so that we can verify // that this pid has started after ourself func GetProcessStartTime(pid int) (string, error) { @@ -41,3 +77,26 @@ func parseStartTime(stat string) (string, error) { parts := strings.Split(strings.TrimSpace(s[len(s)-1]), " ") return parts[22-3], nil // starts at 3 (after the filename pos `2`) } + +func GetProcessState(pid int) (State, error) { + data, err := ioutil.ReadFile(filepath.Join("/proc", strconv.Itoa(pid), "stat")) + if err != nil { + return State(0), err + } + return parseState(string(data)) + +} + +func parseState(data string) (State, error) { + i := strings.LastIndex(data, ")") + if i <= 2 || i >= len(data)-3 { + return State(0), fmt.Errorf("invalid stat data: %q", data) + } + + parts := strings.Split(data[i+2:], " ") + + var state int + fmt.Sscanf(parts[3-3], "%c", &state) + stateStr := State(state) + return stateStr, nil +} -- 2.7.4.3