From 6d742be8bc7a1f3c4a46fc9405478d4572a53e9f Mon Sep 17 00:00:00 2001 From: Eran Turgeman <81029514+eranturgeman@users.noreply.github.com> Date: Thu, 30 Nov 2023 11:17:21 +0200 Subject: [PATCH] Race condition in RunCmdWithOutputParser (#41) Co-authored-by: yahavi --- .github/workflows/analysis.yml | 4 +- .github/workflows/frogbot-fix-go.yml | 2 +- .github/workflows/frogbot-scan-pr-go.yml | 2 +- .github/workflows/test.yml | 2 +- go.mod | 2 +- io/cmd.go | 130 +++++++++++++++++------ io/cmd_test.go | 103 ++++++++++++++++++ 7 files changed, 208 insertions(+), 37 deletions(-) create mode 100644 io/cmd_test.go diff --git a/.github/workflows/analysis.yml b/.github/workflows/analysis.yml index c398717..e847680 100644 --- a/.github/workflows/analysis.yml +++ b/.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 diff --git a/.github/workflows/frogbot-fix-go.yml b/.github/workflows/frogbot-fix-go.yml index 2fcdf7f..84c550e 100644 --- a/.github/workflows/frogbot-fix-go.yml +++ b/.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: diff --git a/.github/workflows/frogbot-scan-pr-go.yml b/.github/workflows/frogbot-scan-pr-go.yml index 0a19766..87f618e 100644 --- a/.github/workflows/frogbot-scan-pr-go.yml +++ b/.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: diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 5ab119a..8377cfe 100644 --- a/.github/workflows/test.yml +++ b/.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 diff --git a/go.mod b/go.mod index bd3e1b0..95b050f 100644 --- a/go.mod +++ b/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 diff --git a/io/cmd.go b/io/cmd.go index 5c54c85..0f6cc3c 100644 --- a/io/cmd.go +++ b/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 +} diff --git a/io/cmd_test.go b/io/cmd_test.go new file mode 100644 index 0000000..0511fea --- /dev/null +++ b/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) + } + }) + } +}