From fa31eeb3ea243e9f1e761c4e0871e4ffb78c7584 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Wed, 8 Sep 2021 15:16:19 +0200 Subject: [PATCH 1/4] Ignore DoesNotExist and ErrPermission in recursive ReadDir These can occur during recursion, but are not necessarily a problem of the ReadDir, so just assume such problematic files are not directories and return them. The caller can then figure out whether they are even needed at all, and an error should be returned. This allows simply ignoring a broken symlink when it would not even be used because of its extension. --- _testdata/brokensymlink | 1 + paths.go | 12 ++++++- paths_test.go | 72 +++++++++++++++++++++++++---------------- 3 files changed, 56 insertions(+), 29 deletions(-) create mode 120000 _testdata/brokensymlink diff --git a/_testdata/brokensymlink b/_testdata/brokensymlink new file mode 120000 index 0000000..e07da69 --- /dev/null +++ b/_testdata/brokensymlink @@ -0,0 +1 @@ +nonexistent \ No newline at end of file diff --git a/paths.go b/paths.go index 6021208..e425ed6 100644 --- a/paths.go +++ b/paths.go @@ -334,7 +334,17 @@ func (p *Path) ReadDirRecursive() (PathList, error) { paths.Add(path) if isDir, err := path.IsDirCheck(); err != nil { - return nil, err + // Do not report not exists (broken symlink) or + // permission errors, since then the entry + // *does* exist, so let our caller figure out + // whether these files are relevant and this is + // actually a problem. We cannot ignore *all* + // errors, since other errors during recursion + // (i.e. infinite recursion due to looping + // symlinks) should still be returned by us. + if !errors.Is(err, os.ErrNotExist) && !errors.Is(err, os.ErrPermission) { + return nil, err + } } else if isDir { subPaths, err := path.ReadDirRecursive() if err != nil { diff --git a/paths_test.go b/paths_test.go index da28432..64b8fdf 100644 --- a/paths_test.go +++ b/paths_test.go @@ -111,6 +111,19 @@ func TestPath(t *testing.T) { require.False(t, anotherFilePath.Exist()) require.True(t, anotherFilePath.NotExist()) + brokenFilePath := testPath.Join("brokensymlink") + pathEqualsTo(t, "_testdata/brokensymlink", brokenFilePath) + isDir, err = brokenFilePath.IsDirCheck() + require.False(t, isDir) + require.Error(t, err) + require.False(t, brokenFilePath.IsDir()) + require.False(t, brokenFilePath.IsNotDir()) + exist, err = brokenFilePath.ExistCheck() + require.False(t, exist) + require.NoError(t, err) + require.False(t, brokenFilePath.Exist()) + require.True(t, brokenFilePath.NotExist()) + list, err := folderPath.ReadDir() require.NoError(t, err) require.Len(t, list, 4) @@ -260,24 +273,25 @@ func TestReadDirRecursive(t *testing.T) { list, err := testPath.ReadDirRecursive() require.NoError(t, err) - require.Len(t, list, 16) + require.Len(t, list, 17) pathEqualsTo(t, "_testdata/anotherFile", list[0]) - pathEqualsTo(t, "_testdata/file", list[1]) - pathEqualsTo(t, "_testdata/folder", list[2]) - pathEqualsTo(t, "_testdata/folder/.hidden", list[3]) - pathEqualsTo(t, "_testdata/folder/file2", list[4]) - pathEqualsTo(t, "_testdata/folder/file3", list[5]) - pathEqualsTo(t, "_testdata/folder/subfolder", list[6]) - pathEqualsTo(t, "_testdata/folder/subfolder/file4", list[7]) - pathEqualsTo(t, "_testdata/symlinktofolder", list[8]) - pathEqualsTo(t, "_testdata/symlinktofolder/.hidden", list[9]) - pathEqualsTo(t, "_testdata/symlinktofolder/file2", list[10]) - pathEqualsTo(t, "_testdata/symlinktofolder/file3", list[11]) - pathEqualsTo(t, "_testdata/symlinktofolder/subfolder", list[12]) - pathEqualsTo(t, "_testdata/symlinktofolder/subfolder/file4", list[13]) - pathEqualsTo(t, "_testdata/test.txt", list[14]) - pathEqualsTo(t, "_testdata/test.txt.gz", list[15]) + pathEqualsTo(t, "_testdata/brokensymlink", list[1]) + pathEqualsTo(t, "_testdata/file", list[2]) + pathEqualsTo(t, "_testdata/folder", list[3]) + pathEqualsTo(t, "_testdata/folder/.hidden", list[4]) + pathEqualsTo(t, "_testdata/folder/file2", list[5]) + pathEqualsTo(t, "_testdata/folder/file3", list[6]) + pathEqualsTo(t, "_testdata/folder/subfolder", list[7]) + pathEqualsTo(t, "_testdata/folder/subfolder/file4", list[8]) + pathEqualsTo(t, "_testdata/symlinktofolder", list[9]) + pathEqualsTo(t, "_testdata/symlinktofolder/.hidden", list[10]) + pathEqualsTo(t, "_testdata/symlinktofolder/file2", list[11]) + pathEqualsTo(t, "_testdata/symlinktofolder/file3", list[12]) + pathEqualsTo(t, "_testdata/symlinktofolder/subfolder", list[13]) + pathEqualsTo(t, "_testdata/symlinktofolder/subfolder/file4", list[14]) + pathEqualsTo(t, "_testdata/test.txt", list[15]) + pathEqualsTo(t, "_testdata/test.txt.gz", list[16]) // Test symlink loop tmp, err := MkTempDir("", "") @@ -298,14 +312,15 @@ func TestFilterDirs(t *testing.T) { list, err := testPath.ReadDir() require.NoError(t, err) - require.Len(t, list, 6) + require.Len(t, list, 7) pathEqualsTo(t, "_testdata/anotherFile", list[0]) - pathEqualsTo(t, "_testdata/file", list[1]) - pathEqualsTo(t, "_testdata/folder", list[2]) - pathEqualsTo(t, "_testdata/symlinktofolder", list[3]) - pathEqualsTo(t, "_testdata/test.txt", list[4]) - pathEqualsTo(t, "_testdata/test.txt.gz", list[5]) + pathEqualsTo(t, "_testdata/brokensymlink", list[1]) + pathEqualsTo(t, "_testdata/file", list[2]) + pathEqualsTo(t, "_testdata/folder", list[3]) + pathEqualsTo(t, "_testdata/symlinktofolder", list[4]) + pathEqualsTo(t, "_testdata/test.txt", list[5]) + pathEqualsTo(t, "_testdata/test.txt.gz", list[6]) list.FilterDirs() require.Len(t, list, 2) @@ -318,14 +333,15 @@ func TestFilterOutDirs(t *testing.T) { list, err := testPath.ReadDir() require.NoError(t, err) - require.Len(t, list, 6) + require.Len(t, list, 7) pathEqualsTo(t, "_testdata/anotherFile", list[0]) - pathEqualsTo(t, "_testdata/file", list[1]) - pathEqualsTo(t, "_testdata/folder", list[2]) - pathEqualsTo(t, "_testdata/symlinktofolder", list[3]) - pathEqualsTo(t, "_testdata/test.txt", list[4]) - pathEqualsTo(t, "_testdata/test.txt.gz", list[5]) + pathEqualsTo(t, "_testdata/brokensymlink", list[1]) + pathEqualsTo(t, "_testdata/file", list[2]) + pathEqualsTo(t, "_testdata/folder", list[3]) + pathEqualsTo(t, "_testdata/symlinktofolder", list[4]) + pathEqualsTo(t, "_testdata/test.txt", list[5]) + pathEqualsTo(t, "_testdata/test.txt.gz", list[6]) list.FilterOutDirs() require.Len(t, list, 4) From 753b6ddbc835a7d7c4cfdd1cad87215511823e74 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Wed, 8 Sep 2021 15:56:56 +0200 Subject: [PATCH 2/4] Add Path.Lstat method --- paths.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/paths.go b/paths.go index e425ed6..c2e06b3 100644 --- a/paths.go +++ b/paths.go @@ -76,6 +76,13 @@ func (p *Path) Stat() (os.FileInfo, error) { return os.Stat(p.path) } +// Lstat returns a FileInfo like stat, but when the path points to a +// symlink, returns information about the symlink itself instead of the +// target like Stat(). +func (p *Path) Lstat() (os.FileInfo, error) { + return os.Lstat(p.path) +} + // Clone create a copy of the Path object func (p *Path) Clone() *Path { return New(p.path) From fed04092d13d3d6498c1439a572aab133a51de2f Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Wed, 8 Sep 2021 15:57:21 +0200 Subject: [PATCH 3/4] Ensure that CopyDirTo really ignores symlinks There was some code that claimed to ignore symlinks, but: - It would run *after* recursive directory copying, so any symlinks to directories would still be copied (potentially creating a destination directory that is a lot bigger than the original) - It checked for the symlink bit in the result of Stat(), but that resolves the symlink and returns the mode bits of the target, so that code would never fire. This fixes both problems by moving the check for symlinks up, and using Lstat() instead of Stat(), so symlinks are ignored as intended. --- paths.go | 16 ++++++++-------- paths_test.go | 4 ++++ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/paths.go b/paths.go index c2e06b3..b373274 100644 --- a/paths.go +++ b/paths.go @@ -435,12 +435,18 @@ func (p *Path) CopyDirTo(dst *Path) error { } for _, srcPath := range srcFiles { - srcPathInfo, err := srcPath.Stat() + srcPathInfo, err := srcPath.Lstat() if err != nil { return fmt.Errorf("getting stat info for %s: %s", srcPath, err) } - dstPath := dst.Join(srcPath.Base()) + // Skip symlinks. + if srcPathInfo.Mode()&os.ModeSymlink != 0 { + // TODO + continue + } + + dstPath := dst.Join(srcPath.Base()) if srcPathInfo.IsDir() { if err := srcPath.CopyDirTo(dstPath); err != nil { return fmt.Errorf("copying %s to %s: %s", srcPath, dstPath, err) @@ -448,12 +454,6 @@ func (p *Path) CopyDirTo(dst *Path) error { continue } - // Skip symlinks. - if srcPathInfo.Mode()&os.ModeSymlink != 0 { - // TODO - continue - } - if err := srcPath.CopyTo(dstPath); err != nil { return fmt.Errorf("copying %s to %s: %s", srcPath, dstPath, err) } diff --git a/paths_test.go b/paths_test.go index 64b8fdf..e365c62 100644 --- a/paths_test.go +++ b/paths_test.go @@ -243,6 +243,10 @@ func TestCopyDir(t *testing.T) { require.False(t, isdir) require.NoError(t, err) + exist, err = tmp.Join("dest", "symlinktofolder").ExistCheck() + require.False(t, exist) + require.NoError(t, err) + err = src.CopyDirTo(tmp.Join("dest")) require.Error(t, err, "copying dir to already existing") From 429170ecd10aa3fea4bcb28472697bb67836144e Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Wed, 8 Sep 2021 16:07:44 +0200 Subject: [PATCH 4/4] Fix FilterOutDirs on broken symlinks Broken symlinks would previously be treated as directories and filtered out, due to the use of IsNotDir (which defaults to false on Stat errors), but it makes more sense to only filter out elements that are certainly directories, so use IsDir (which also defaults to false on Stat errors) and negate its result. --- list.go | 2 +- paths_test.go | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/list.go b/list.go index 6f79bb4..99814f4 100644 --- a/list.go +++ b/list.go @@ -78,7 +78,7 @@ func (p *PathList) FilterDirs() { func (p *PathList) FilterOutDirs() { res := (*p)[:0] for _, path := range *p { - if path.IsNotDir() { + if !path.IsDir() { res = append(res, path) } } diff --git a/paths_test.go b/paths_test.go index e365c62..f5a976d 100644 --- a/paths_test.go +++ b/paths_test.go @@ -348,11 +348,12 @@ func TestFilterOutDirs(t *testing.T) { pathEqualsTo(t, "_testdata/test.txt.gz", list[6]) list.FilterOutDirs() - require.Len(t, list, 4) + require.Len(t, list, 5) pathEqualsTo(t, "_testdata/anotherFile", list[0]) - pathEqualsTo(t, "_testdata/file", list[1]) - pathEqualsTo(t, "_testdata/test.txt", list[2]) - pathEqualsTo(t, "_testdata/test.txt.gz", list[3]) + pathEqualsTo(t, "_testdata/brokensymlink", list[1]) + pathEqualsTo(t, "_testdata/file", list[2]) + pathEqualsTo(t, "_testdata/test.txt", list[3]) + pathEqualsTo(t, "_testdata/test.txt.gz", list[4]) } func TestEquivalentPaths(t *testing.T) {