pkg/process: fix potential race condition

Following conditions were not protected:
1. Use and increase next pid
2. Append and remove process from the list

1 files changed, 49 insertions, 36 deletions

diff --git a/pkg/process/manager.go b/pkg/process/manager.go
index ae83a0b5..babfceed 100644
--- a/pkg/process/manager.go
+++ b/pkg/process/manager.go
@@ -9,6 +9,7 @@ import (
 	"errors"
 	"fmt"
 	"os/exec"
+	"sync"
 	"time"
 
 	log "gopkg.in/clog.v1"
@@ -18,43 +19,65 @@ var (
 	ErrExecTimeout = errors.New("Process execution timeout")
 )
 
-// Common timeout.
-var (
-	// NOTE: could be custom in config file for default.
-	DEFAULT = 60 * time.Second
-)
+const DEFAULT_TIMEOUT = 60 * time.Second
 
-// Process represents a working process inherit from Gogs.
+// Process represents a running process calls shell command.
 type Process struct {
-	Pid         int64 // Process ID, not system one.
+	PID         int64
 	Description string
 	Start       time.Time
 	Cmd         *exec.Cmd
 }
 
-// List of existing processes.
-var (
-	curPid    int64 = 1
-	Processes []*Process
-)
+type pidCounter struct {
+	sync.Mutex
+
+	// The current number of pid, initial is 0, and increase 1 every time it's been used.
+	pid int64
+}
 
-// Add adds a existing process and returns its PID.
+func (c *pidCounter) PID() int64 {
+	c.pid++
+	return c.pid
+}
+
+var counter = new(pidCounter)
+var Processes []*Process
+
+// Add adds a process to global list and returns its PID.
 func Add(desc string, cmd *exec.Cmd) int64 {
-	pid := curPid
+	counter.Lock()
+	defer counter.Unlock()
+
+	pid := counter.PID()
 	Processes = append(Processes, &Process{
-		Pid:         pid,
+		PID:         pid,
 		Description: desc,
 		Start:       time.Now(),
 		Cmd:         cmd,
 	})
-	curPid++
 	return pid
 }
 
-// Exec starts executing a command in given path, it records its process and timeout.
+// Remove removes a process from global list.
+// It returns true if the process is found and removed by given pid.
+func Remove(pid int64) bool {
+	counter.Lock()
+	defer counter.Unlock()
+
+	for i := range Processes {
+		if Processes[i].PID == pid {
+			Processes = append(Processes[:i], Processes[i+1:]...)
+			return true
+		}
+	}
+	return false
+}
+
+// Exec starts executing a shell command in given path, it tracks corresponding process and timeout.
 func ExecDir(timeout time.Duration, dir, desc, cmdName string, args ...string) (string, string, error) {
 	if timeout == -1 {
-		timeout = DEFAULT
+		timeout = DEFAULT_TIMEOUT
 	}
 
 	bufOut := new(bytes.Buffer)
@@ -78,7 +101,7 @@ func ExecDir(timeout time.Duration, dir, desc, cmdName string, args ...string) (
 	select {
 	case <-time.After(timeout):
 		if errKill := Kill(pid); errKill != nil {
-			log.Error(4, "Exec(%d:%s): %v", pid, desc, errKill)
+			log.Error(2, "Fail to kill timeout process [pid: %d, desc: %s]: %v", pid, desc, errKill)
 		}
 		<-done
 		return "", ErrExecTimeout.Error(), ErrExecTimeout
@@ -89,37 +112,27 @@ func ExecDir(timeout time.Duration, dir, desc, cmdName string, args ...string) (
 	return bufOut.String(), bufErr.String(), err
 }
 
-// Exec starts executing a command, it records its process and timeout.
+// Exec starts executing a shell command, it tracks corresponding process and timeout.
 func ExecTimeout(timeout time.Duration, desc, cmdName string, args ...string) (string, string, error) {
 	return ExecDir(timeout, "", desc, cmdName, args...)
 }
 
-// Exec starts executing a command, it records its process and has default timeout.
+// Exec starts executing a shell command, it tracks corresponding its process and use default timeout.
 func Exec(desc, cmdName string, args ...string) (string, string, error) {
 	return ExecDir(-1, "", desc, cmdName, args...)
 }
 
-// Remove removes a process from list.
-func Remove(pid int64) {
-	for i, proc := range Processes {
-		if proc.Pid == pid {
-			Processes = append(Processes[:i], Processes[i+1:]...)
-			return
-		}
-	}
-}
-
-// Kill kills and removes a process from list.
+// Kill kills and removes a process from global list.
 func Kill(pid int64) error {
-	for i, proc := range Processes {
-		if proc.Pid == pid {
+	for _, proc := range Processes {
+		if proc.PID == pid {
 			if proc.Cmd != nil && proc.Cmd.Process != nil &&
 				proc.Cmd.ProcessState != nil && !proc.Cmd.ProcessState.Exited() {
 				if err := proc.Cmd.Process.Kill(); err != nil {
-					return fmt.Errorf("fail to kill process(%d/%s): %v", proc.Pid, proc.Description, err)
+					return fmt.Errorf("fail to kill process [pid: %d, desc: %s]: %v", proc.PID, proc.Description, err)
 				}
 			}
-			Processes = append(Processes[:i], Processes[i+1:]...)
+			Remove(pid)
 			return nil
 		}
 	}