Add a separate lock for pid.

Random-Liu · Random-Liu · commit 952d58297d81 · 2019-02-01T08:48:26.000-08:00
Signed-off-by: Lantao Liu &lt;lantaol@google.com&gt;
diff --git a/runtime/v1/linux/proc/exec.go b/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,9 +69,7 @@ func (e *execProcess) ID() string {
 }
 
 func (e *execProcess) Pid() int {
-	e.mu.Lock()
-	defer e.mu.Unlock()
-	return e.pid
+	return e.pid.get()
 }
 
 func (e *execProcess) ExitStatus() int {
@@ -143,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")
@@ -168,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))
@@ -227,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
 }
 
@@ -245,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
diff --git a/runtime/v1/linux/proc/init.go b/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
diff --git a/runtime/v1/linux/proc/utils.go b/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 == "" {

Original file line number	Diff line number	Diff line change
`@@ -407,6 +407,7 @@ func (p Init) exec(ctx context.Context, path string, r ExecConfig) (proc.Proce`
`407`	`407`	`Terminal: r.Terminal,`
`408`	`408`	`},`
`409`	`409`	`waitBlock: make(chan struct{}),`
	`410`	`+ pid: &safePid{},`
`410`	`411`	`}`
`411`	`412`	`e.execState = &execCreatedState{p: e}`
`412`	`413`	`return e, nil`