scan: improve functionality with locally saved images

[Mab879]

Previously the locally saved image flow was at best confusing and at worst just wrong for some cases. This change simplified the handling of local images and reduces resources that were previously allocated for no good reason. Tests were also added to verify both docker and podman saved images work as expected. The application now expects all saved images to follow the OCI spec https://github.com/opencontainers/image-spec/blob/v1.1.1/image-layout.md

Back port of quay/clair-action#247

[vojtapolasek]

Thank you for the update, it seems as a good improvement. I consulted with Claude and it seems that we discovered few valid issues. Please have a look. Thank you.

[vojtapolasek]

add

       defer f.Close()

Othervise the file descriptor will leak.
You can test it by putting the following for example into image/leak_test.go:

package image

import (
	"context"
	"os"
	"testing"
)

func countFDs(t *testing.T) int {
	t.Helper()
	entries, err := os.ReadDir("/proc/self/fd")
	if err != nil {
		t.Fatalf("reading /proc/self/fd: %v", err)
	}
	return len(entries)
}

func TestManifestFromLocalFileDescriptorLeak(t *testing.T) {
	exportTar := writeTarFromTxtar(t, "testdata/docker_save.txtar")
	importDir := t.TempDir()
	ctx := context.Background()

	before := countFDs(t)
	for i := 0; i < 100; i++ {
		_, err := ManifestFromLocal(ctx, exportTar, importDir)
		if err != nil {
			t.Fatalf("iteration %d: %v", i, err)
		}
	}
	after := countFDs(t)

	leaked := after - before
	t.Logf("FDs before: %d, after: %d, leaked: %d", before, after, leaked)
	if leaked > 5 {
		t.Fatalf("file descriptor leak detected: %d descriptors leaked after 100 calls", leaked)
	}
}

[vojtapolasek]

Here is a problem Claude discovered and explained. I tried to verify it and it seems valid.

Problem

In image/manifest.go:252-265, when ManifestFromLocal processes gzip-compressed
image layers, it creates temporary files via os.CreateTemp to hold the decompressed
data. These temp files are never closed and never removed:

tmp, err := os.CreateTemp("", "layer-*.tar")  // created here
// ... data is decompressed into tmp ...
rAt = tmp  // assigned to interface, original *os.File reference is lost

The caller in scan.go:163-167 defers l.Close() on each layer, but that calls
claircore.Layer.Close(), which does not know about the underlying temp file.

Impact: Every scan of a gzip-compressed image leaks one temp file per
compressed layer, wasting file descriptors during the scan and leaving orphaned
files in the system temp directory permanently.

Fix

Add a single os.Remove(tmp.Name()) call immediately after the temp file is
written and rewound. On Linux, removing an open file unlinks it from the
directory while the file descriptor remains valid. The disk space is reclaimed
automatically when the fd is closed or the process exits. This requires no
signature changes and no caller modifications.

Patches

Apply both patches with git apply from the repository root.

Fix (image/manifest.go)

--- a/image/manifest.go
+++ b/image/manifest.go
@@ -262,6 +262,9 @@ func ManifestFromLocal(ctx context.Context, exportDir string, importDir string)
 					if _, err := tmp.Seek(0, io.SeekStart); err != nil {
 						return nil, fmt.Errorf("unable to rewind temp file: %w", err)
 					}
+					// Unlink the temp file now; the fd stays valid on Linux
+					// and the space is reclaimed once the fd is closed.
+					os.Remove(tmp.Name())
 					rAt = tmp
 				case "application/vnd.oci.image.layer.v1.tar", "application/vnd.docker.image.rootfs.diff.tar":

Test (image/manifest_test.go)

The test uses the existing podman_save.txtar fixture, which contains a
tar+gzip layer and therefore exercises the os.CreateTemp code path. It
snapshots layer-*.tar files in os.TempDir() before and after calling
ManifestFromLocal, and fails if any new ones appear.

Run with:

go test ./image/ -run TestLocalManifestGzipTempFileCleanup -v

--- a/image/manifest_test.go
+++ b/image/manifest_test.go
@@ -64,6 +64,40 @@ func writeTarFromTxtar(t *testing.T, txtarPath string) string {
 	return tmpTar
 }
 
+func TestLocalManifestGzipTempFileCleanup(t *testing.T) {
+	t.Parallel()
+	// Use the podman_save fixture which has a gzip-compressed layer,
+	// exercising the os.CreateTemp code path in ManifestFromLocal.
+	exportTar := writeTarFromTxtar(t, "testdata/podman_save.txtar")
+	importDir := t.TempDir()
+	ctx := context.Background()
+
+	// Snapshot existing layer-*.tar files so we only detect new leaks.
+	before := map[string]struct{}{}
+	if existing, _ := filepath.Glob(filepath.Join(os.TempDir(), "layer-*.tar")); existing != nil {
+		for _, p := range existing {
+			before[p] = struct{}{}
+		}
+	}
+
+	m, err := ManifestFromLocal(ctx, exportTar, importDir)
+	if err != nil {
+		t.Fatalf("ManifestFromLocal error: %v", err)
+	}
+
+	// The temp files should have been unlinked immediately after creation.
+	after, _ := filepath.Glob(filepath.Join(os.TempDir(), "layer-*.tar"))
+	for _, p := range after {
+		if _, ok := before[p]; !ok {
+			t.Errorf("leaked temp file created during test: %s", p)
+		}
+	}
+
+	// The layer data should still be readable through the open fd.
+	for _, l := range m.Layers {
+		l.Close()
+	}
+}
+
 func TestLocalManifest(t *testing.T) {
 	t.Parallel()
 	tests := []struct {

[vojtapolasek]

It seems that the importdir parameter is not used at all.

[vojtapolasek]

This is not a critical issue, but I think it is worth fixing while we are at it. Coauthored by Claude.

Fix: Nil layer panic when blob is missing from tar

Problem

In image/manifest.go:225, out.Layers is pre-allocated with
make([]*claircore.Layer, len(m.Layers)). The loop that follows iterates
through tar entries trying to match each one to a declared layer by digest.
If a layer blob is missing from the tar (corrupted archive, partial save,
etc.), its slot in out.Layers stays nil.

The caller in scan.go:163-167 unconditionally calls l.Close() on every
layer:

defer func() {
    for _, l := range mf.Layers {
        l.Close()  // panics if l is nil
    }
}()

This causes a nil pointer dereference panic instead of a clear error message.

Fix

Add a validation loop after the tar iteration that checks every slot is
populated. If any layer is missing, return a descriptive error identifying
which layer (by index and digest) was not found.

Patches

Apply both patches with git apply from the repository root.

Fix (image/manifest.go)

--- a/image/manifest.go
+++ b/image/manifest.go
@@ -284,5 +284,10 @@ func ManifestFromLocal(ctx context.Context, exportDir string, importDir string)
 		}
 	}
+	for i, l := range out.Layers {
+		if l == nil {
+			return nil, fmt.Errorf("layer %d (%s) not found in tar", i, m.Layers[i].Digest)
+		}
+	}
 	return out, nil
 
 }

Test (image/manifest_test.go)

The test uses a new fixture (testdata/missing_layer.txtar) containing a
valid OCI index and manifest that references a layer digest, but the
corresponding blob is absent from the tar. ManifestFromLocal must return
an error rather than a manifest with nil layer slots.

Run with:

go test ./image/ -run TestLocalManifestMissingLayer -v

--- a/image/manifest_test.go
+++ b/image/manifest_test.go
@@ -64,6 +64,22 @@ func writeTarFromTxtar(t *testing.T, txtarPath string) string {
 	return tmpTar
 }
 
+func TestLocalManifestMissingLayer(t *testing.T) {
+	t.Parallel()
+	// The fixture manifest references a layer blob that is absent from the tar.
+	// ManifestFromLocal must return an error instead of a manifest with nil layers.
+	exportTar := writeTarFromTxtar(t, "testdata/missing_layer.txtar")
+	importDir := t.TempDir()
+	ctx := context.Background()
+
+	_, err := ManifestFromLocal(ctx, exportTar, importDir)
+	if err == nil {
+		t.Fatal("expected error for missing layer blob, got nil")
+	}
+	if !strings.Contains(err.Error(), "not found in tar") {
+		t.Fatalf("unexpected error message: %v", err)
+	}
+}
+
 func TestLocalManifest(t *testing.T) {

Test fixture (image/testdata/missing_layer.txtar)

-- index.json --
{
    "schemaVersion": 2,
    "manifests": [
        {
            "mediaType": "application/vnd.oci.image.manifest.v1+json",
            "digest": "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
            "size": 999
        }
    ]
}
-- blobs/sha256/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa --
{
    "schemaVersion": 2,
    "mediaType": "application/vnd.oci.image.manifest.v1+json",
    "config": {
        "mediaType": "application/vnd.oci.image.config.v1+json",
        "digest": "sha256:not_used",
        "size": 1
    },
    "layers": [
        {
            "mediaType": "application/vnd.oci.image.layer.v1.tar",
            "digest": "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
            "size": 1024
        }
    ]
}