Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 77 additions & 22 deletions internal/tool/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,9 @@
func executeCommandForFiles(configurationFile *os.File, toolExecution codacy.ToolExecution, patternDescriptions *[]codacy.PatternDescription, language string, files []string) ([]codacy.Result, error) {
semgrepCmd := createCommand(configurationFile, toolExecution.SourceDir, language, files)

semgrepOutput, semgrepError, err := runCommand(semgrepCmd)
output, semgrepError, err := runAndParseCommand(semgrepCmd, patternDescriptions)
if err != nil {
return nil, errors.New("Error running semgrep: " + *semgrepError + "\n" + err.Error())
}

output, err := parseCommandOutput(patternDescriptions, *semgrepOutput)
if err != nil {
return nil, err
return nil, errors.New("Error running semgrep: " + semgrepError + "\n" + err.Error())
}
return output, nil
}
Expand Down Expand Up @@ -104,32 +99,58 @@
return cmdParams
}

func runCommand(cmd *exec.Cmd) (*string, *string, error) {
var stderr bytes.Buffer
cmd.Stderr = &stderr
cmdOutput, err := cmd.Output()
func runAndParseCommand(cmd *exec.Cmd, patternDescriptions *[]codacy.PatternDescription) ([]codacy.Result, string, error) {

Check warning on line 102 in internal/tool/command.go

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

internal/tool/command.go#L102

Method runAndParseCommand has a cyclomatic complexity of 10 (limit is 7)
stdoutPipe, err := cmd.StdoutPipe()
if err != nil {
stderrString := stderr.String()
return nil, &stderrString, err
return nil, "", err
}

stderrPipe, err := cmd.StderrPipe()
if err != nil {
return nil, "", err
}

stderrTail := &limitedBuffer{max: maxStderrBytes}
stderrDone := make(chan error, 1)
go func() {
_, copyErr := io.Copy(stderrTail, stderrPipe)
stderrDone <- copyErr
}()

if err := cmd.Start(); err != nil {
return nil, "", err
}

results, parseErr := parseCommandOutput(patternDescriptions, stdoutPipe)
if parseErr != nil && cmd.Process != nil {
_ = cmd.Process.Kill()
}

waitErr := cmd.Wait()
stderrCopyErr := <-stderrDone
if stderrCopyErr != nil && !isBenignStreamClose(stderrCopyErr) {
return nil, stderrTail.String(), stderrCopyErr
}
if parseErr != nil {
return nil, stderrTail.String(), parseErr
}
cmdOutputString := string(cmdOutput)
return &cmdOutputString, nil, nil
if waitErr != nil {
return nil, stderrTail.String(), waitErr
}
Comment on lines +131 to +139

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Checking stderrCopyErr before parseErr can mask the actual parsing error. If parseCommandOutput fails (e.g., due to invalid JSON), the process is killed, which often causes io.Copy on the stderr pipe to fail with a non-benign error (like a closed pipe). If this happens, the secondary stderrCopyErr will be returned instead of the root cause parseErr, making debugging extremely difficult. We should check and return parseErr first.

Suggested change
if stderrCopyErr != nil && !isBenignStreamClose(stderrCopyErr) {
return nil, stderrTail.String(), stderrCopyErr
}
if parseErr != nil {
return nil, stderrTail.String(), parseErr
}
cmdOutputString := string(cmdOutput)
return &cmdOutputString, nil, nil
if waitErr != nil {
return nil, stderrTail.String(), waitErr
}
if parseErr != nil {
return nil, stderrTail.String(), parseErr
}
if stderrCopyErr != nil && !isBenignStreamClose(stderrCopyErr) {
return nil, stderrTail.String(), stderrCopyErr
}
if waitErr != nil {
return nil, stderrTail.String(), waitErr
}


return results, "", nil
}

func parseCommandOutput(patternDescriptions *[]codacy.PatternDescription, commandOutput string) ([]codacy.Result, error) {
func parseCommandOutput(patternDescriptions *[]codacy.PatternDescription, stream io.Reader) ([]codacy.Result, error) {
var result []codacy.Result

// Convert the JSON string to a []byte slice
jsonData := []byte(commandOutput)
// Create a bytes.Reader from the []byte slice
reader := bytes.NewReader(jsonData)
// Create a JSON decoder
decoder := json.NewDecoder(reader)
decoder := json.NewDecoder(stream)
// Read and process the JSON stream
for {
var semgrepOutput SemgrepOutput // or a struct that matches your JSON structure
if err := decoder.Decode(&semgrepOutput); err != nil {
if err == io.EOF {
if isBenignStreamClose(err) {
break // End of input
}
return nil, err
Expand All @@ -143,6 +164,40 @@
return result, nil
}

const maxStderrBytes = 64 * 1024

type limitedBuffer struct {
buf bytes.Buffer
max int
}

func (l *limitedBuffer) Write(p []byte) (int, error) {
if l.max <= 0 {
return len(p), nil
}
if len(p) >= l.max {
l.buf.Reset()
_, _ = l.buf.Write(p[len(p)-l.max:])
return len(p), nil
}
if l.buf.Len()+len(p) > l.max {
drop := l.buf.Len() + len(p) - l.max
current := l.buf.Bytes()
l.buf.Reset()
_, _ = l.buf.Write(current[drop:])
}
Comment on lines +183 to +188

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation of limitedBuffer.Write performs an overlapping write on the same underlying array of bytes.Buffer after calling Reset(). While Go's copy is overlap-safe, relying on this behavior alongside bytes.Buffer's internal implementation details (e.g., that Reset does not reallocate or clear the slice) is fragile and non-idiomatic. Copying the remaining bytes to a temporary slice first is much safer and more maintainable.

Suggested change
if l.buf.Len()+len(p) > l.max {
drop := l.buf.Len() + len(p) - l.max
current := l.buf.Bytes()
l.buf.Reset()
_, _ = l.buf.Write(current[drop:])
}
if l.buf.Len()+len(p) > l.max {
drop := l.buf.Len() + len(p) - l.max
temp := make([]byte, l.buf.Len()-drop)
copy(temp, l.buf.Bytes()[drop:])
l.buf.Reset()
_, _ = l.buf.Write(temp)
}

_, _ = l.buf.Write(p)
return len(p), nil
}

func (l *limitedBuffer) String() string {
return l.buf.String()
}

func isBenignStreamClose(err error) bool {
return errors.Is(err, io.EOF) || errors.Is(err, os.ErrClosed) || strings.Contains(err.Error(), "file already closed")
}
Comment on lines +197 to +199

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If err is nil, calling err.Error() will cause a nil pointer dereference panic. Although the current call sites guard against nil, adding a defensive nil check at the beginning of isBenignStreamClose makes this helper function robust and safe for future reuse.

func isBenignStreamClose(err error) bool {
	if err == nil {
		return false
	}
	return errors.Is(err, io.EOF) || errors.Is(err, os.ErrClosed) || strings.Contains(err.Error(), "file already closed")
}


func appendIssueToResult(result []codacy.Result, patternDescriptions *[]codacy.PatternDescription, semgrepOutput SemgrepOutput) []codacy.Result {
for _, semgrepRes := range semgrepOutput.Results {
if semgrepRes.Extra.IsIgnored {
Expand Down
48 changes: 37 additions & 11 deletions internal/tool/command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"os"
"os/exec"
"path/filepath"
"strings"
"testing"

codacy "github.com/codacy/codacy-engine-golang-seed/v6"
Expand Down Expand Up @@ -56,30 +57,32 @@ func TestCreateCommandParameters(t *testing.T) {
assert.Subset(t, cmdParams, expectedParams)
}

func TestRunCommand(t *testing.T) {
func TestRunAndParseCommand(t *testing.T) {
// Arrange
mockCmd := exec.Command("echo", "Testing runCommand()")
mockCmd := exec.Command("echo", "{\"results\":[],\"errors\":[]}")
patternDescriptions := []codacy.PatternDescription{}

// Act
stdout, stderr, err := runCommand(mockCmd)
results, stderr, err := runAndParseCommand(mockCmd, &patternDescriptions)

// Assert
assert.NoError(t, err)
assert.Nil(t, stderr)
assert.Equal(t, "Testing runCommand()\n", *stdout)
assert.Empty(t, stderr)
assert.Empty(t, results)
}

func TestRunCommand_Error(t *testing.T) {
func TestRunAndParseCommand_Error(t *testing.T) {
// Arrange
mockCmd := exec.Command("invalid_command_name")
patternDescriptions := []codacy.PatternDescription{}

// Act
stdout, stderr, err := runCommand(mockCmd)
results, stderr, err := runAndParseCommand(mockCmd, &patternDescriptions)

// Assert
assert.Error(t, err, "Expected an error running an invalid command")
assert.Empty(t, stdout, "Expected empty stdout for a failed command")
assert.Empty(t, stderr, "Expected empty stderr for a failed command")
assert.Empty(t, results, "Expected empty results for a failed command")
assert.Empty(t, stderr, "Expected empty stderr for a failed command start failure")
}

func TestParseCommandOutput(t *testing.T) {
Expand All @@ -93,7 +96,7 @@ func TestParseCommandOutput(t *testing.T) {
commandOutput := "{\"version\": \"1.49.0\", \"results\": [{\"check_id\": \"bash.curl.security.curl-eval.curl-eval\", \"path\": \"src/bash/curl-eval.bash\", \"start\": {\"line\": 5}, \"end\": {\"line\": 5}, \"extra\": {\"message\": \"Sample message\"}}], \"errors\": []}"

// Act
result, err := parseCommandOutput(&mockPatternDescriptions, commandOutput)
result, err := parseCommandOutput(&mockPatternDescriptions, strings.NewReader(commandOutput))

// Assert
assert.NoError(t, err, "Expected no error during parsing command output")
Expand Down Expand Up @@ -162,7 +165,7 @@ func TestAppendToResultWithIgnore(t *testing.T) {
}`

// Act
result, _ := parseCommandOutput(&mockPatternDescriptions, validSemgrepOutput)
result, _ := parseCommandOutput(&mockPatternDescriptions, strings.NewReader(validSemgrepOutput))

// Assert
assert.Len(t, result, 2, "Expected length of the result slice to be 2")
Expand Down Expand Up @@ -338,3 +341,26 @@ func TestWriteMessageWithInvalidPatternID(t *testing.T) {
// Assert
assert.Equal(t, docgen.GetFirstSentence(nonEmptyMessage), description, "Expected first sentence of non-empty message for invalid pattern ID")
}

func TestLimitedBufferKeepsOnlyTail(t *testing.T) {
// Arrange
buffer := &limitedBuffer{max: 5}

// Act
_, _ = buffer.Write([]byte("123"))
_, _ = buffer.Write([]byte("4567"))

// Assert
assert.Equal(t, "34567", buffer.String())
}

func TestLimitedBufferLargeWriteTruncatesToMax(t *testing.T) {
// Arrange
buffer := &limitedBuffer{max: 4}

// Act
_, _ = buffer.Write([]byte("abcdef"))

// Assert
assert.Equal(t, "cdef", buffer.String())
}