From 1e13247d6df023cea2d662e5ef4b2ecb9acb28a8 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 27 Sep 2022 17:08:08 +0200 Subject: [PATCH] pkg/idtools: mkdirAs(): fix infinite loops and repeated "chown" This fixes an inifinite loop in mkdirAs(), used by `MkdirAllAndChown`, `MkdirAndChown`, and `MkdirAllAndChownNew`, as well as directories being chown'd multiple times when relative paths are used. The for loop in this function was incorrectly assuming that; 1. `filepath.Dir()` would always return the parent directory of any given path 2. traversing any given path to ultimately result in "/" While this is correct for absolute and "cleaned" paths, both assumptions are incorrect in some variations of "path"; 1. for paths with a trailing path-separator ("some/path/"), or dot ("."), `filepath.Dir()` considers the (implicit) "." to be a location _within_ the directory, and returns "some/path" as ("parent") directory. This resulted in the path itself to be included _twice_ in the list of paths to chown. 2. for relative paths ("./some-path", "../some-path"), "traversing" the path would never end in "/", causing the for loop to run indefinitely: ```go // walk back to "/" looking for directories which do not exist // and add them to the paths array for chown after creation dirPath := path for { dirPath = filepath.Dir(dirPath) if dirPath == "/" { break } if _, err := os.Stat(dirPath); err != nil && os.IsNotExist(err) { paths = append(paths, dirPath) } } ``` A _partial_ mitigation for this would be to use `filepath.Clean()` before using the path (while `filepath.Dir()` _does_ call `filepath.Clean()`, it only does so _after_ some processing, so only cleans the result). Doing so would prevent the double chown from happening, but would not prevent the "final" path to be "." or ".." (in the relative path case), still causing an infinite loop, or additional checks for "." / ".." to be needed. | path | filepath.Dir(path) | filepath.Dir(filepath.Clean(path)) | |----------------|--------------------|------------------------------------| | some-path | . | . | | ./some-path | . | . | | ../some-path | .. | .. | | some/path/ | some/path | some | | ./some/path/ | some/path | some | | ../some/path/ | ../some/path | ../some | | some/path/. | some/path | some | | ./some/path/. | some/path | some | | ../some/path/. | ../some/path | ../some | | /some/path/ | /some/path | /some | | /some/path/. | /some/path | /some | Instead, this patch adds a `filepath.Abs()` to the function, so make sure that paths are both cleaned, and not resulting in an infinite loop. Signed-off-by: Sebastiaan van Stijn --- pkg/idtools/idtools_unix.go | 4 ++ pkg/idtools/idtools_unix_test.go | 92 ++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+) diff --git a/pkg/idtools/idtools_unix.go b/pkg/idtools/idtools_unix.go index 2f7cac8caa763..98330d2b23da0 100644 --- a/pkg/idtools/idtools_unix.go +++ b/pkg/idtools/idtools_unix.go @@ -30,6 +30,10 @@ func mkdirAs(path string, mode os.FileMode, owner Identity, mkAll, chownExisting // chown the full directory path if it exists var paths []string + path, err := filepath.Abs(path) + if err != nil { + return err + } stat, err := system.Stat(path) if err == nil { diff --git a/pkg/idtools/idtools_unix_test.go b/pkg/idtools/idtools_unix_test.go index 162d02578f78c..32803477749c1 100644 --- a/pkg/idtools/idtools_unix_test.go +++ b/pkg/idtools/idtools_unix_test.go @@ -127,6 +127,98 @@ func TestMkdirAllAndChownNew(t *testing.T) { assert.NilError(t, compareTrees(testTree, verifyTree)) } +func TestMkdirAllAndChownNewRelative(t *testing.T) { + RequiresRoot(t) + + tests := []struct { + in string + out []string + }{ + { + in: "dir1", + out: []string{"dir1"}, + }, + { + in: "dir2/subdir2", + out: []string{"dir2", "dir2/subdir2"}, + }, + { + in: "dir3/subdir3/", + out: []string{"dir3", "dir3/subdir3"}, + }, + { + in: "dir4/subdir4/.", + out: []string{"dir4", "dir4/subdir4"}, + }, + { + in: "dir5/././subdir5/", + out: []string{"dir5", "dir5/subdir5"}, + }, + { + in: "./dir6", + out: []string{"dir6"}, + }, + { + in: "./dir7/subdir7", + out: []string{"dir7", "dir7/subdir7"}, + }, + { + in: "./dir8/subdir8/", + out: []string{"dir8", "dir8/subdir8"}, + }, + { + in: "./dir9/subdir9/.", + out: []string{"dir9", "dir9/subdir9"}, + }, + { + in: "./dir10/././subdir10/", + out: []string{"dir10", "dir10/subdir10"}, + }, + } + + // Set the current working directory to the temp-dir, as we're + // testing relative paths. + tmpDir := t.TempDir() + setWorkingDirectory(t, tmpDir) + + const expectedUIDGID = 101 + + for _, tc := range tests { + tc := tc + t.Run(tc.in, func(t *testing.T) { + for _, p := range tc.out { + _, err := os.Stat(p) + assert.ErrorIs(t, err, os.ErrNotExist) + } + + err := MkdirAllAndChownNew(tc.in, 0755, Identity{UID: expectedUIDGID, GID: expectedUIDGID}) + assert.Check(t, err) + + for _, p := range tc.out { + s := &unix.Stat_t{} + err = unix.Stat(p, s) + if assert.Check(t, err) { + assert.Check(t, is.Equal(uint64(s.Uid), uint64(expectedUIDGID))) + assert.Check(t, is.Equal(uint64(s.Gid), uint64(expectedUIDGID))) + } + } + }) + } +} + +// Change the current working directory for the duration of the test. This may +// break if tests are run in parallel. +func setWorkingDirectory(t *testing.T, dir string) { + t.Helper() + cwd, err := os.Getwd() + assert.NilError(t, err) + t.Cleanup(func() { + assert.NilError(t, os.Chdir(cwd)) + }) + err = os.Chdir(dir) + assert.NilError(t, err) +} + func TestMkdirAndChown(t *testing.T) { RequiresRoot(t) dirName, err := os.MkdirTemp("", "mkdir")