fix: surface the original analysis error instead of context cancellation (aquasecurity#10793)

Author: DmitriyLewen

pkg/fanal/analyzer/analyzer.go

@@ -490,6 +490,9 @@ func (ag AnalyzerGroup) AnalyzeFile(ctx context.Context, eg *errgroup.Group, lim
 		}
 
 		if err = limit.Acquire(ctx, 1); err != nil {
+			// The goroutine below (which closes rc) is not started on this path,
+			// so close the opened file here to avoid leaking the handle.
+			_ = rc.Close()
 			return xerrors.Errorf("semaphore acquire: %w", err)
 		}
 

pkg/fanal/artifact/image/image.go

@@ -465,8 +465,8 @@ func (a Artifact) inspectLayer(ctx context.Context, layer types.Layer, disabled
 	defer composite.Cleanup()
 
 	// Walk a tar layer
-	opqDirs, whFiles, err := a.walker.Walk(cr, func(filePath string, info os.FileInfo, opener analyzer.Opener) error {
-		if err = a.analyzer.AnalyzeFile(egCtx, eg, limit, result, "", filePath, info, opener, disabled, opts); err != nil {
+	opqDirs, whFiles, walkErr := a.walker.Walk(cr, func(filePath string, info os.FileInfo, opener analyzer.Opener) error {
+		if err := a.analyzer.AnalyzeFile(egCtx, eg, limit, result, "", filePath, info, opener, disabled, opts); err != nil {
 			return xerrors.Errorf("failed to analyze %s: %w", filePath, err)
 		}
 
@@ -487,14 +487,16 @@ func (a Artifact) inspectLayer(ctx context.Context, layer types.Layer, disabled
 
 		return nil
 	})
-	if err != nil {
-		return types.BlobInfo{}, xerrors.Errorf("walk error: %w", err)
-	}
 
-	// Wait for all the goroutine to finish and check errors
+	// errgroup cancels egCtx when an analysis goroutine fails, so the walk above can
+	// fail with context.Canceled and mask the real cause (e.g. a remote 429).
+	// Surface eg.Wait()'s error first; fall back to the walk error only when the group is clean.
 	if err = eg.Wait(); err != nil {
 		return types.BlobInfo{}, xerrors.Errorf("analyze error: %w", err)
 	}
+	if walkErr != nil {
+		return types.BlobInfo{}, xerrors.Errorf("walk error: %w", walkErr)
+	}
 
 	// Post-analysis
 	if err = a.analyzer.PostAnalyze(ctx, composite, result, opts); err != nil {

pkg/fanal/artifact/local/fs.go

@@ -212,23 +212,29 @@ func (a Artifact) Inspect(ctx context.Context) (artifact.Reference, error) {
 
 	// Use static paths instead of traversing the filesystem when all analyzers implement StaticPathAnalyzer
 	// so that we can analyze files faster
+	var walkErr error
 	if paths, canUseStaticPaths := a.analyzer.StaticPaths(a.artifactOption.DisabledAnalyzers); canUseStaticPaths {
 		// Analyze files in static paths
 		a.logger.Debug("Analyzing files in static paths")
 		if err = a.analyzeWithStaticPaths(egCtx, eg, limit, result, composite, opts, paths); err != nil {
-			return artifact.Reference{}, xerrors.Errorf("analyze with static paths: %w", err)
+			walkErr = xerrors.Errorf("analyze with static paths: %w", err)
 		}
 	} else {
 		// Analyze files by traversing the root directory
 		if err = a.analyzeWithRootDir(egCtx, eg, limit, result, composite, opts); err != nil {
-			return artifact.Reference{}, xerrors.Errorf("analyze with traversal: %w", err)
+			walkErr = xerrors.Errorf("analyze with traversal: %w", err)
 		}
 	}
 
-	// Wait for all the goroutine to finish.
+	// errgroup cancels egCtx when an analysis goroutine fails, so the walk above can
+	// fail with context.Canceled and mask the real cause (e.g. a remote 429).
+	// Surface eg.Wait()'s error first; fall back to the walk error only when the group is clean.
 	if err = eg.Wait(); err != nil {
 		return artifact.Reference{}, xerrors.Errorf("analyze error: %w", err)
 	}
+	if walkErr != nil {
+		return artifact.Reference{}, walkErr
+	}
 
 	// Post-analysis
 	if err = a.analyzer.PostAnalyze(ctx, composite, result, opts); err != nil {

pkg/fanal/artifact/local/fs_test.go

@@ -1,8 +1,11 @@
 package local
 
 import (
+	"context"
+	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -15,6 +18,7 @@ import (
 	"github.com/aquasecurity/trivy/pkg/fanal/types"
 	"github.com/aquasecurity/trivy/pkg/fanal/walker"
 	"github.com/aquasecurity/trivy/pkg/misconf"
+	trivytypes "github.com/aquasecurity/trivy/pkg/types"
 	"github.com/aquasecurity/trivy/pkg/uuid"
 
 	_ "github.com/aquasecurity/trivy/pkg/fanal/analyzer/config/all"
@@ -2663,3 +2667,40 @@ func Test_sanitizeRemoteURL(t *testing.T) {
 		})
 	}
 }
+
+// userErrorAnalyzer fails every matching file with a *types.UserError,
+// emulating a fatal analyzer error such as a remote Maven 429.
+type userErrorAnalyzer struct{}
+
+func (userErrorAnalyzer) Type() analyzer.Type { return "user-error-test" }
+func (userErrorAnalyzer) Version() int        { return 1 }
+func (userErrorAnalyzer) Required(filePath string, _ os.FileInfo) bool {
+	return strings.HasSuffix(filePath, ".usererror")
+}
+
+func (userErrorAnalyzer) Analyze(_ context.Context, _ analyzer.AnalysisInput) (*analyzer.AnalysisResult, error) {
+	return nil, &trivytypes.UserError{Message: "429 Too Many Requests"}
+}
+
+// TestArtifact_Inspect_AnalyzeErrorNotMasked is a regression test for #10790:
+// a fatal analyzer error (a remote Maven 429, as *types.UserError) must be
+// surfaced, not masked by the context.Canceled the walk hits after egCtx cancels.
+func TestArtifact_Inspect_AnalyzeErrorNotMasked(t *testing.T) {
+	dir := t.TempDir()
+	// Parallel=1 with several files: the first file cancels egCtx, so a later
+	// file's semaphore acquire fails with context.Canceled.
+	for i := range 10 {
+		require.NoError(t, os.WriteFile(
+			filepath.Join(dir, fmt.Sprintf("%d.usererror", i)), []byte("x"), 0o600))
+	}
+
+	analyzer.RegisterAnalyzer(userErrorAnalyzer{})
+	t.Cleanup(func() { analyzer.DeregisterAnalyzer("user-error-test") })
+
+	a, err := NewArtifact(dir, cache.NewMemoryCache(), walker.NewFS(), artifact.Option{Parallel: 1})
+	require.NoError(t, err)
+
+	_, err = a.Inspect(t.Context())
+	require.Error(t, err)
+	assert.ErrorContains(t, err, "429 Too Many Requests")
+}

pkg/fanal/artifact/vm/vm.go

@@ -102,9 +102,9 @@ func (a *Storage) Analyze(ctx context.Context, r *io.SectionReader) (types.BlobI
 	defer composite.Cleanup()
 
 	// TODO: Always walk from the root directory. Consider whether there is a need to be able to set optional
-	err = a.walker.Walk(r, "/", a.artifactOption.WalkerOption, func(filePath string, info os.FileInfo, opener analyzer.Opener) error {
+	walkErr := a.walker.Walk(r, "/", a.artifactOption.WalkerOption, func(filePath string, info os.FileInfo, opener analyzer.Opener) error {
 		path := strings.TrimPrefix(filePath, "/")
-		if err = a.analyzer.AnalyzeFile(egCtx, eg, limit, result, "/", path, info, opener, nil, opts); err != nil {
+		if err := a.analyzer.AnalyzeFile(egCtx, eg, limit, result, "/", path, info, opener, nil, opts); err != nil {
 			return xerrors.Errorf("analyze file (%s): %w", path, err)
 		}
 
@@ -126,14 +126,16 @@ func (a *Storage) Analyze(ctx context.Context, r *io.SectionReader) (types.BlobI
 
 		return nil
 	})
-	if err != nil {
-		return types.BlobInfo{}, xerrors.Errorf("walk vm error: %w", err)
-	}
 
-	// Wait for all the goroutine to finish.
+	// errgroup cancels egCtx when an analysis goroutine fails, so the walk above can
+	// fail with context.Canceled and mask the real cause (e.g. a remote 429).
+	// Surface eg.Wait()'s error first; fall back to the walk error only when the group is clean.
 	if err = eg.Wait(); err != nil {
 		return types.BlobInfo{}, xerrors.Errorf("analyze error: %w", err)
 	}
+	if walkErr != nil {
+		return types.BlobInfo{}, xerrors.Errorf("walk vm error: %w", walkErr)
+	}
 
 	// Post-analysis
 	if err = a.analyzer.PostAnalyze(ctx, composite, result, opts); err != nil {