From a545a378ec9a635f5daf15c10c0128384a871bca Mon Sep 17 00:00:00 2001
From: Alex Goodman <alex.goodman@anchore.com>
Date: Wed, 30 Nov 2022 10:19:24 -0500
Subject: [PATCH] fix link cycles with non-existent paths

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
---
 pkg/filetree/filetree.go      | 23 +++++++++++++++++------
 pkg/filetree/filetree_test.go | 18 ++++++++++++++++++
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/pkg/filetree/filetree.go b/pkg/filetree/filetree.go
index fd30f81a..43df31af 100644
--- a/pkg/filetree/filetree.go
+++ b/pkg/filetree/filetree.go
@@ -168,7 +168,7 @@ func (t *FileTree) node(p file.Path, strategy linkResolutionStrategy) (*filenode
 	var currentNode *filenode.FileNode
 	var err error
 	if strategy.FollowAncestorLinks {
-		currentNode, err = t.resolveAncestorLinks(normalizedPath)
+		currentNode, err = t.resolveAncestorLinks(normalizedPath, nil)
 		if err != nil {
 			return currentNode, err
 		}
@@ -185,14 +185,14 @@ func (t *FileTree) node(p file.Path, strategy linkResolutionStrategy) (*filenode
 	}
 
 	if strategy.FollowBasenameLinks {
-		currentNode, err = t.resolveNodeLinks(currentNode, !strategy.DoNotFollowDeadBasenameLinks)
+		currentNode, err = t.resolveNodeLinks(currentNode, !strategy.DoNotFollowDeadBasenameLinks, nil)
 	}
 	return currentNode, err
 }
 
 // return FileNode of the basename in the given path (no resolution is done at or past the basename). Note: it is
 // assumed that the given path has already been normalized.
-func (t *FileTree) resolveAncestorLinks(path file.Path) (*filenode.FileNode, error) {
+func (t *FileTree) resolveAncestorLinks(path file.Path, attemptedPaths internal.Set) (*filenode.FileNode, error) {
 	// performance optimization... see if there is a node at the path (as if it is a real path). If so,
 	// use it, otherwise, continue with ancestor resolution
 	currentNode, err := t.node(path, linkResolutionStrategy{})
@@ -248,7 +248,7 @@ func (t *FileTree) resolveAncestorLinks(path file.Path) (*filenode.FileNode, err
 		// links until the next Node is resolved (or not).
 		isLastPart := idx == len(pathParts)-1
 		if !isLastPart && currentNode.IsLink() {
-			currentNode, err = t.resolveNodeLinks(currentNode, true)
+			currentNode, err = t.resolveNodeLinks(currentNode, true, attemptedPaths)
 			if err != nil {
 				// only expected to happen on cycles
 				return currentNode, err
@@ -266,11 +266,16 @@ func (t *FileTree) resolveAncestorLinks(path file.Path) (*filenode.FileNode, err
 
 // followNode takes the given FileNode and resolves all links at the base of the real path for the node (this implies
 // that NO ancestors are considered).
-func (t *FileTree) resolveNodeLinks(n *filenode.FileNode, followDeadBasenameLinks bool) (*filenode.FileNode, error) {
+func (t *FileTree) resolveNodeLinks(n *filenode.FileNode, followDeadBasenameLinks bool, attemptedPaths internal.Set) (*filenode.FileNode, error) {
 	if n == nil {
 		return nil, fmt.Errorf("cannot resolve links with nil Node given")
 	}
 
+	// we need to short-circuit link resolution that never resolves (cycles) due to a cycle referencing nodes that do not exist
+	if attemptedPaths == nil {
+		attemptedPaths = internal.NewStringSet()
+	}
+
 	// note: this assumes that callers are passing paths in which the constituent parts are NOT symlinks
 	var lastNode *filenode.FileNode
 
@@ -318,8 +323,14 @@ func (t *FileTree) resolveNodeLinks(n *filenode.FileNode, followDeadBasenameLink
 		// preserve the current Node for the next loop (in case we shouldn't follow a potentially dead link)
 		lastNode = currentNode
 
+		// break any cycles with non-existent paths (before attempting to look the path up again)
+		if attemptedPaths.Contains(string(nextPath)) {
+			return nil, ErrLinkCycleDetected
+		}
+
 		// get the next Node (based on the next path)
-		currentNode, err = t.resolveAncestorLinks(nextPath)
+		attemptedPaths.Add(string(nextPath))
+		currentNode, err = t.resolveAncestorLinks(nextPath, attemptedPaths)
 		if err != nil {
 			// only expected to occur upon cycle detection
 			return currentNode, err
diff --git a/pkg/filetree/filetree_test.go b/pkg/filetree/filetree_test.go
index d6d71e84..0d3e0c27 100644
--- a/pkg/filetree/filetree_test.go
+++ b/pkg/filetree/filetree_test.go
@@ -3,6 +3,7 @@ package filetree
 import (
 	"errors"
 	"fmt"
+	"github.com/stretchr/testify/require"
 	"testing"
 
 	"github.com/anchore/stereoscope/internal"
@@ -787,6 +788,23 @@ func TestFileTree_File_CycleDetection(t *testing.T) {
 
 }
 
+func TestFileTree_File_DeadCycleDetection(t *testing.T) {
+	tr := NewFileTree()
+	_, err := tr.AddSymLink("/somewhere/acorn", "noobaa-core/../acorn/bin/acorn")
+	require.NoError(t, err)
+
+	// the test.... do we stop when a cycle is detected?
+	exists, _, err := tr.File("/somewhere/acorn", FollowBasenameLinks)
+	if err != ErrLinkCycleDetected {
+		t.Fatalf("should have gotten an error on resolving a file")
+	}
+
+	if exists {
+		t.Errorf("resolution should not exist in cycle")
+	}
+
+}
+
 func TestFileTree_AllFiles(t *testing.T) {
 	tr := NewFileTree()