fix(snap): release temp directories on every snap failure path

---------
Signed-off-by: Christopher Phillips <32073428+spiffcs@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Co-authored-by: Alex Goodman <wagoodman@users.noreply.github.com>

syft/source/snapsource/manifest.go

@@ -1,6 +1,7 @@
 package snapsource
 
 import (
+	"errors"
 	"fmt"
 
 	"github.com/goccy/go-yaml"
@@ -21,6 +22,11 @@ type snapManifest struct {
 
 const manifestLocation = "/meta/snap.yaml"
 
+// errNoManifest means the payload is a readable squashfs that simply does not carry a snap manifest.
+// This is distinct from a manifest that is present but unusable, and callers are expected to treat
+// the two differently.
+var errNoManifest = errors.New("no snap manifest file found")
+
 func parseManifest(resolver file.Resolver) (*snapManifest, error) {
 	locations, err := resolver.FilesByPath(manifestLocation)
 	if err != nil {
@@ -28,7 +34,7 @@ func parseManifest(resolver file.Resolver) (*snapManifest, error) {
 	}
 
 	if len(locations) == 0 {
-		return nil, fmt.Errorf("no snap manifest file found")
+		return nil, errNoManifest
 	}
 
 	if len(locations) > 1 {
@@ -48,9 +54,8 @@ func parseManifest(resolver file.Resolver) (*snapManifest, error) {
 		return nil, fmt.Errorf("unable to decode snap manifest file: %w", err)
 	}
 
-	if manifest.Name == "" || manifest.Version == "" {
-		return nil, fmt.Errorf("invalid snap manifest file: missing name or version")
-	}
-
+	// note: name and version are deliberately not validated here. a manifest that decoded but is
+	// missing them still carries usable fields (summary, base, grade, ...) and throwing it away would
+	// describe less than we know. the caller surfaces the missing identity.
 	return &manifest, nil
 }

syft/source/snapsource/snap.go

@@ -77,22 +77,33 @@ func getRemoteSnapFile(ctx context.Context, fs afero.Fs, getter intFile.Getter,
 	return newSnapFileFromRemote(ctx, fs, cfg, getter, info)
 }
 
-func newSnapFileFromRemote(ctx context.Context, fs afero.Fs, cfg Config, getter intFile.Getter, info *remoteSnap) (*snapFile, error) {
+func newSnapFileFromRemote(ctx context.Context, fs afero.Fs, cfg Config, getter intFile.Getter, info *remoteSnap) (_ *snapFile, err error) {
 	t, err := afero.TempDir(fs, "", "syft-snap-")
 	if err != nil {
 		return nil, fmt.Errorf("failed to create temp directory: %w", err)
 	}
 
+	closer := func() error {
+		return fs.RemoveAll(t)
+	}
+
+	// any failure past this point must not leave the temp directory (or a partially downloaded snap)
+	// behind. On success the caller owns the directory by way of snapFile.Cleanup.
+	defer func() {
+		if err == nil {
+			return
+		}
+		if closeErr := closer(); closeErr != nil {
+			log.WithFields("error", closeErr, "directory", t).Warn("unable to remove temp directory for snap")
+		}
+	}()
+
 	snapFilePath := path.Join(t, path.Base(info.URL))
 	err = downloadSnap(getter, info, snapFilePath)
 	if err != nil {
 		return nil, fmt.Errorf("failed to download snap file: %w", err)
 	}
 
-	closer := func() error {
-		return fs.RemoveAll(t)
-	}
-
 	mimeType, digests, err := getSnapFileInfo(ctx, fs, snapFilePath, cfg.DigestAlgorithms)
 	if err != nil {
 		return nil, err

syft/source/snapsource/snap_source.go

@@ -3,6 +3,7 @@ package snapsource
 import (
 	"context"
 	"crypto"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -119,7 +120,21 @@ func newFromPath(cfg Config, f *snapFile) (source.Source, error) {
 		closer:       f.Cleanup,
 	}
 
-	return s, s.extractManifest()
+	if err := s.extractManifest(); err != nil {
+		// It is tried on any path ending in .snap or .squashfs, so declining one is routine and
+		// a later provider usually succeeds. When the user asked for --from snap there is no
+		// fallback and this error reaches them intact.
+		log.WithFields("error", err, "path", f.Path).Debug("unable to read snap")
+
+		// never hand back a source alongside an error: callers that discard the source on error would
+		// leave behind the open squashfs file and the temp directory holding a downloaded snap
+		if closeErr := s.Close(); closeErr != nil {
+			log.WithFields("error", closeErr, "path", f.Path).Warn("unable to clean up snap source")
+		}
+		return nil, err
+	}
+
+	return s, nil
 }
 
 func (s *snapSource) extractManifest() error {
@@ -128,13 +143,26 @@ func (s *snapSource) extractManifest() error {
 		return fmt.Errorf("unable to create snap file resolver: %w", err)
 	}
 
+	// a manifest problem must never fail the source. this source is the only provider that can descend
+	// into squashfs (filesource cannot), so returning an error here would fall through to a provider
+	// that reports zero packages for a payload that has plenty. describe what we can instead.
 	manifest, err := parseManifest(r)
-	if err != nil {
-		return fmt.Errorf("unable to parse snap manifest file: %w", err)
+	switch {
+	case errors.Is(err, errNoManifest):
+		// not every squashfs payload is a snap, and one without a manifest is still worth cataloging
+		log.WithFields("path", s.squashfsPath).Debug("no snap manifest file found")
+		return nil
+	case err != nil:
+		// the manifest is there but unusable, which is worth surfacing: the SBOM will have no source
+		// name or version to show for it
+		log.WithFields("error", err, "path", s.squashfsPath).Warn("unable to parse snap manifest file")
+		return nil
 	}
 
-	if manifest != nil {
-		s.manifest = *manifest
+	s.manifest = *manifest
+
+	if s.manifest.Name == "" || s.manifest.Version == "" {
+		log.WithFields("path", s.squashfsPath).Warn("snap manifest is missing name or version")
 	}
 	return nil
 }
@@ -176,25 +204,34 @@ func (s snapSource) Describe() source.Description {
 	}
 }
 
+// Close releases everything the source holds. Every step runs even if an earlier one fails, since
+// bailing early would skip the temp directory removal and leave a downloaded snap on disk. Safe to
+// call more than once.
 func (s *snapSource) Close() error {
+	s.mutex.Lock()
+	defer s.mutex.Unlock()
+
+	var errs error
 	if s.squashFileCloser != nil {
 		if err := s.squashFileCloser(); err != nil {
-			return fmt.Errorf("unable to close snap resolver: %w", err)
+			errs = errors.Join(errs, fmt.Errorf("unable to close snap resolver: %w", err))
 		}
 		s.squashFileCloser = nil
 	}
 	s.resolver = nil
 	if s.fs != nil {
 		if err := s.fs.Close(); err != nil {
-			return fmt.Errorf("unable to close snap squashfs: %w", err)
+			errs = errors.Join(errs, fmt.Errorf("unable to close snap squashfs: %w", err))
 		}
+		s.fs = nil
 	}
 	if s.closer != nil {
 		if err := s.closer(); err != nil {
-			return fmt.Errorf("unable to close snap source: %w", err)
+			errs = errors.Join(errs, fmt.Errorf("unable to close snap source: %w", err))
 		}
+		s.closer = nil
 	}
-	return nil
+	return errs
 }
 
 func (s *snapSource) FileResolver(_ source.Scope) (file.Resolver, error) {

syft/source/snapsource/snap_source_test.go

@@ -2,15 +2,23 @@ package snapsource
 
 import (
 	"crypto"
+	"errors"
 	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+	"sync"
 	"testing"
 
+	diskFile "github.com/diskfs/go-diskfs/backend/file"
+	"github.com/diskfs/go-diskfs/filesystem/squashfs"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/anchore/go-homedir"
 	"github.com/anchore/stereoscope/pkg/image"
+	"github.com/anchore/syft/syft/source"
 )
 
 func TestNewFromLocal(t *testing.T) {
@@ -84,3 +92,205 @@ func TestNewFromLocal(t *testing.T) {
 		})
 	}
 }
+
+func TestNewFromPathOnResolverError(t *testing.T) {
+	tests := []struct {
+		name string
+		// remote snaps carry a cleanup closure for the temp directory holding the download; local snaps
+		// have none, since the file belongs to the user
+		remote          bool
+		wantCleanupRuns int
+		wantFileRemoved bool
+	}{
+		{
+			name:            "downloaded snap is cleaned up",
+			remote:          true,
+			wantCleanupRuns: 1,
+			wantFileRemoved: true,
+		},
+		{
+			name:            "local snap file is left in place",
+			remote:          false,
+			wantCleanupRuns: 0,
+			wantFileRemoved: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// the resolver reads the squashfs from the real filesystem, so the payload must live on disk
+			tempDir := t.TempDir()
+			snapPath := filepath.Join(tempDir, "not-really-a-snap.snap")
+			require.NoError(t, os.WriteFile(snapPath, []byte("not squashfs"), 0600))
+
+			var cleanupRuns int
+			f := &snapFile{
+				Path:     snapPath,
+				MimeType: "text/plain",
+			}
+			if tt.remote {
+				f.Cleanup = func() error {
+					cleanupRuns++
+					return os.RemoveAll(tempDir)
+				}
+			}
+
+			got, err := newFromPath(Config{Request: snapPath}, f)
+
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "unable to create snap file resolver")
+
+			// a source returned alongside an error is discarded by callers without being closed, which
+			// would leave the downloaded snap behind
+			assert.Nil(t, got, "expected nil source on error")
+			assert.Equal(t, tt.wantCleanupRuns, cleanupRuns)
+
+			_, statErr := os.Stat(snapPath)
+			if tt.wantFileRemoved {
+				assert.True(t, os.IsNotExist(statErr), "snap file was not cleaned up")
+			} else {
+				assert.NoError(t, statErr, "the user's snap file must never be removed")
+			}
+		})
+	}
+}
+
+// a manifest problem must never fail the source: snapsource is the only provider that can descend
+// into squashfs, so an error here falls through to filesource and every package inside the payload
+// silently disappears from the SBOM.
+func TestNewFromPathToleratesManifestProblems(t *testing.T) {
+	tests := []struct {
+		name        string
+		manifest    string // written to /meta/snap.yaml; empty means no manifest at all
+		wantName    string
+		wantVersion string
+		wantSummary string
+	}{
+		{
+			name:     "no manifest at all",
+			manifest: "",
+		},
+		{
+			name:     "manifest is not valid yaml",
+			manifest: "name: etcd\n\tversion: 3.4\n",
+		},
+		{
+			name:        "manifest is missing name and version",
+			manifest:    "summary: a thing\nbase: core22\n",
+			wantSummary: "a thing",
+		},
+		{
+			name:        "manifest is complete",
+			manifest:    "name: etcd\nversion: 3.4.0\nsummary: a thing\n",
+			wantName:    "etcd",
+			wantVersion: "3.4.0",
+			wantSummary: "a thing",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			snapPath := writeSquashfs(t, tt.manifest)
+
+			src, err := newFromPath(Config{Request: snapPath}, &snapFile{Path: snapPath})
+			require.NoError(t, err, "a manifest problem must not fail the source")
+			require.NotNil(t, src)
+			t.Cleanup(func() { _ = src.Close() })
+
+			desc := src.Describe()
+			assert.Equal(t, tt.wantName, desc.Name)
+			assert.Equal(t, tt.wantVersion, desc.Version)
+
+			meta, ok := desc.Metadata.(source.SnapMetadata)
+			require.True(t, ok, "expected snap metadata")
+			assert.Equal(t, tt.wantSummary, meta.Summary, "fields that did decode must survive")
+
+			// the whole point: the payload is still walkable, so cataloging sees what is inside
+			r, err := src.FileResolver(source.SquashedScope)
+			require.NoError(t, err)
+			locations, err := r.FilesByPath("/payload.txt")
+			require.NoError(t, err)
+			assert.Len(t, locations, 1, "contents must remain catalogable regardless of the manifest")
+		})
+	}
+}
+
+func TestSnapSourceClose(t *testing.T) {
+	newSource := func(closeErr, cleanupErr error) (*snapSource, *int) {
+		var cleanupRuns int
+		return &snapSource{
+			mutex: &sync.Mutex{},
+			squashFileCloser: func() error {
+				return closeErr
+			},
+			closer: func() error {
+				cleanupRuns++
+				return cleanupErr
+			},
+		}, &cleanupRuns
+	}
+
+	t.Run("temp directory is removed even when closing the file handle fails", func(t *testing.T) {
+		s, cleanupRuns := newSource(errors.New("handle is busy"), nil)
+
+		err := s.Close()
+
+		// the whole point: a failure here used to return early and skip the cleanup below, leaving a
+		// downloaded snap on disk
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "handle is busy")
+		assert.Equal(t, 1, *cleanupRuns, "temp directory removal was skipped")
+	})
+
+	t.Run("every failure is reported, not just the first", func(t *testing.T) {
+		s, _ := newSource(errors.New("handle is busy"), errors.New("directory is busy"))
+
+		err := s.Close()
+
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "handle is busy")
+		assert.Contains(t, err.Error(), "directory is busy")
+	})
+
+	t.Run("is safe to call twice", func(t *testing.T) {
+		s, cleanupRuns := newSource(nil, nil)
+
+		require.NoError(t, s.Close())
+		require.NoError(t, s.Close())
+		assert.Equal(t, 1, *cleanupRuns, "cleanup must not run again on a second close")
+	})
+}
+
+// writeSquashfs builds a real squashfs image containing /payload.txt and, when manifest is non-empty,
+// /meta/snap.yaml. Returns the path to the image.
+func writeSquashfs(t *testing.T, manifest string) string {
+	t.Helper()
+
+	snapPath := filepath.Join(t.TempDir(), "test.snap")
+	f, err := os.Create(snapPath)
+	require.NoError(t, err)
+	defer func() { require.NoError(t, f.Close()) }()
+
+	fs, err := squashfs.Create(diskFile.New(f, false), 0, 0, 4096)
+	require.NoError(t, err)
+
+	write := func(path, contents string) {
+		t.Helper()
+		w, err := fs.OpenFile(path, os.O_CREATE|os.O_RDWR)
+		require.NoError(t, err)
+		_, err = w.Write([]byte(contents))
+		require.NoError(t, err)
+	}
+
+	// note: go-diskfs wants paths relative to the image root, without a leading slash
+	require.NoError(t, fs.Mkdir("."))
+	write("payload.txt", "hello")
+
+	if manifest != "" {
+		require.NoError(t, fs.Mkdir("meta"))
+		write(strings.TrimPrefix(manifestLocation, "/"), manifest)
+	}
+
+	require.NoError(t, fs.Finalize(squashfs.FinalizeOptions{}))
+	return snapPath
+}

syft/source/snapsource/snap_test.go

@@ -264,6 +264,10 @@ func TestNewSnapFileFromRemote(t *testing.T) {
 
 				_, err = fs.Stat(result.Path)
 				assert.True(t, os.IsNotExist(err))
+
+				// cleanup owns the whole temp directory, not just the snap file within it
+				_, err = fs.Stat(filepath.Dir(result.Path))
+				assert.True(t, os.IsNotExist(err), "temp directory outlived the cleanup closure")
 			},
 		},
 		{
@@ -315,6 +319,75 @@ func TestNewSnapFileFromRemote(t *testing.T) {
 			expectError: true,
 			errorMsg:    "failed to download snap file",
 		},
+		{
+			name: "download writes partial data then fails",
+			cfg: Config{
+				DigestAlgorithms: []crypto.Hash{crypto.SHA256},
+			},
+			info: &remoteSnap{
+				snapIdentity: snapIdentity{
+					Name:         "partial-snap",
+					Channel:      "stable",
+					Architecture: "amd64",
+				},
+				URL: "https://api.snapcraft.io/download/partial_snap.snap",
+			},
+			setupMock: func(mockGetter *mockFileGetter, fs afero.Fs) {
+				mockGetter.On("GetFile", mock.AnythingOfType("string"), "https://api.snapcraft.io/download/partial_snap.snap", mock.Anything).Run(func(args mock.Arguments) {
+					// simulate a transfer that writes some bytes before the connection drops
+					require.NoError(t, createMockSquashfsFile(fs, args.String(0)))
+				}).Return(fmt.Errorf("connection reset by peer"))
+			},
+			expectError: true,
+			errorMsg:    "failed to download snap file",
+		},
+		{
+			// isSquashFSFile trusts the .snap extension, so non-squashfs content under that name is accepted
+			name: "downloaded file has a snap extension but non-snap content",
+			cfg: Config{
+				DigestAlgorithms: []crypto.Hash{crypto.SHA256},
+			},
+			info: &remoteSnap{
+				snapIdentity: snapIdentity{
+					Name:         "not-a-snap",
+					Channel:      "stable",
+					Architecture: "amd64",
+				},
+				URL: "https://api.snapcraft.io/download/not_a_snap.snap",
+			},
+			setupMock: func(mockGetter *mockFileGetter, fs afero.Fs) {
+				mockGetter.On("GetFile", mock.AnythingOfType("string"), "https://api.snapcraft.io/download/not_a_snap.snap", mock.Anything).Run(func(args mock.Arguments) {
+					require.NoError(t, afero.WriteFile(fs, args.String(0), []byte("not squashfs"), 0644))
+				}).Return(nil)
+			},
+			expectError: false,
+			validate: func(t *testing.T, result *snapFile, fs afero.Fs) {
+				assert.NotNil(t, result)
+				assert.Contains(t, result.Path, "not_a_snap.snap")
+				assert.NotNil(t, result.Cleanup)
+			},
+		},
+		{
+			name: "downloaded file is not a snap",
+			cfg: Config{
+				DigestAlgorithms: []crypto.Hash{crypto.SHA256},
+			},
+			info: &remoteSnap{
+				snapIdentity: snapIdentity{
+					Name:         "not-a-snap",
+					Channel:      "stable",
+					Architecture: "amd64",
+				},
+				URL: "https://api.snapcraft.io/download/not_a_snap.tar.gz",
+			},
+			setupMock: func(mockGetter *mockFileGetter, fs afero.Fs) {
+				mockGetter.On("GetFile", mock.AnythingOfType("string"), "https://api.snapcraft.io/download/not_a_snap.tar.gz", mock.Anything).Run(func(args mock.Arguments) {
+					require.NoError(t, afero.WriteFile(fs, args.String(0), []byte("not squashfs"), 0644))
+				}).Return(nil)
+			},
+			expectError: true,
+			errorMsg:    "not a valid squashfs/snap file",
+		},
 	}
 
 	for _, tt := range tests {
@@ -334,8 +407,18 @@ func TestNewSnapFileFromRemote(t *testing.T) {
 					assert.Contains(t, err.Error(), tt.errorMsg)
 				}
 				assert.Nil(t, result)
+
+				// the temp directory and any partially downloaded payload must not be left behind. The
+				// download destination handed to the getter lives directly under the temp directory.
+				require.Len(t, mockGetter.Calls, 1)
+				tempDir := filepath.Dir(mockGetter.Calls[0].Arguments.String(0))
+				_, statErr := fs.Stat(tempDir)
+				assert.True(t, os.IsNotExist(statErr), "temp directory %q was not cleaned up after failure", tempDir)
 			} else {
 				require.NoError(t, err)
+				if result.Cleanup != nil {
+					t.Cleanup(func() { _ = result.Cleanup() })
+				}
 				if tt.validate != nil {
 					tt.validate(t, result, fs)
 				}