From d33a4d07fd54a36f5c83c136496db308482ec57a Mon Sep 17 00:00:00 2001 From: mitchell Date: Mon, 18 Nov 2024 17:24:47 -0500 Subject: [PATCH 1/6] Handle recursive links in runtime sources. --- internal/smartlink/smartlink.go | 30 ++++++++-- internal/smartlink/smartlink_lin_mac.go | 6 -- internal/smartlink/smartlink_test.go | 75 +++++++++++++++++++++++++ pkg/runtime/depot.go | 4 +- pkg/runtime/links_windows.go | 2 +- 5 files changed, 104 insertions(+), 13 deletions(-) create mode 100644 internal/smartlink/smartlink_test.go diff --git a/internal/smartlink/smartlink.go b/internal/smartlink/smartlink.go index feb5134fa5..49fbe2d01d 100644 --- a/internal/smartlink/smartlink.go +++ b/internal/smartlink/smartlink.go @@ -10,7 +10,7 @@ import ( ) // LinkContents will link the contents of src to desc -func LinkContents(src, dest string) error { +func LinkContents(src, dest string, visited map[string]bool) error { if !fileutils.DirExists(src) { return errs.New("src dir does not exist: %s", src) } @@ -24,12 +24,23 @@ func LinkContents(src, dest string) error { return errs.Wrap(err, "Could not resolve src and dest paths") } + if visited == nil { + visited = make(map[string]bool) + } + if _, exists := visited[src]; exists { + // We've encountered a recursive link. This is most often the case when the resolved src has + // already been visited. In that case, just link the dest to the src (which may be a directory; + // this is fine). + return linkFile(src, dest) + } + visited[src] = true + entries, err := os.ReadDir(src) if err != nil { return errs.Wrap(err, "Reading dir %s failed", src) } for _, entry := range entries { - if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name())); err != nil { + if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name()), visited); err != nil { return errs.Wrap(err, "Link failed") } } @@ -39,13 +50,24 @@ func LinkContents(src, dest string) error { // Link creates a link from src to target. MS decided to support Symlinks but only if you opt into developer mode (go figure), // which we cannot reasonably force on our users. So on Windows we will instead create dirs and hardlinks. -func Link(src, dest string) error { +func Link(src, dest string, visited map[string]bool) error { var err error src, dest, err = resolvePaths(src, dest) if err != nil { return errs.Wrap(err, "Could not resolve src and dest paths") } + if visited == nil { + visited = make(map[string]bool) + } + if _, exists := visited[src]; exists { + // We've encountered a recursive link. This is most often the case when the resolved src has + // already been visited. In that case, just link the dest to the src (which may be a directory; + // this is fine). + return linkFile(src, dest) + } + visited[src] = true + if fileutils.IsDir(src) { if err := fileutils.Mkdir(dest); err != nil { return errs.Wrap(err, "could not create directory %s", dest) @@ -55,7 +77,7 @@ func Link(src, dest string) error { return errs.Wrap(err, "could not read directory %s", src) } for _, entry := range entries { - if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name())); err != nil { + if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name()), visited); err != nil { return errs.Wrap(err, "sub link failed") } } diff --git a/internal/smartlink/smartlink_lin_mac.go b/internal/smartlink/smartlink_lin_mac.go index b19f45d411..edc065f280 100644 --- a/internal/smartlink/smartlink_lin_mac.go +++ b/internal/smartlink/smartlink_lin_mac.go @@ -5,16 +5,10 @@ package smartlink import ( "os" - - "github.com/ActiveState/cli/internal/errs" - "github.com/ActiveState/cli/internal/fileutils" ) // file will create a symlink from src to dest, and falls back on a hardlink if no symlink is available. // This is a workaround for the fact that Windows does not support symlinks without admin privileges. func linkFile(src, dest string) error { - if fileutils.IsDir(src) { - return errs.New("src is a directory, not a file: %s", src) - } return os.Symlink(src, dest) } diff --git a/internal/smartlink/smartlink_test.go b/internal/smartlink/smartlink_test.go new file mode 100644 index 0000000000..5b52fbb0d3 --- /dev/null +++ b/internal/smartlink/smartlink_test.go @@ -0,0 +1,75 @@ +package smartlink + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/ActiveState/cli/internal/fileutils" + "github.com/stretchr/testify/require" +) + +func TestLinkContentsWithCircularLink(t *testing.T) { + srcDir, err := os.MkdirTemp("", "src") + require.NoError(t, err) + defer os.RemoveAll(srcDir) + + destDir, err := os.MkdirTemp("", "dest") + require.NoError(t, err) + defer os.RemoveAll(destDir) + + // Create test file structure: + // src/ + // ├── regular.txt + // └── subdir/ + // ├── circle -> subdir (circular link) + // └── subfile.txt + + testFile := filepath.Join(srcDir, "regular.txt") + err = os.WriteFile(testFile, []byte("test content"), 0644) + require.NoError(t, err) + + subDir := filepath.Join(srcDir, "subdir") + err = os.Mkdir(subDir, 0755) + require.NoError(t, err) + + subFile := filepath.Join(subDir, "subfile.txt") + err = os.WriteFile(subFile, []byte("sub content"), 0644) + require.NoError(t, err) + + circularLink := filepath.Join(subDir, "circle") + err = os.Symlink(subDir, circularLink) + require.NoError(t, err) + + err = LinkContents(srcDir, destDir, nil) + if runtime.GOOS == "windows" { + require.Error(t, err) + return // hard links to directories are not allowed on Windows + } + require.NoError(t, err) + + // Verify file structure. + destFile := filepath.Join(destDir, "regular.txt") + require.FileExists(t, destFile) + content, err := os.ReadFile(destFile) + require.NoError(t, err) + require.Equal(t, "test content", string(content)) + + destSubFile := filepath.Join(destDir, "subdir", "subfile.txt") + require.FileExists(t, destSubFile) + subContent, err := os.ReadFile(destSubFile) + require.NoError(t, err) + require.Equal(t, "sub content", string(subContent)) + + destCircular := filepath.Join(destDir, "subdir", "circle") + require.FileExists(t, destCircular) + target, err := fileutils.ResolveUniquePath(destCircular) + require.NoError(t, err) + srcCircular := filepath.Join(srcDir, "subdir") + if runtime.GOOS == "darwin" { + srcCircular, err = fileutils.ResolveUniquePath(srcCircular) // needed for full $TMPDIR resolution + require.NoError(t, err) + } + require.Equal(t, target, srcCircular) +} diff --git a/pkg/runtime/depot.go b/pkg/runtime/depot.go index 610418e952..c53a1ac1e6 100644 --- a/pkg/runtime/depot.go +++ b/pkg/runtime/depot.go @@ -177,7 +177,7 @@ func (d *depot) DeployViaLink(id strfmt.UUID, relativeSrc, absoluteDest string) } // Copy or link the artifact files, depending on whether the artifact in question relies on file transformations - if err := smartlink.LinkContents(absoluteSrc, absoluteDest); err != nil { + if err := smartlink.LinkContents(absoluteSrc, absoluteDest, nil); err != nil { return errs.Wrap(err, "failed to link artifact") } @@ -295,7 +295,7 @@ func (d *depot) Undeploy(id strfmt.UUID, relativeSrc, path string) error { for sharedFile, relinkSrc := range redeploys { switch deploy.Type { case deploymentTypeLink: - if err := smartlink.Link(relinkSrc, sharedFile); err != nil { + if err := smartlink.Link(relinkSrc, sharedFile, nil); err != nil { return errs.Wrap(err, "failed to relink file") } case deploymentTypeCopy: diff --git a/pkg/runtime/links_windows.go b/pkg/runtime/links_windows.go index 0bdbbe79e8..907882a66f 100644 --- a/pkg/runtime/links_windows.go +++ b/pkg/runtime/links_windows.go @@ -43,7 +43,7 @@ func supportsHardLinks(path string) (supported bool) { } logging.Debug("Attempting to link '%s' to '%s'", lnk, target) - err = smartlink.Link(target, lnk) + err = smartlink.Link(target, lnk, nil) if err != nil { logging.Debug("Test link creation failed: %v", err) return false From da5fee5f07321b6400f2d1e89eec3831a64376aa Mon Sep 17 00:00:00 2001 From: mitchell Date: Thu, 21 Nov 2024 13:47:57 -0500 Subject: [PATCH 2/6] Recurse into recursively linked directories not more than once. This results in a destination without directory links, which are not supported on Windows. --- internal/smartlink/smartlink.go | 33 ++++++++++--------------- internal/smartlink/smartlink_lin_mac.go | 6 +++++ internal/smartlink/smartlink_test.go | 30 +++++++++++----------- pkg/runtime/depot.go | 2 +- 4 files changed, 34 insertions(+), 37 deletions(-) diff --git a/internal/smartlink/smartlink.go b/internal/smartlink/smartlink.go index 49fbe2d01d..3df90034dd 100644 --- a/internal/smartlink/smartlink.go +++ b/internal/smartlink/smartlink.go @@ -10,7 +10,7 @@ import ( ) // LinkContents will link the contents of src to desc -func LinkContents(src, dest string, visited map[string]bool) error { +func LinkContents(src, dest string) error { if !fileutils.DirExists(src) { return errs.New("src dir does not exist: %s", src) } @@ -24,23 +24,12 @@ func LinkContents(src, dest string, visited map[string]bool) error { return errs.Wrap(err, "Could not resolve src and dest paths") } - if visited == nil { - visited = make(map[string]bool) - } - if _, exists := visited[src]; exists { - // We've encountered a recursive link. This is most often the case when the resolved src has - // already been visited. In that case, just link the dest to the src (which may be a directory; - // this is fine). - return linkFile(src, dest) - } - visited[src] = true - entries, err := os.ReadDir(src) if err != nil { return errs.Wrap(err, "Reading dir %s failed", src) } for _, entry := range entries { - if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name()), visited); err != nil { + if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name()), nil); err != nil { return errs.Wrap(err, "Link failed") } } @@ -50,7 +39,7 @@ func LinkContents(src, dest string, visited map[string]bool) error { // Link creates a link from src to target. MS decided to support Symlinks but only if you opt into developer mode (go figure), // which we cannot reasonably force on our users. So on Windows we will instead create dirs and hardlinks. -func Link(src, dest string, visited map[string]bool) error { +func Link(src, dest string, visited map[string]int) error { var err error src, dest, err = resolvePaths(src, dest) if err != nil { @@ -58,15 +47,19 @@ func Link(src, dest string, visited map[string]bool) error { } if visited == nil { - visited = make(map[string]bool) + visited = make(map[string]int) } - if _, exists := visited[src]; exists { + if count, exists := visited[src]; exists { // We've encountered a recursive link. This is most often the case when the resolved src has - // already been visited. In that case, just link the dest to the src (which may be a directory; - // this is fine). - return linkFile(src, dest) + // already been visited. We will recurse into the directory no more than once, so that any + // runtime paths that reference the link will not silently fail. + if count > 1 { + return nil + } + visited[src]++ + } else { + visited[src] = 1 } - visited[src] = true if fileutils.IsDir(src) { if err := fileutils.Mkdir(dest); err != nil { diff --git a/internal/smartlink/smartlink_lin_mac.go b/internal/smartlink/smartlink_lin_mac.go index edc065f280..b19f45d411 100644 --- a/internal/smartlink/smartlink_lin_mac.go +++ b/internal/smartlink/smartlink_lin_mac.go @@ -5,10 +5,16 @@ package smartlink import ( "os" + + "github.com/ActiveState/cli/internal/errs" + "github.com/ActiveState/cli/internal/fileutils" ) // file will create a symlink from src to dest, and falls back on a hardlink if no symlink is available. // This is a workaround for the fact that Windows does not support symlinks without admin privileges. func linkFile(src, dest string) error { + if fileutils.IsDir(src) { + return errs.New("src is a directory, not a file: %s", src) + } return os.Symlink(src, dest) } diff --git a/internal/smartlink/smartlink_test.go b/internal/smartlink/smartlink_test.go index 5b52fbb0d3..1222a0413d 100644 --- a/internal/smartlink/smartlink_test.go +++ b/internal/smartlink/smartlink_test.go @@ -3,10 +3,8 @@ package smartlink import ( "os" "path/filepath" - "runtime" "testing" - "github.com/ActiveState/cli/internal/fileutils" "github.com/stretchr/testify/require" ) @@ -42,14 +40,17 @@ func TestLinkContentsWithCircularLink(t *testing.T) { err = os.Symlink(subDir, circularLink) require.NoError(t, err) - err = LinkContents(srcDir, destDir, nil) - if runtime.GOOS == "windows" { - require.Error(t, err) - return // hard links to directories are not allowed on Windows - } + err = LinkContents(srcDir, destDir) require.NoError(t, err) // Verify file structure. + // src/ + // ├── regular.txt + // └── subdir/ + // ├── circle + // │ │ (no subdir/) + // │ └── subfile.txt + // └── subfile.txt destFile := filepath.Join(destDir, "regular.txt") require.FileExists(t, destFile) content, err := os.ReadFile(destFile) @@ -62,14 +63,11 @@ func TestLinkContentsWithCircularLink(t *testing.T) { require.NoError(t, err) require.Equal(t, "sub content", string(subContent)) - destCircular := filepath.Join(destDir, "subdir", "circle") - require.FileExists(t, destCircular) - target, err := fileutils.ResolveUniquePath(destCircular) + require.NoDirExists(t, filepath.Join(destDir, "subdir", "circle", "circle")) + + destCircularSubFile := filepath.Join(destDir, "subdir", "circle", "subfile.txt") + require.FileExists(t, destCircularSubFile) + subContent, err = os.ReadFile(destCircularSubFile) require.NoError(t, err) - srcCircular := filepath.Join(srcDir, "subdir") - if runtime.GOOS == "darwin" { - srcCircular, err = fileutils.ResolveUniquePath(srcCircular) // needed for full $TMPDIR resolution - require.NoError(t, err) - } - require.Equal(t, target, srcCircular) + require.Equal(t, "sub content", string(subContent)) } diff --git a/pkg/runtime/depot.go b/pkg/runtime/depot.go index c53a1ac1e6..8443fcb3ee 100644 --- a/pkg/runtime/depot.go +++ b/pkg/runtime/depot.go @@ -177,7 +177,7 @@ func (d *depot) DeployViaLink(id strfmt.UUID, relativeSrc, absoluteDest string) } // Copy or link the artifact files, depending on whether the artifact in question relies on file transformations - if err := smartlink.LinkContents(absoluteSrc, absoluteDest, nil); err != nil { + if err := smartlink.LinkContents(absoluteSrc, absoluteDest); err != nil { return errs.Wrap(err, "failed to link artifact") } From 9dd2ff9dab7d8102282f4a67950516a9fec05067 Mon Sep 17 00:00:00 2001 From: mitchell Date: Thu, 21 Nov 2024 19:07:41 -0500 Subject: [PATCH 3/6] Revert "Recurse into recursively linked directories not more than once." This reverts commit da5fee5f07321b6400f2d1e89eec3831a64376aa. --- internal/smartlink/smartlink.go | 33 +++++++++++++++---------- internal/smartlink/smartlink_lin_mac.go | 6 ----- internal/smartlink/smartlink_test.go | 30 +++++++++++----------- pkg/runtime/depot.go | 2 +- 4 files changed, 37 insertions(+), 34 deletions(-) diff --git a/internal/smartlink/smartlink.go b/internal/smartlink/smartlink.go index 3df90034dd..49fbe2d01d 100644 --- a/internal/smartlink/smartlink.go +++ b/internal/smartlink/smartlink.go @@ -10,7 +10,7 @@ import ( ) // LinkContents will link the contents of src to desc -func LinkContents(src, dest string) error { +func LinkContents(src, dest string, visited map[string]bool) error { if !fileutils.DirExists(src) { return errs.New("src dir does not exist: %s", src) } @@ -24,12 +24,23 @@ func LinkContents(src, dest string) error { return errs.Wrap(err, "Could not resolve src and dest paths") } + if visited == nil { + visited = make(map[string]bool) + } + if _, exists := visited[src]; exists { + // We've encountered a recursive link. This is most often the case when the resolved src has + // already been visited. In that case, just link the dest to the src (which may be a directory; + // this is fine). + return linkFile(src, dest) + } + visited[src] = true + entries, err := os.ReadDir(src) if err != nil { return errs.Wrap(err, "Reading dir %s failed", src) } for _, entry := range entries { - if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name()), nil); err != nil { + if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name()), visited); err != nil { return errs.Wrap(err, "Link failed") } } @@ -39,7 +50,7 @@ func LinkContents(src, dest string) error { // Link creates a link from src to target. MS decided to support Symlinks but only if you opt into developer mode (go figure), // which we cannot reasonably force on our users. So on Windows we will instead create dirs and hardlinks. -func Link(src, dest string, visited map[string]int) error { +func Link(src, dest string, visited map[string]bool) error { var err error src, dest, err = resolvePaths(src, dest) if err != nil { @@ -47,19 +58,15 @@ func Link(src, dest string, visited map[string]int) error { } if visited == nil { - visited = make(map[string]int) + visited = make(map[string]bool) } - if count, exists := visited[src]; exists { + if _, exists := visited[src]; exists { // We've encountered a recursive link. This is most often the case when the resolved src has - // already been visited. We will recurse into the directory no more than once, so that any - // runtime paths that reference the link will not silently fail. - if count > 1 { - return nil - } - visited[src]++ - } else { - visited[src] = 1 + // already been visited. In that case, just link the dest to the src (which may be a directory; + // this is fine). + return linkFile(src, dest) } + visited[src] = true if fileutils.IsDir(src) { if err := fileutils.Mkdir(dest); err != nil { diff --git a/internal/smartlink/smartlink_lin_mac.go b/internal/smartlink/smartlink_lin_mac.go index b19f45d411..edc065f280 100644 --- a/internal/smartlink/smartlink_lin_mac.go +++ b/internal/smartlink/smartlink_lin_mac.go @@ -5,16 +5,10 @@ package smartlink import ( "os" - - "github.com/ActiveState/cli/internal/errs" - "github.com/ActiveState/cli/internal/fileutils" ) // file will create a symlink from src to dest, and falls back on a hardlink if no symlink is available. // This is a workaround for the fact that Windows does not support symlinks without admin privileges. func linkFile(src, dest string) error { - if fileutils.IsDir(src) { - return errs.New("src is a directory, not a file: %s", src) - } return os.Symlink(src, dest) } diff --git a/internal/smartlink/smartlink_test.go b/internal/smartlink/smartlink_test.go index 1222a0413d..5b52fbb0d3 100644 --- a/internal/smartlink/smartlink_test.go +++ b/internal/smartlink/smartlink_test.go @@ -3,8 +3,10 @@ package smartlink import ( "os" "path/filepath" + "runtime" "testing" + "github.com/ActiveState/cli/internal/fileutils" "github.com/stretchr/testify/require" ) @@ -40,17 +42,14 @@ func TestLinkContentsWithCircularLink(t *testing.T) { err = os.Symlink(subDir, circularLink) require.NoError(t, err) - err = LinkContents(srcDir, destDir) + err = LinkContents(srcDir, destDir, nil) + if runtime.GOOS == "windows" { + require.Error(t, err) + return // hard links to directories are not allowed on Windows + } require.NoError(t, err) // Verify file structure. - // src/ - // ├── regular.txt - // └── subdir/ - // ├── circle - // │ │ (no subdir/) - // │ └── subfile.txt - // └── subfile.txt destFile := filepath.Join(destDir, "regular.txt") require.FileExists(t, destFile) content, err := os.ReadFile(destFile) @@ -63,11 +62,14 @@ func TestLinkContentsWithCircularLink(t *testing.T) { require.NoError(t, err) require.Equal(t, "sub content", string(subContent)) - require.NoDirExists(t, filepath.Join(destDir, "subdir", "circle", "circle")) - - destCircularSubFile := filepath.Join(destDir, "subdir", "circle", "subfile.txt") - require.FileExists(t, destCircularSubFile) - subContent, err = os.ReadFile(destCircularSubFile) + destCircular := filepath.Join(destDir, "subdir", "circle") + require.FileExists(t, destCircular) + target, err := fileutils.ResolveUniquePath(destCircular) require.NoError(t, err) - require.Equal(t, "sub content", string(subContent)) + srcCircular := filepath.Join(srcDir, "subdir") + if runtime.GOOS == "darwin" { + srcCircular, err = fileutils.ResolveUniquePath(srcCircular) // needed for full $TMPDIR resolution + require.NoError(t, err) + } + require.Equal(t, target, srcCircular) } diff --git a/pkg/runtime/depot.go b/pkg/runtime/depot.go index 8443fcb3ee..c53a1ac1e6 100644 --- a/pkg/runtime/depot.go +++ b/pkg/runtime/depot.go @@ -177,7 +177,7 @@ func (d *depot) DeployViaLink(id strfmt.UUID, relativeSrc, absoluteDest string) } // Copy or link the artifact files, depending on whether the artifact in question relies on file transformations - if err := smartlink.LinkContents(absoluteSrc, absoluteDest); err != nil { + if err := smartlink.LinkContents(absoluteSrc, absoluteDest, nil); err != nil { return errs.Wrap(err, "failed to link artifact") } From f0c778435c92d4a97717775f18abc52c7f61b86a Mon Sep 17 00:00:00 2001 From: mitchell Date: Thu, 21 Nov 2024 19:20:38 -0500 Subject: [PATCH 4/6] Make it explicit that re-encountering file involves a symlink source. --- internal/smartlink/smartlink.go | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/internal/smartlink/smartlink.go b/internal/smartlink/smartlink.go index 49fbe2d01d..2c62efd01f 100644 --- a/internal/smartlink/smartlink.go +++ b/internal/smartlink/smartlink.go @@ -24,17 +24,6 @@ func LinkContents(src, dest string, visited map[string]bool) error { return errs.Wrap(err, "Could not resolve src and dest paths") } - if visited == nil { - visited = make(map[string]bool) - } - if _, exists := visited[src]; exists { - // We've encountered a recursive link. This is most often the case when the resolved src has - // already been visited. In that case, just link the dest to the src (which may be a directory; - // this is fine). - return linkFile(src, dest) - } - visited[src] = true - entries, err := os.ReadDir(src) if err != nil { return errs.Wrap(err, "Reading dir %s failed", src) @@ -51,6 +40,8 @@ func LinkContents(src, dest string, visited map[string]bool) error { // Link creates a link from src to target. MS decided to support Symlinks but only if you opt into developer mode (go figure), // which we cannot reasonably force on our users. So on Windows we will instead create dirs and hardlinks. func Link(src, dest string, visited map[string]bool) error { + srcWasSymlink := isSymlink(src) + var err error src, dest, err = resolvePaths(src, dest) if err != nil { @@ -60,7 +51,7 @@ func Link(src, dest string, visited map[string]bool) error { if visited == nil { visited = make(map[string]bool) } - if _, exists := visited[src]; exists { + if _, exists := visited[src]; exists && srcWasSymlink { // We've encountered a recursive link. This is most often the case when the resolved src has // already been visited. In that case, just link the dest to the src (which may be a directory; // this is fine). @@ -153,6 +144,11 @@ func UnlinkContents(src, dest string) error { return nil } +func isSymlink(src string) bool { + target, err := fileutils.SymlinkTarget(src) + return err == nil && src != target +} + // resolvePaths will resolve src and dest to absolute paths and return them. // This is to ensure that we're always comparing apples to apples when doing string comparisons on paths. func resolvePaths(src, dest string) (string, string, error) { From b75482b5310cdb82448e8869fecee90ada254e39 Mon Sep 17 00:00:00 2001 From: mitchell Date: Fri, 22 Nov 2024 17:18:13 -0500 Subject: [PATCH 5/6] When deploying via link, deploy symlinks directly instead of resolving them first. --- internal/smartlink/smartlink.go | 32 +++++++++++----------------- internal/smartlink/smartlink_test.go | 2 +- pkg/runtime/depot.go | 4 ++-- pkg/runtime/links_windows.go | 2 +- 4 files changed, 17 insertions(+), 23 deletions(-) diff --git a/internal/smartlink/smartlink.go b/internal/smartlink/smartlink.go index 2c62efd01f..f32c806a6b 100644 --- a/internal/smartlink/smartlink.go +++ b/internal/smartlink/smartlink.go @@ -10,7 +10,7 @@ import ( ) // LinkContents will link the contents of src to desc -func LinkContents(src, dest string, visited map[string]bool) error { +func LinkContents(src, dest string) error { if !fileutils.DirExists(src) { return errs.New("src dir does not exist: %s", src) } @@ -29,7 +29,7 @@ func LinkContents(src, dest string, visited map[string]bool) error { return errs.Wrap(err, "Reading dir %s failed", src) } for _, entry := range entries { - if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name()), visited); err != nil { + if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name())); err != nil { return errs.Wrap(err, "Link failed") } } @@ -39,27 +39,21 @@ func LinkContents(src, dest string, visited map[string]bool) error { // Link creates a link from src to target. MS decided to support Symlinks but only if you opt into developer mode (go figure), // which we cannot reasonably force on our users. So on Windows we will instead create dirs and hardlinks. -func Link(src, dest string, visited map[string]bool) error { - srcWasSymlink := isSymlink(src) - +func Link(src, dest string) error { var err error src, dest, err = resolvePaths(src, dest) if err != nil { return errs.Wrap(err, "Could not resolve src and dest paths") } - if visited == nil { - visited = make(map[string]bool) - } - if _, exists := visited[src]; exists && srcWasSymlink { - // We've encountered a recursive link. This is most often the case when the resolved src has - // already been visited. In that case, just link the dest to the src (which may be a directory; - // this is fine). - return linkFile(src, dest) - } - visited[src] = true - if fileutils.IsDir(src) { + if isSymlink(src) { + // Links to directories are okay on Linux and macOS, but will fail on Windows. + // If we ever get here on Windows, the artifact being deployed is bad and there's nothing we + // can do about it except receive the report from Rollbar and report it internally. + return linkFile(src, dest) + } + if err := fileutils.Mkdir(dest); err != nil { return errs.Wrap(err, "could not create directory %s", dest) } @@ -68,7 +62,7 @@ func Link(src, dest string, visited map[string]bool) error { return errs.Wrap(err, "could not read directory %s", src) } for _, entry := range entries { - if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name()), visited); err != nil { + if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name())); err != nil { return errs.Wrap(err, "sub link failed") } } @@ -153,11 +147,11 @@ func isSymlink(src string) bool { // This is to ensure that we're always comparing apples to apples when doing string comparisons on paths. func resolvePaths(src, dest string) (string, string, error) { var err error - src, err = fileutils.ResolveUniquePath(src) + src, err = filepath.Abs(filepath.Clean(src)) if err != nil { return "", "", errs.Wrap(err, "Could not resolve src path") } - dest, err = fileutils.ResolveUniquePath(dest) + dest, err = filepath.Abs(filepath.Clean(dest)) if err != nil { return "", "", errs.Wrap(err, "Could not resolve dest path") } diff --git a/internal/smartlink/smartlink_test.go b/internal/smartlink/smartlink_test.go index 5b52fbb0d3..50a772254f 100644 --- a/internal/smartlink/smartlink_test.go +++ b/internal/smartlink/smartlink_test.go @@ -42,7 +42,7 @@ func TestLinkContentsWithCircularLink(t *testing.T) { err = os.Symlink(subDir, circularLink) require.NoError(t, err) - err = LinkContents(srcDir, destDir, nil) + err = LinkContents(srcDir, destDir) if runtime.GOOS == "windows" { require.Error(t, err) return // hard links to directories are not allowed on Windows diff --git a/pkg/runtime/depot.go b/pkg/runtime/depot.go index c53a1ac1e6..610418e952 100644 --- a/pkg/runtime/depot.go +++ b/pkg/runtime/depot.go @@ -177,7 +177,7 @@ func (d *depot) DeployViaLink(id strfmt.UUID, relativeSrc, absoluteDest string) } // Copy or link the artifact files, depending on whether the artifact in question relies on file transformations - if err := smartlink.LinkContents(absoluteSrc, absoluteDest, nil); err != nil { + if err := smartlink.LinkContents(absoluteSrc, absoluteDest); err != nil { return errs.Wrap(err, "failed to link artifact") } @@ -295,7 +295,7 @@ func (d *depot) Undeploy(id strfmt.UUID, relativeSrc, path string) error { for sharedFile, relinkSrc := range redeploys { switch deploy.Type { case deploymentTypeLink: - if err := smartlink.Link(relinkSrc, sharedFile, nil); err != nil { + if err := smartlink.Link(relinkSrc, sharedFile); err != nil { return errs.Wrap(err, "failed to relink file") } case deploymentTypeCopy: diff --git a/pkg/runtime/links_windows.go b/pkg/runtime/links_windows.go index 907882a66f..0bdbbe79e8 100644 --- a/pkg/runtime/links_windows.go +++ b/pkg/runtime/links_windows.go @@ -43,7 +43,7 @@ func supportsHardLinks(path string) (supported bool) { } logging.Debug("Attempting to link '%s' to '%s'", lnk, target) - err = smartlink.Link(target, lnk, nil) + err = smartlink.Link(target, lnk) if err != nil { logging.Debug("Test link creation failed: %v", err) return false From d1a7e18160cf6c9badad22e50292c3a85111d210 Mon Sep 17 00:00:00 2001 From: mitchell Date: Wed, 27 Nov 2024 11:08:08 -0500 Subject: [PATCH 6/6] Minimize changes from previous commit. --- internal/smartlink/smartlink.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/smartlink/smartlink.go b/internal/smartlink/smartlink.go index f32c806a6b..c101abde5e 100644 --- a/internal/smartlink/smartlink.go +++ b/internal/smartlink/smartlink.go @@ -40,6 +40,8 @@ func LinkContents(src, dest string) error { // Link creates a link from src to target. MS decided to support Symlinks but only if you opt into developer mode (go figure), // which we cannot reasonably force on our users. So on Windows we will instead create dirs and hardlinks. func Link(src, dest string) error { + originalSrc := src + var err error src, dest, err = resolvePaths(src, dest) if err != nil { @@ -47,11 +49,14 @@ func Link(src, dest string) error { } if fileutils.IsDir(src) { - if isSymlink(src) { + if fileutils.IsSymlink(originalSrc) { + // If the original src is a symlink, the resolved src is no longer a symlink and could point + // to a parent directory, resulting in a recursive directory structure. + // Avoid any potential problems by simply linking the original link to the target. // Links to directories are okay on Linux and macOS, but will fail on Windows. // If we ever get here on Windows, the artifact being deployed is bad and there's nothing we // can do about it except receive the report from Rollbar and report it internally. - return linkFile(src, dest) + return linkFile(originalSrc, dest) } if err := fileutils.Mkdir(dest); err != nil { @@ -138,20 +143,15 @@ func UnlinkContents(src, dest string) error { return nil } -func isSymlink(src string) bool { - target, err := fileutils.SymlinkTarget(src) - return err == nil && src != target -} - // resolvePaths will resolve src and dest to absolute paths and return them. // This is to ensure that we're always comparing apples to apples when doing string comparisons on paths. func resolvePaths(src, dest string) (string, string, error) { var err error - src, err = filepath.Abs(filepath.Clean(src)) + src, err = fileutils.ResolveUniquePath(src) if err != nil { return "", "", errs.Wrap(err, "Could not resolve src path") } - dest, err = filepath.Abs(filepath.Clean(dest)) + dest, err = fileutils.ResolveUniquePath(dest) if err != nil { return "", "", errs.Wrap(err, "Could not resolve dest path") }