Repository: ossf/package-analysis
Pull request #756 (2023-06-08 07:01:52)

remove restriction on number of stdout and stderr lines recorded
Limiting the number of lines of stdout and stderr saved during dynamic analysis sometimes makes debugging difficult. Since we already limit the number of bytes recorded, limiting the number of lines doesn't bring much added benefit for storage costs. This PR removes the line limit.

Eventually we will move to recording the whole stdout and stderr in separate files.


Patch:
diff --git a/internal/dynamicanalysis/analysis.go b/internal/dynamicanalysis/analysis.go
index fae4f47..65e6938 100644
--- a/internal/dynamicanalysis/analysis.go
+++ b/internal/dynamicanalysis/analysis.go
@@ -9,11 +9,11 @@ import (
 	"github.com/ossf/package-analysis/internal/packetcapture"
 	"github.com/ossf/package-analysis/internal/sandbox"
 	"github.com/ossf/package-analysis/internal/strace"
+	"github.com/ossf/package-analysis/internal/utils"
 	"github.com/ossf/package-analysis/pkg/api/analysisrun"
 )
 
 const (
-	maxOutputLines = 20
 	maxOutputBytes = 4 * 1024
 )
 
@@ -71,8 +71,8 @@ func Run(sb sandbox.Sandbox, command string, args []string) (*Result, error) {
 	analysisResult := Result{
 		StraceSummary: analysisrun.StraceSummary{
 			Status: analysis.StatusForRunResult(r),
-			Stdout: lastLines(r.Stdout(), maxOutputLines, maxOutputBytes),
-			Stderr: lastLines(r.Stderr(), maxOutputLines, maxOutputBytes),
+			Stdout: utils.LastNBytes(r.Stdout(), maxOutputBytes),
+			Stderr: utils.LastNBytes(r.Stderr(), maxOutputBytes),
 		},
 	}
 	analysisResult.setData(straceResult, dns)
diff --git a/internal/dynamicanalysis/output.go b/internal/dynamicanalysis/output.go
deleted file mode 100644
index 3019832..0000000
--- a/internal/dynamicanalysis/output.go
+++ /dev/null
@@ -1,36 +0,0 @@
-package dynamicanalysis
-
-// lastLines returns a byte array containing only the last maxLines of lines
-// in b.
-//
-// A line is defined the by the occurance of a '\n' in the output.
-//
-// If there are fewer than maxLines then the entire intput byte array will be
-// returned.
-//
-// No more than maxBytes will be in the returned byte array.
-func lastLines(b []byte, maxLines, maxBytes int) []byte {
-	if len(b) == 0 {
-		return b
-	}
-	// only consider the last maxBytes of b, or all of b if maxBytes is bigger
-	// than the size of b.
-	startOff := len(b) - maxBytes
-	if startOff < 0 {
-		startOff = 0
-	}
-	b = b[startOff:]
-	// count each newline from the end of the bufffer
-	lines := 0
-	lineOff := len(b) - 1
-	for off := len(b) - 1; off > 0; off-- {
-		if b[off] == '\n' {
-			lines++
-			lineOff = off
-		}
-		if lines > maxLines {
-			return b[lineOff+1:]
-		}
-	}
-	return b
-}
diff --git a/internal/utils/last_bytes.go b/internal/utils/last_bytes.go
new file mode 100644
index 0000000..6be6bb2
--- /dev/null
+++ b/internal/utils/last_bytes.go
@@ -0,0 +1,14 @@
+package utils
+
+// LastNBytes returns the last n bytes from b.
+// If len(b) <= n, b itself is returned, otherwise a copy of the bytes is returned.
+// If n is negative, the function will panic
+func LastNBytes(b []byte, n int) []byte {
+	if n < 0 {
+		panic("n cannot be negative")
+	}
+	if len(b) <= n {
+		return b
+	}
+	return b[(len(b) - n):]
+}


Test patch:
diff --git a/internal/dynamicanalysis/output_test.go b/internal/dynamicanalysis/output_test.go
deleted file mode 100644
index 48700ca..0000000
--- a/internal/dynamicanalysis/output_test.go
+++ /dev/null
@@ -1,46 +0,0 @@
-package dynamicanalysis
-
-import (
-	"bytes"
-	"testing"
-)
-
-func TestEmptyBuffer(t *testing.T) {
-	b := []byte{}
-	if test := lastLines(b, 10, 100); len(test) > 0 {
-		t.Fatalf("lastLines() = %v; want empty byte array", test)
-	}
-}
-
-func TestSmallBuffer(t *testing.T) {
-	b := []byte("hello world")
-	if test := lastLines(b, 10, 100); !bytes.Equal(test, b) {
-		t.Fatalf("lastLines() = %v, want %v", test, b)
-	}
-}
-
-func TestSmallBufferWithFewLines(t *testing.T) {
-	b := []byte("hello\nworld\n")
-	if test := lastLines(b, 10, 100); !bytes.Equal(test, b) {
-		t.Fatalf("lastLines() = %v, want %v", test, b)
-	}
-}
-
-func TestSmallBufferWithManyLines(t *testing.T) {
-	b := []byte("one\ntwo\nthree\nfour\nfive\nsix\nseven\neight\nnine\nten\n")
-	exp := []byte("six\nseven\neight\nnine\nten\n")
-	if test := lastLines(b, 5, 100); !bytes.Equal(test, exp) {
-		t.Fatalf("lastLines() = %v, want %v", test, exp)
-	}
-}
-
-func TestBigBufferWithNoLines(t *testing.T) {
-	b := make([]byte, 1000, 1000)
-	for i := range b {
-		b[i] = 'a' + byte(i%26)
-	}
-	exp := []byte("qrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijkl")
-	if test := lastLines(b, 10, 100); !bytes.Equal(test, exp) {
-		t.Fatalf("lastLines() = %v (%d), want %v (%d)", test, len(test), exp, len(exp))
-	}
-}
diff --git a/internal/utils/last_bytes_test.go b/internal/utils/last_bytes_test.go
new file mode 100644
index 0000000..39f8810
--- /dev/null
+++ b/internal/utils/last_bytes_test.go
@@ -0,0 +1,61 @@
+package utils
+
+import (
+	"reflect"
+	"testing"
+)
+
+type lastNBytesTestCase struct {
+	name  string
+	bytes []byte
+	n     int
+	want  []byte
+}
+
+func TestLastNBytes(t *testing.T) {
+	tests := []lastNBytesTestCase{
+		{
+			"empty_0",
+			[]byte{},
+			0,
+			[]byte{},
+		},
+		{
+			"empty_10",
+			[]byte{},
+			10,
+			[]byte{},
+		},
+		{
+			"abcd_0",
+			[]byte{'a', 'b', 'c', 'd'},
+			0,
+			[]byte{},
+		},
+		{
+			"abcd_1",
+			[]byte{'a', 'b', 'c', 'd'},
+			1,
+			[]byte{'d'},
+		},
+		{
+			"abcd_4",
+			[]byte{'a', 'b', 'c', 'd'},
+			4,
+			[]byte{'a', 'b', 'c', 'd'},
+		},
+		{
+			"abcd_5",
+			[]byte{'a', 'b', 'c', 'd'},
+			5,
+			[]byte{'a', 'b', 'c', 'd'},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := LastNBytes(tt.bytes, tt.n); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("LastNBytes() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}