Merge pull request #2970 from Random-Liu/fix-exec-race-condition

Fix exec race condition

runtime/v1/linux/proc/deleted_state.go

@@ -69,7 +69,3 @@ func (s *deletedState) SetExited(status int) {
 func (s *deletedState) Exec(ctx context.Context, path string, r *ExecConfig) (proc.Process, error) {
 	return nil, errors.Errorf("cannot exec in a deleted state")
 }
-
-func (s *deletedState) Pid() int {
-	return -1
-}

runtime/v1/linux/proc/exec.go

@@ -49,7 +49,7 @@ type execProcess struct {
 	io      runc.IO
 	status  int
 	exited  time.Time
-	pid     int
+	pid     *safePid
 	closers []io.Closer
 	stdin   io.Closer
 	stdio   proc.Stdio
@@ -69,11 +69,7 @@ func (e *execProcess) ID() string {
 }
 
 func (e *execProcess) Pid() int {
-	return e.execState.Pid()
+	return e.pid.get()
-}
-
-func (e *execProcess) pidv() int {
-	return e.pid
 }
 
 func (e *execProcess) ExitStatus() int {
@@ -145,7 +141,7 @@ func (e *execProcess) Kill(ctx context.Context, sig uint32, _ bool) error {
 }
 
 func (e *execProcess) kill(ctx context.Context, sig uint32, _ bool) error {
-	pid := e.pid
+	pid := e.pid.get()
 	if pid != 0 {
 		if err := unix.Kill(pid, syscall.Signal(sig)); err != nil {
 			return errors.Wrapf(checkKillError(err), "exec kill error")
@@ -170,6 +166,12 @@ func (e *execProcess) Start(ctx context.Context) error {
 }
 
 func (e *execProcess) start(ctx context.Context) (err error) {
+	// The reaper may receive exit signal right after
+	// the container is started, before the e.pid is updated.
+	// In that case, we want to block the signal handler to
+	// access e.pid until it is updated.
+	e.pid.Lock()
+	defer e.pid.Unlock()
 	var (
 		socket  *runc.Socket
 		pidfile = filepath.Join(e.path, fmt.Sprintf("%s.pid", e.id))
@@ -229,7 +231,7 @@ func (e *execProcess) start(ctx context.Context) (err error) {
 	if err != nil {
 		return errors.Wrap(err, "failed to retrieve OCI runtime exec pid")
 	}
-	e.pid = pid
+	e.pid.pid = pid
 	return nil
 }
 
@@ -247,11 +249,11 @@ func (e *execProcess) Status(ctx context.Context) (string, error) {
 	e.mu.Lock()
 	defer e.mu.Unlock()
 	// if we don't have a pid then the exec process has just been created
-	if e.pid == 0 {
+	if e.pid.get() == 0 {
 		return "created", nil
 	}
 	// if we have a pid and it can be signaled, the process is running
-	if err := unix.Kill(e.pid, 0); err == nil {
+	if err := unix.Kill(e.pid.get(), 0); err == nil {
 		return "running", nil
 	}
 	// else if we have a pid but it can nolonger be signaled, it has stopped

runtime/v1/linux/proc/exec_state.go

@@ -31,7 +31,6 @@ type execState interface {
 	Delete(context.Context) error
 	Kill(context.Context, uint32, bool) error
 	SetExited(int)
-	Pid() int
 }
 
 type execCreatedState struct {
@@ -83,12 +82,6 @@ func (s *execCreatedState) SetExited(status int) {
 	}
 }
 
-func (s *execCreatedState) Pid() int {
-	s.p.mu.Lock()
-	defer s.p.mu.Unlock()
-	return s.p.pidv()
-}
-
 type execRunningState struct {
 	p *execProcess
 }
@@ -127,12 +120,6 @@ func (s *execRunningState) SetExited(status int) {
 	}
 }
 
-func (s *execRunningState) Pid() int {
-	s.p.mu.Lock()
-	defer s.p.mu.Unlock()
-	return s.p.pidv()
-}
-
 type execStoppedState struct {
 	p *execProcess
 }
@@ -170,7 +157,3 @@ func (s *execStoppedState) Kill(ctx context.Context, sig uint32, all bool) error
 func (s *execStoppedState) SetExited(status int) {
 	// no op
 }
-
-func (s *execStoppedState) Pid() int {
-	return s.p.pidv()
-}

runtime/v1/linux/proc/init.go

@@ -407,6 +407,7 @@ func (p *Init) exec(ctx context.Context, path string, r *ExecConfig) (proc.Proce
 			Terminal: r.Terminal,
 		},
 		waitBlock: make(chan struct{}),
+		pid:       &safePid{},
 	}
 	e.execState = &execCreatedState{p: e}
 	return e, nil

runtime/v1/linux/proc/utils.go

@@ -23,6 +23,7 @@ import (
 	"io"
 	"os"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/containerd/containerd/errdefs"
@@ -31,6 +32,18 @@ import (
 	"golang.org/x/sys/unix"
 )
 
+// safePid is a thread safe wrapper for pid.
+type safePid struct {
+	sync.Mutex
+	pid int
+}
+
+func (s *safePid) get() int {
+	s.Lock()
+	defer s.Unlock()
+	return s.pid
+}
+
 // TODO(mlaventure): move to runc package?
 func getLastRuntimeError(r *runc.Runc) (string, error) {
 	if r.Log == "" {