From 23b83cb73669c98e66095aee2c37ac9d0cf97162 Mon Sep 17 00:00:00 2001 From: Unknwon Date: Sun, 11 Jun 2017 02:28:08 -0400 Subject: [PATCH] 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 --- models/action.go | 2 +- models/ssh_key.go | 2 +- pkg/process/manager.go | 85 ++++++++++++++++++++++++------------------ 3 files changed, 51 insertions(+), 38 deletions(-) diff --git a/models/action.go b/models/action.go index 1715c05ab..8b15f1cfd 100644 --- a/models/action.go +++ b/models/action.go @@ -75,7 +75,7 @@ type Action struct { ID int64 UserID int64 // Receiver user ID OpType ActionType - ActUserID int64 // Doer user OD + ActUserID int64 // Doer user ID ActUserName string // Doer user name ActAvatar string `xorm:"-"` RepoID int64 `xorm:"INDEX"` diff --git a/models/ssh_key.go b/models/ssh_key.go index 798c58f21..00dc1af71 100644 --- a/models/ssh_key.go +++ b/models/ssh_key.go @@ -289,7 +289,7 @@ func CheckPublicKeyString(content string) (_ string, err error) { return "", errors.New("only a single line with a single key please") } - // remove any unnecessary whitespace now + // Remove any unnecessary whitespace content = strings.TrimSpace(content) if !setting.SSH.MinimumKeySizeCheck { diff --git a/pkg/process/manager.go b/pkg/process/manager.go index ae83a0b57..babfceed2 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 } }