acirenderer: check recursive dependencies.

This patch checks and returns an error when a recursive dependency is detected. A recursive dependency is something like: A --- A or A --- B --- A Having the same image referenced by different branches isn't a recursion: A --- B --- C \-- C
appc · Apr 11, 2016 · 6a3f5bb · 6a3f5bb
1 parent 80ab01d
commit 6a3f5bb
Show file tree

Hide file tree

Showing 2 changed files with 308 additions and 0 deletions.
diff --git a/pkg/acirenderer/acirenderer_test.go b/pkg/acirenderer/acirenderer_test.go
@@ -223,6 +223,300 @@ func TestGetUpperPWLM(t *testing.T) {
 	}
 }
 
+// Test dependency recursion.
+// This tests A --- A
+// If not checked it will cause an infinite recursion.
+func TestRecursiveDep1(t *testing.T) {
+	dir, err := ioutil.TempDir("", tstprefix)
+	if err != nil {
+		t.Fatalf("error creating tempdir: %v", err)
+	}
+	defer os.RemoveAll(dir)
+	ds := NewTestStore()
+
+	// B
+	imj := `
+		{
+		    "acKind": "ImageManifest",
+		    "acVersion": "0.1.1",
+		    "name": "example.com/test01"
+		}
+	`
+
+	imj, err = addDependencies(imj,
+		types.Dependency{
+			ImageName: "example.com/test01",
+		},
+	)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	entries := []*testTarEntry{
+		{
+			contents: imj,
+			header: &tar.Header{
+				Name: "manifest",
+				Size: int64(len(imj)),
+			},
+		},
+		// An empty dir
+		{
+			header: &tar.Header{
+				Name:     "rootfs/a",
+				Typeflag: tar.TypeDir,
+			},
+		},
+	}
+
+	key1, err := newTestACI(entries, dir, ds)
+
+	expectedFiles := []*fileInfo{
+		&fileInfo{path: "manifest", typeflag: tar.TypeReg},
+		&fileInfo{path: "rootfs/a", typeflag: tar.TypeDir},
+	}
+
+	err = checkRenderACI("example.com/test01", expectedFiles, ds)
+	if err == nil {
+		t.Fatalf("expected recursion error")
+	} else {
+		expectedErr := fmt.Errorf("recursion error, image with key %s already referenced by a parent image", key1)
+		if err.Error() != expectedErr.Error() {
+			t.Fatalf("expected error: %v, got: %v", expectedErr, err)
+		}
+	}
+}
+
+// Test dependency recursion.
+// This tests A --- B --- A
+// If not checked it will cause an infinite recursion.
+func TestRecursiveDep2(t *testing.T) {
+	dir, err := ioutil.TempDir("", tstprefix)
+	if err != nil {
+		t.Fatalf("error creating tempdir: %v", err)
+	}
+	defer os.RemoveAll(dir)
+	ds := NewTestStore()
+
+	// B
+	imj := `
+		{
+		    "acKind": "ImageManifest",
+		    "acVersion": "0.1.1",
+		    "name": "example.com/test01"
+		}
+	`
+
+	// Make B depend on A
+	imj, err = addDependencies(imj,
+		types.Dependency{
+			ImageName: "example.com/test02",
+		},
+	)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	entries := []*testTarEntry{
+		{
+			contents: imj,
+			header: &tar.Header{
+				Name: "manifest",
+				Size: int64(len(imj)),
+			},
+		},
+		// An empty dir
+		{
+			header: &tar.Header{
+				Name:     "rootfs/a",
+				Typeflag: tar.TypeDir,
+			},
+		},
+	}
+
+	key1, err := newTestACI(entries, dir, ds)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	// A
+	imj = `
+		{
+		    "acKind": "ImageManifest",
+		    "acVersion": "0.1.1",
+		    "name": "example.com/test02"
+		}
+	`
+
+	k1, _ := types.NewHash(key1)
+	imj, err = addDependencies(imj,
+		types.Dependency{
+			ImageName: "example.com/test01",
+			ImageID:   k1},
+	)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	entries = []*testTarEntry{
+		{
+			contents: imj,
+			header: &tar.Header{
+				Name: "manifest",
+				Size: int64(len(imj)),
+			},
+		},
+	}
+
+	key2, err := newTestACI(entries, dir, ds)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	expectedFiles := []*fileInfo{
+		&fileInfo{path: "manifest", typeflag: tar.TypeReg},
+		&fileInfo{path: "rootfs/a", typeflag: tar.TypeDir},
+	}
+
+	err = checkRenderACI("example.com/test02", expectedFiles, ds)
+	if err == nil {
+		t.Fatalf("expected recursion error")
+	} else {
+		expectedErr := fmt.Errorf("recursion error, image with key %s already referenced by a parent image", key2)
+		if err.Error() != expectedErr.Error() {
+			t.Fatalf("expected error: %v, got: %v", expectedErr, err)
+		}
+	}
+}
+
+// Having an image referenced inside different branches is not a recursion problem
+// This tests this dep tree:
+// A --- B --- C
+//   \-- C
+func TestNonRecursiveDep(t *testing.T) {
+	dir, err := ioutil.TempDir("", tstprefix)
+	if err != nil {
+		t.Fatalf("error creating tempdir: %v", err)
+	}
+	defer os.RemoveAll(dir)
+	ds := NewTestStore()
+
+	// C
+	imj := `
+		{
+		    "acKind": "ImageManifest",
+		    "acVersion": "0.1.1",
+		    "name": "example.com/test01"
+		}
+	`
+
+	entries := []*testTarEntry{
+		{
+			contents: imj,
+			header: &tar.Header{
+				Name: "manifest",
+				Size: int64(len(imj)),
+			},
+		},
+		// An empty dir
+		{
+			header: &tar.Header{
+				Name:     "rootfs/a",
+				Typeflag: tar.TypeDir,
+			},
+		},
+	}
+
+	key1, err := newTestACI(entries, dir, ds)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	// B
+	imj = `
+		{
+		    "acKind": "ImageManifest",
+		    "acVersion": "0.1.1",
+		    "name": "example.com/test02"
+		}
+	`
+
+	k1, _ := types.NewHash(key1)
+	imj, err = addDependencies(imj,
+		// C
+		types.Dependency{
+			ImageName: "example.com/test01",
+			ImageID:   k1},
+	)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	entries = []*testTarEntry{
+		{
+			contents: imj,
+			header: &tar.Header{
+				Name: "manifest",
+				Size: int64(len(imj)),
+			},
+		},
+	}
+
+	key2, err := newTestACI(entries, dir, ds)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	// A
+	imj = `
+		{
+		    "acKind": "ImageManifest",
+		    "acVersion": "0.1.1",
+		    "name": "example.com/test03"
+		}
+	`
+
+	k2, _ := types.NewHash(key2)
+	imj, err = addDependencies(imj,
+		// B
+		types.Dependency{
+			ImageName: "example.com/test02",
+			ImageID:   k2},
+		// C
+		types.Dependency{
+			ImageName: "example.com/test01",
+			ImageID:   k1},
+	)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	entries = []*testTarEntry{
+		{
+			contents: imj,
+			header: &tar.Header{
+				Name: "manifest",
+				Size: int64(len(imj)),
+			},
+		},
+	}
+
+	_, err = newTestACI(entries, dir, ds)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	expectedFiles := []*fileInfo{
+		&fileInfo{path: "manifest", typeflag: tar.TypeReg},
+		&fileInfo{path: "rootfs/a", typeflag: tar.TypeDir},
+	}
+
+	err = checkRenderACI("example.com/test03", expectedFiles, ds)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
 // Test an image with 1 dep. The parent provides a dir not provided by the image.
 func TestDirFromParent(t *testing.T) {
 	dir, err := ioutil.TempDir("", tstprefix)

diff --git a/pkg/acirenderer/resolve.go b/pkg/acirenderer/resolve.go
@@ -16,6 +16,7 @@ package acirenderer
 
 import (
 	"container/list"
+	"fmt"
 
 	"github.com/appc/spec/schema/types"
 )
@@ -75,6 +76,19 @@ func createDepList(key string, ap ACIRegistry) (Images, error) {
 			if err != nil {
 				return nil, err
 			}
+			// Check that an upper image is not the same as this dependency (same key)
+			// Walk up the current branch and check that this depKey is not already in this branch
+			curlevel := img.Level + 1
+			for tel := el; tel != nil; tel = tel.Prev() {
+				timg := tel.Value.(Image)
+				if timg.Level < curlevel {
+					if depKey == timg.Key {
+						return nil, fmt.Errorf("recursion error, image with key %s already referenced by a parent image", depKey)
+					}
+				}
+				curlevel = timg.Level
+			}
+
 			depimg = Image{Im: im, Key: depKey, Level: img.Level + 1}
 			imgsl.InsertAfter(depimg, el)
 		}