Race condition in RunCmdWithOutputParser (#41)

Co-authored-by: yahavi <[email protected]>

.github/workflows/analysis.yml

@@ -9,7 +9,7 @@ jobs:
       - name: Install Go
         uses: actions/setup-go@v3
         with:
-          go-version: 1.19.x
+          go-version: 1.20.x
       - name: Static Code Analysis
         uses: dominikh/staticcheck-action@v1
         with:
@@ -23,7 +23,7 @@ jobs:
       - name: Install Go
         uses: actions/setup-go@v3
         with:
-          go-version: 1.19.x
+          go-version: 1.20.x
       - name: Install gosec
         run: curl -sfL https://raw.githubusercontent.com/securego/gosec/master/install.sh | sh -s -- -b $(go env GOPATH)/bin
       - name: Run gosec

.github/workflows/frogbot-fix-go.yml

@@ -18,7 +18,7 @@ jobs:
       - name: Setup Go
         uses: actions/setup-go@v3
         with:
-          go-version: 1.19.x
+          go-version: 1.20.x
 
       - uses: jfrog/frogbot@v2
         env:

.github/workflows/frogbot-scan-pr-go.yml

@@ -19,7 +19,7 @@ jobs:
       - name: Setup Go
         uses: actions/setup-go@v3
         with:
-          go-version: 1.19.x
+          go-version: 1.20.x
 
       - uses: jfrog/frogbot@v2
         env:

.github/workflows/test.yml

@@ -17,7 +17,7 @@ jobs:
       - name: Install Go
         uses: actions/setup-go@v3
         with:
-          go-version: 1.19.x
+          go-version: 1.20.x
 
       - name: Go Cache
         uses: actions/cache@v3

go.mod

@@ -1,6 +1,6 @@
 module github.com/jfrog/gofrog
 
-go 1.19
+go 1.20
 
 require (
 	github.com/mholt/archiver/v3 v3.5.1

io/cmd.go

@@ -2,12 +2,14 @@ package io
 
 import (
 	"bufio"
+	"bytes"
 	"errors"
 	"fmt"
 	"io"
 	"os"
 	"os/exec"
 	"regexp"
+	"strings"
 	"sync"
 )
 
@@ -87,54 +89,34 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO
 		return
 	}
 	errChan := make(chan error)
+	stdoutBuilder := strings.Builder{}
 	wg.Add(1)
 	go func() {
+		defer wg.Done()
 		for stdoutReader.Scan() {
-			line := stdoutReader.Text()
-			for _, regExp := range regExpStruct {
-				matched := regExp.RegExp.Match([]byte(line))
-				if matched {
-					regExp.MatchedResults = regExp.RegExp.FindStringSubmatch(line)
-					regExp.Line = line
-					line, err = regExp.ExecFunc(regExp)
-					if err != nil {
-						errChan <- err
-					}
-				}
-			}
+			line, _ := processLine(regExpStruct, stdoutReader.Text(), errChan)
 			if prompt {
 				fmt.Fprintf(os.Stderr, line+"\n")
 			}
-			stdOut += line + "\n"
+			stdoutBuilder.WriteString(line)
+			stdoutBuilder.WriteRune('\n')
 		}
-		wg.Done()
 	}()
+	stderrBuilder := strings.Builder{}
 	wg.Add(1)
 	go func() {
+		defer wg.Done()
 		for stderrReader.Scan() {
-			line := stderrReader.Text()
-			var scannerError error
-			for _, regExp := range regExpStruct {
-				matched := regExp.RegExp.Match([]byte(line))
-				if matched {
-					regExp.MatchedResults = regExp.RegExp.FindStringSubmatch(line)
-					regExp.Line = line
-					line, scannerError = regExp.ExecFunc(regExp)
-					if scannerError != nil {
-						errChan <- scannerError
-						break
-					}
-				}
-			}
+			line, hasError := processLine(regExpStruct, stderrReader.Text(), errChan)
 			if prompt {
 				fmt.Fprintf(os.Stderr, line+"\n")
 			}
-			errorOut += line + "\n"
-			if scannerError != nil {
+			stderrBuilder.WriteString(line)
+			stderrBuilder.WriteRune('\n')
+			if hasError {
 				break
 			}
 		}
-		wg.Done()
 	}()
 
 	go func() {
@@ -145,6 +127,8 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO
 	for err = range errChan {
 		return
 	}
+	stdOut = stdoutBuilder.String()
+	errorOut = stderrBuilder.String()
 
 	err = cmd.Wait()
 	if err != nil {
@@ -158,6 +142,34 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO
 	return
 }
 
+// Run all of the input regExpStruct array on the input stdout or stderr line.
+// If an error occurred, add it to the error channel.
+// regExpStruct - Array of command output patterns to process the line
+// line - string line from stdout or stderr
+// errChan - if an error occurred, add it to this channel
+func processLine(regExpStruct []*CmdOutputPattern, line string, errChan chan error) (processedLine string, hasError bool) {
+	var err error
+	processedLine = line
+	for _, regExp := range regExpStruct {
+		if !regExp.RegExp.MatchString(processedLine) {
+			continue
+		}
+		results := CmdOutputPattern{
+			RegExp:         regExp.RegExp,
+			MatchedResults: regExp.RegExp.FindStringSubmatch(processedLine),
+			Line:           processedLine,
+			ExecFunc:       regExp.ExecFunc,
+		}
+		processedLine, err = regExp.ExecFunc(&results)
+		if err != nil {
+			errChan <- err
+			hasError = true
+			break
+		}
+	}
+	return
+}
+
 // Create command stdout and stderr readers.
 // The returned readers are automatically closed after the running command exit and shouldn't be closed explicitly.
 // cmd - The command to execute
@@ -192,3 +204,59 @@ type CmdOutputPattern struct {
 	Line           string
 	ExecFunc       func(pattern *CmdOutputPattern) (string, error)
 }
+
+type Command struct {
+	Executable string
+	CmdName    string
+	CmdArgs    []string
+	Dir        string
+	StrWriter  io.WriteCloser
+	ErrWriter  io.WriteCloser
+}
+
+func NewCommand(executable, cmdName string, cmdArgs []string) *Command {
+	return &Command{Executable: executable, CmdName: cmdName, CmdArgs: cmdArgs}
+}
+
+func (config *Command) RunWithOutput() (data []byte, err error) {
+	cmd := config.GetCmd()
+	var stdout bytes.Buffer
+	var stderr bytes.Buffer
+	cmd.Stdout = &stdout
+	cmd.Stderr = &stderr
+	err = cmd.Run()
+	if err != nil {
+		return nil, fmt.Errorf("failed running command: '%s %s' with error: %s - %s",
+			cmd.Dir,
+			strings.Join(cmd.Args, " "),
+			err.Error(),
+			stderr.String(),
+		)
+	}
+	return stdout.Bytes(), nil
+}
+
+func (config *Command) GetCmd() (cmd *exec.Cmd) {
+	var cmdStr []string
+	if config.CmdName != "" {
+		cmdStr = append(cmdStr, config.CmdName)
+	}
+	if config.CmdArgs != nil && len(config.CmdArgs) > 0 {
+		cmdStr = append(cmdStr, config.CmdArgs...)
+	}
+	cmd = exec.Command(config.Executable, cmdStr...)
+	cmd.Dir = config.Dir
+	return
+}
+
+func (config *Command) GetEnv() map[string]string {
+	return map[string]string{}
+}
+
+func (config *Command) GetStdWriter() io.WriteCloser {
+	return config.StrWriter
+}
+
+func (config *Command) GetErrWriter() io.WriteCloser {
+	return config.ErrWriter
+}

io/cmd_test.go

@@ -0,0 +1,103 @@
+package io
+
+import (
+	"errors"
+	"regexp"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+var matchAllRegexp = regexp.MustCompile(".*")
+var errParsing = errors.New("parsing error")
+
+func TestRunCmdWithOutputParser(t *testing.T) {
+	config := NewCommand("go", "", []string{"version"})
+	parserCalled := false
+	stdout, stderr, exitOk, err := RunCmdWithOutputParser(config, false, &CmdOutputPattern{
+		RegExp: matchAllRegexp,
+		ExecFunc: func(pattern *CmdOutputPattern) (string, error) {
+			parserCalled = true
+			return pattern.Line, nil
+		},
+	})
+	assert.NoError(t, err)
+	assert.True(t, parserCalled)
+	assert.True(t, exitOk)
+	assert.Contains(t, stdout, "go version")
+	assert.Empty(t, stderr)
+}
+
+func TestRunCmdWithOutputParserError(t *testing.T) {
+	config := NewCommand("go", "", []string{"version"})
+	_, _, exitOk, err := RunCmdWithOutputParser(config, false, &CmdOutputPattern{
+		RegExp:   matchAllRegexp,
+		ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line, errParsing },
+	})
+	assert.ErrorContains(t, err, "parsing error")
+	assert.False(t, exitOk)
+}
+
+var processLineCases = []struct {
+	name              string
+	cmdOutputPatterns []*CmdOutputPattern
+	line              string
+	expectedOutput    string
+	expectError       bool
+}{
+	{"Empty", []*CmdOutputPattern{}, "", "", false},
+
+	{"Simple", []*CmdOutputPattern{{
+		RegExp:   matchAllRegexp,
+		ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line, nil },
+	}}, "hello", "hello", false},
+
+	{"Append character once", []*CmdOutputPattern{{
+		RegExp:   matchAllRegexp,
+		ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line[1:], nil },
+	}}, "hello", "ello", false},
+
+	{"Append character twice", []*CmdOutputPattern{
+		{
+			RegExp:   matchAllRegexp,
+			ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "l", nil },
+		},
+		{
+			RegExp:   matchAllRegexp,
+			ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "o", nil },
+		},
+	}, "hel", "hello", false},
+
+	{"Doesn't match", []*CmdOutputPattern{
+		{
+			RegExp:   regexp.MustCompile("doesn't match"),
+			ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "aaaaaa", nil },
+		},
+		{
+			RegExp:   matchAllRegexp,
+			ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "o", nil },
+		},
+	}, "hell", "hello", false},
+
+	{"Parsing error", []*CmdOutputPattern{{
+		RegExp:   matchAllRegexp,
+		ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return "", errParsing },
+	}}, "hello", "", true},
+}
+
+func TestProcessLine(t *testing.T) {
+	for _, testCase := range processLineCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			errChan := make(chan error, 1)
+			defer close(errChan)
+			processedLine, hasErrors := processLine(testCase.cmdOutputPatterns, testCase.line, errChan)
+			if testCase.expectError {
+				assert.True(t, hasErrors)
+				assert.ErrorIs(t, errParsing, <-errChan)
+			} else {
+				assert.False(t, hasErrors)
+				assert.Equal(t, testCase.expectedOutput, processedLine)
+			}
+		})
+	}
+}