diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go index 1b174259..62e3dbc2 100644 --- a/internal/sandbox/sandbox.go +++ b/internal/sandbox/sandbox.go @@ -503,12 +503,7 @@ func (s *podmanSandbox) Run(ctx context.Context, command string, args ...string) } err = cmd.Wait() - if err == nil { - result.status = RunStatusSuccess - } else if _, ok := err.(*exec.ExitError); ok { - result.status = RunStatusFailure - err = nil - } + result.status, err = classifyRunResult(ctx, err) // Stop the container stopCmd := s.stopContainerCmd(ctx) @@ -528,6 +523,24 @@ func (s *podmanSandbox) Run(ctx context.Context, command string, args ...string) return result, err } +// classifyRunResult determines the RunStatus for a sandboxed command based on +// the error returned by cmd.Wait() and the run's context. A context that has +// exceeded its deadline takes priority over the process error, since a +// killed process surfaces to Wait() as a generic *exec.ExitError that is +// otherwise indistinguishable from an ordinary non-zero exit. +func classifyRunResult(ctx context.Context, err error) (RunStatus, error) { + if err == nil { + return RunStatusSuccess, nil + } + if ctx.Err() == context.DeadlineExceeded { + return RunStatusTimeout, nil + } + if _, ok := err.(*exec.ExitError); ok { + return RunStatusFailure, nil + } + return RunStatusUnknown, err +} + // Clean implements the Sandbox interface. func (s *podmanSandbox) Clean(ctx context.Context) error { if s.container == "" { diff --git a/internal/sandbox/sandbox_test.go b/internal/sandbox/sandbox_test.go new file mode 100644 index 00000000..09a8fdcd --- /dev/null +++ b/internal/sandbox/sandbox_test.go @@ -0,0 +1,60 @@ +package sandbox + +import ( + "context" + "os/exec" + "testing" + "time" +) + +// TestClassifyRunResult_Timeout reproduces ossf/package-analysis#142: a +// command killed because its context deadline expired must be classified as +// RunStatusTimeout, not RunStatusFailure. Before the fix, cmd.Wait() returns +// a generic *exec.ExitError ("signal: killed") that is indistinguishable +// from an ordinary non-zero exit, so the timeout was silently reported as a +// regular failure. +func TestClassifyRunResult_Timeout(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + + cmd := exec.CommandContext(ctx, "sleep", "5") + err := cmd.Run() + if err == nil { + t.Fatal("expected the sleep command to be killed by the context deadline") + } + + status, gotErr := classifyRunResult(ctx, err) + if status != RunStatusTimeout { + t.Errorf("classifyRunResult() status = %v, want RunStatusTimeout", status) + } + if gotErr != nil { + t.Errorf("classifyRunResult() err = %v, want nil", gotErr) + } +} + +func TestClassifyRunResult_Success(t *testing.T) { + status, err := classifyRunResult(context.Background(), nil) + if status != RunStatusSuccess { + t.Errorf("classifyRunResult() status = %v, want RunStatusSuccess", status) + } + if err != nil { + t.Errorf("classifyRunResult() err = %v, want nil", err) + } +} + +func TestClassifyRunResult_Failure(t *testing.T) { + ctx := context.Background() + cmd := exec.CommandContext(ctx, "sh", "-c", "exit 3") + err := cmd.Run() + if err == nil { + t.Fatal("expected the command to exit with a non-zero status") + } + + status, gotErr := classifyRunResult(ctx, err) + if status != RunStatusFailure { + t.Errorf("classifyRunResult() status = %v, want RunStatusFailure", status) + } + if gotErr != nil { + t.Errorf("classifyRunResult() err = %v, want nil", gotErr) + } +} diff --git a/internal/worker/rundynamic.go b/internal/worker/rundynamic.go index e121294f..2e7d76f3 100644 --- a/internal/worker/rundynamic.go +++ b/internal/worker/rundynamic.go @@ -32,6 +32,14 @@ import ( // defaultDynamicAnalysisImage is container image name of the default dynamic analysis sandbox const defaultDynamicAnalysisImage = "gcr.io/ossf-malware-analysis/dynamic-analysis" +// dynamicAnalysisPhaseTimeout bounds how long a single dynamic analysis phase +// may run before it is treated as a timeout. It is kept below the sandbox +// container's own self-destruct timer (`sleep 30m`, see +// sandboxes/dynamicanalysis/Dockerfile) so that the Go side can detect and +// report the timeout via sandbox.RunStatusTimeout instead of racing the +// container's forced shutdown. +const dynamicAnalysisPhaseTimeout = 25 * time.Minute + /* DynamicAnalysisResult holds all data and status from RunDynamicAnalysis. @@ -263,6 +271,8 @@ func straceDebugLogFilename(pkg *pkgmanager.Pkg, phase analysisrun.DynamicPhase) func runDynamicAnalysisPhase(ctx context.Context, pkg *pkgmanager.Pkg, sb sandbox.Sandbox, analysisCmd string, phase analysisrun.DynamicPhase, result *DynamicAnalysisResult) error { phaseCtx := log.ContextWithAttrs(ctx, log.Label("phase", string(phase))) + phaseCtx, cancel := context.WithTimeout(phaseCtx, dynamicAnalysisPhaseTimeout) + defer cancel() startTime := time.Now() args := dynamicanalysis.MakeAnalysisArgs(pkg, phase)