From 0955c88c2e64673e29f10e2aaf99b27278e757c0 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 16 Oct 2022 12:59:14 +0200 Subject: [PATCH 1/4] pkg/chrootarchive: use t.TempDir() Signed-off-by: Sebastiaan van Stijn --- pkg/chrootarchive/archive_test.go | 61 +++++--------------------- pkg/chrootarchive/archive_unix_test.go | 6 +-- 2 files changed, 12 insertions(+), 55 deletions(-) diff --git a/pkg/chrootarchive/archive_test.go b/pkg/chrootarchive/archive_test.go index d00769c54a..07bf73a9fd 100644 --- a/pkg/chrootarchive/archive_test.go +++ b/pkg/chrootarchive/archive_test.go @@ -43,11 +43,7 @@ func CopyWithTar(src, dst string) error { func TestChrootTarUntar(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") - tmpdir, err := os.MkdirTemp("", "docker-TestChrootTarUntar") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmpdir) + tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") if err := system.MkdirAll(src, 0700); err != nil { t.Fatal(err) @@ -75,11 +71,7 @@ func TestChrootTarUntar(t *testing.T) { // local images) func TestChrootUntarWithHugeExcludesList(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") - tmpdir, err := os.MkdirTemp("", "docker-TestChrootUntarHugeExcludes") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmpdir) + tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") if err := system.MkdirAll(src, 0700); err != nil { t.Fatal(err) @@ -110,12 +102,7 @@ func TestChrootUntarWithHugeExcludesList(t *testing.T) { } func TestChrootUntarEmptyArchive(t *testing.T) { - tmpdir, err := os.MkdirTemp("", "docker-TestChrootUntarEmptyArchive") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmpdir) - if err := Untar(nil, tmpdir, nil); err == nil { + if err := Untar(nil, t.TempDir(), nil); err == nil { t.Fatal("expected error on empty archive") } } @@ -176,11 +163,7 @@ func compareFiles(src string, dest string) error { func TestChrootTarUntarWithSymlink(t *testing.T) { skip.If(t, runtime.GOOS == "windows", "FIXME: figure out why this is failing") skip.If(t, os.Getuid() != 0, "skipping test that requires root") - tmpdir, err := os.MkdirTemp("", "docker-TestChrootTarUntarWithSymlink") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmpdir) + tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") if err := system.MkdirAll(src, 0700); err != nil { t.Fatal(err) @@ -200,11 +183,7 @@ func TestChrootTarUntarWithSymlink(t *testing.T) { func TestChrootCopyWithTar(t *testing.T) { skip.If(t, runtime.GOOS == "windows", "FIXME: figure out why this is failing") skip.If(t, os.Getuid() != 0, "skipping test that requires root") - tmpdir, err := os.MkdirTemp("", "docker-TestChrootCopyWithTar") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmpdir) + tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") if err := system.MkdirAll(src, 0700); err != nil { t.Fatal(err) @@ -247,11 +226,7 @@ func TestChrootCopyWithTar(t *testing.T) { func TestChrootCopyFileWithTar(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") - tmpdir, err := os.MkdirTemp("", "docker-TestChrootCopyFileWithTar") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmpdir) + tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") if err := system.MkdirAll(src, 0700); err != nil { t.Fatal(err) @@ -292,11 +267,7 @@ func TestChrootCopyFileWithTar(t *testing.T) { func TestChrootUntarPath(t *testing.T) { skip.If(t, runtime.GOOS == "windows", "FIXME: figure out why this is failing") skip.If(t, os.Getuid() != 0, "skipping test that requires root") - tmpdir, err := os.MkdirTemp("", "docker-TestChrootUntarPath") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmpdir) + tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") if err := system.MkdirAll(src, 0700); err != nil { t.Fatal(err) @@ -354,11 +325,7 @@ func (s *slowEmptyTarReader) Read(p []byte) (int, error) { func TestChrootUntarEmptyArchiveFromSlowReader(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") - tmpdir, err := os.MkdirTemp("", "docker-TestChrootUntarEmptyArchiveFromSlowReader") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmpdir) + tmpdir := t.TempDir() dest := filepath.Join(tmpdir, "dest") if err := system.MkdirAll(dest, 0700); err != nil { t.Fatal(err) @@ -371,11 +338,7 @@ func TestChrootUntarEmptyArchiveFromSlowReader(t *testing.T) { func TestChrootApplyEmptyArchiveFromSlowReader(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") - tmpdir, err := os.MkdirTemp("", "docker-TestChrootApplyEmptyArchiveFromSlowReader") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmpdir) + tmpdir := t.TempDir() dest := filepath.Join(tmpdir, "dest") if err := system.MkdirAll(dest, 0700); err != nil { t.Fatal(err) @@ -388,11 +351,7 @@ func TestChrootApplyEmptyArchiveFromSlowReader(t *testing.T) { func TestChrootApplyDotDotFile(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") - tmpdir, err := os.MkdirTemp("", "docker-TestChrootApplyDotDotFile") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmpdir) + tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") if err := system.MkdirAll(src, 0700); err != nil { t.Fatal(err) diff --git a/pkg/chrootarchive/archive_unix_test.go b/pkg/chrootarchive/archive_unix_test.go index cd557bc5cf..8349b4f02c 100644 --- a/pkg/chrootarchive/archive_unix_test.go +++ b/pkg/chrootarchive/archive_unix_test.go @@ -25,13 +25,11 @@ import ( // container path that will actually overwrite data on the host func TestUntarWithMaliciousSymlinks(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") - dir, err := os.MkdirTemp("", t.Name()) - assert.NilError(t, err) - defer os.RemoveAll(dir) + dir := t.TempDir() root := filepath.Join(dir, "root") - err = os.MkdirAll(root, 0755) + err := os.MkdirAll(root, 0o755) assert.NilError(t, err) // Add a file into a directory above root From 8a8202fcdce012a892aaa796b2235c479aeac84d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 16 Oct 2022 13:15:23 +0200 Subject: [PATCH 2/4] pkg/chrootarchive: TestChrootTarUntar fix copy/paste mistake Introduced in 3ac6394b8082d4700483d52fbfe54914be537d9e, which makes no mention of a reason for extracting to the same directory as we created the archive from, so I assume this was a copy/paste mistake and the path was meant to be "dest", not "src". Signed-off-by: Sebastiaan van Stijn --- pkg/chrootarchive/archive_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/chrootarchive/archive_test.go b/pkg/chrootarchive/archive_test.go index 07bf73a9fd..5666be2970 100644 --- a/pkg/chrootarchive/archive_test.go +++ b/pkg/chrootarchive/archive_test.go @@ -58,7 +58,7 @@ func TestChrootTarUntar(t *testing.T) { if err != nil { t.Fatal(err) } - dest := filepath.Join(tmpdir, "src") + dest := filepath.Join(tmpdir, "dest") if err := system.MkdirAll(dest, 0700); err != nil { t.Fatal(err) } From dee3f716b3ebdfffdb91cd934609b75fecb329c0 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 16 Oct 2022 13:53:58 +0200 Subject: [PATCH 3/4] pkg/chrootarchive: replace system.MkdirAll for os.Mkdir system.MkdirAll is a special version of os.Mkdir to handle creating directories using Windows volume paths ("\\?\Volume{4c1b02c1-d990-11dc-99ae-806e6f6e6963}"). This may be important when MkdirAll is used, which traverses all parent paths to create them if missing (ultimately landing on the "volume" path). Commit 62f648b06137a7e21b800f63ac403f7bb4d4f5b4 introduced the system.MkdirAll calls, as a change was made in applyLayer() for Windows to use Windows volume paths as an alternative for chroot (which is not supported on Windows). Later iteractions changed this to regular Windows long-paths (`\\?\`) in 230cfc6ed21e9398b9b3df765e6c02e90031d728, and 9b648dfac6453de5944ee4bb749115d85a253a05. Such paths are handled by the `os` package. However, in these tests, the parent path already exists (all paths created are a direct subdirectory within `tmpDir`). It looks like `MkdirAll` here is used out of convenience to not have to handle `os.ErrExist` errors. As all these tests are running in a fresh temporary directory, there should be no need to handle those, and it's actually desirable to produce an error in that case, as the directory already existing would be unexpected. Because of the above, this test changes `system.MkdirAll` to `os.Mkdir`. As we are changing these lines, this patch also changes the legacy octal notation (`0700`) to the now preferred `0o700`. Signed-off-by: Sebastiaan van Stijn --- pkg/chrootarchive/archive_test.go | 25 ++++++++++++------------- pkg/chrootarchive/archive_unix_test.go | 8 ++++---- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/pkg/chrootarchive/archive_test.go b/pkg/chrootarchive/archive_test.go index 5666be2970..755e7daad9 100644 --- a/pkg/chrootarchive/archive_test.go +++ b/pkg/chrootarchive/archive_test.go @@ -15,7 +15,6 @@ import ( "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/reexec" - "github.com/docker/docker/pkg/system" "gotest.tools/v3/skip" ) @@ -45,7 +44,7 @@ func TestChrootTarUntar(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") - if err := system.MkdirAll(src, 0700); err != nil { + if err := os.Mkdir(src, 0o700); err != nil { t.Fatal(err) } if err := os.WriteFile(filepath.Join(src, "toto"), []byte("hello toto"), 0644); err != nil { @@ -59,7 +58,7 @@ func TestChrootTarUntar(t *testing.T) { t.Fatal(err) } dest := filepath.Join(tmpdir, "dest") - if err := system.MkdirAll(dest, 0700); err != nil { + if err := os.Mkdir(dest, 0o700); err != nil { t.Fatal(err) } if err := Untar(stream, dest, &archive.TarOptions{ExcludePatterns: []string{"lolo"}}); err != nil { @@ -73,7 +72,7 @@ func TestChrootUntarWithHugeExcludesList(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") - if err := system.MkdirAll(src, 0700); err != nil { + if err := os.Mkdir(src, 0o700); err != nil { t.Fatal(err) } if err := os.WriteFile(filepath.Join(src, "toto"), []byte("hello toto"), 0644); err != nil { @@ -84,7 +83,7 @@ func TestChrootUntarWithHugeExcludesList(t *testing.T) { t.Fatal(err) } dest := filepath.Join(tmpdir, "dest") - if err := system.MkdirAll(dest, 0700); err != nil { + if err := os.Mkdir(dest, 0o700); err != nil { t.Fatal(err) } options := &archive.TarOptions{} @@ -165,7 +164,7 @@ func TestChrootTarUntarWithSymlink(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") - if err := system.MkdirAll(src, 0700); err != nil { + if err := os.Mkdir(src, 0o700); err != nil { t.Fatal(err) } if _, err := prepareSourceDirectory(10, src, false); err != nil { @@ -185,7 +184,7 @@ func TestChrootCopyWithTar(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") - if err := system.MkdirAll(src, 0700); err != nil { + if err := os.Mkdir(src, 0o700); err != nil { t.Fatal(err) } if _, err := prepareSourceDirectory(10, src, true); err != nil { @@ -228,7 +227,7 @@ func TestChrootCopyFileWithTar(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") - if err := system.MkdirAll(src, 0700); err != nil { + if err := os.Mkdir(src, 0o700); err != nil { t.Fatal(err) } if _, err := prepareSourceDirectory(10, src, true); err != nil { @@ -269,7 +268,7 @@ func TestChrootUntarPath(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") - if err := system.MkdirAll(src, 0700); err != nil { + if err := os.Mkdir(src, 0o700); err != nil { t.Fatal(err) } if _, err := prepareSourceDirectory(10, src, false); err != nil { @@ -327,7 +326,7 @@ func TestChrootUntarEmptyArchiveFromSlowReader(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") tmpdir := t.TempDir() dest := filepath.Join(tmpdir, "dest") - if err := system.MkdirAll(dest, 0700); err != nil { + if err := os.Mkdir(dest, 0o700); err != nil { t.Fatal(err) } stream := &slowEmptyTarReader{size: 10240, chunkSize: 1024} @@ -340,7 +339,7 @@ func TestChrootApplyEmptyArchiveFromSlowReader(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") tmpdir := t.TempDir() dest := filepath.Join(tmpdir, "dest") - if err := system.MkdirAll(dest, 0700); err != nil { + if err := os.Mkdir(dest, 0o700); err != nil { t.Fatal(err) } stream := &slowEmptyTarReader{size: 10240, chunkSize: 1024} @@ -353,7 +352,7 @@ func TestChrootApplyDotDotFile(t *testing.T) { skip.If(t, os.Getuid() != 0, "skipping test that requires root") tmpdir := t.TempDir() src := filepath.Join(tmpdir, "src") - if err := system.MkdirAll(src, 0700); err != nil { + if err := os.Mkdir(src, 0o700); err != nil { t.Fatal(err) } if err := os.WriteFile(filepath.Join(src, "..gitme"), []byte(""), 0644); err != nil { @@ -364,7 +363,7 @@ func TestChrootApplyDotDotFile(t *testing.T) { t.Fatal(err) } dest := filepath.Join(tmpdir, "dest") - if err := system.MkdirAll(dest, 0700); err != nil { + if err := os.Mkdir(dest, 0o700); err != nil { t.Fatal(err) } if _, err := ApplyLayer(dest, stream); err != nil { diff --git a/pkg/chrootarchive/archive_unix_test.go b/pkg/chrootarchive/archive_unix_test.go index 8349b4f02c..3cac35384c 100644 --- a/pkg/chrootarchive/archive_unix_test.go +++ b/pkg/chrootarchive/archive_unix_test.go @@ -29,7 +29,7 @@ func TestUntarWithMaliciousSymlinks(t *testing.T) { root := filepath.Join(dir, "root") - err := os.MkdirAll(root, 0o755) + err := os.Mkdir(root, 0o755) assert.NilError(t, err) // Add a file into a directory above root @@ -42,7 +42,7 @@ func TestUntarWithMaliciousSymlinks(t *testing.T) { // Before this change, the copy would overwrite the "host" content. // With this change it should not. data := filepath.Join(dir, "data") - err = os.MkdirAll(data, 0755) + err = os.Mkdir(data, 0o755) assert.NilError(t, err) err = os.WriteFile(filepath.Join(data, "local-file"), []byte("pwn3d"), 0644) assert.NilError(t, err) @@ -92,7 +92,7 @@ func TestTarWithMaliciousSymlinks(t *testing.T) { root := filepath.Join(dir, "root") - err = os.MkdirAll(root, 0755) + err = os.Mkdir(root, 0o755) assert.NilError(t, err) hostFileData := []byte("I am a host file") @@ -107,7 +107,7 @@ func TestTarWithMaliciousSymlinks(t *testing.T) { assert.NilError(t, err) data := filepath.Join(dir, "data") - err = os.MkdirAll(data, 0755) + err = os.Mkdir(data, 0o755) assert.NilError(t, err) type testCase struct { From d4d242ba76bd69ecc74329c348a6f0524c78f80b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 16 Oct 2022 14:09:06 +0200 Subject: [PATCH 4/4] pkg/chrootarchive: gofumpt test files Excluding non-test files, as a large refactor of those files is being worked on. Signed-off-by: Sebastiaan van Stijn --- pkg/chrootarchive/archive_test.go | 12 ++++++------ pkg/chrootarchive/archive_unix_test.go | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/chrootarchive/archive_test.go b/pkg/chrootarchive/archive_test.go index 755e7daad9..4e22ac6fd9 100644 --- a/pkg/chrootarchive/archive_test.go +++ b/pkg/chrootarchive/archive_test.go @@ -47,10 +47,10 @@ func TestChrootTarUntar(t *testing.T) { if err := os.Mkdir(src, 0o700); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(src, "toto"), []byte("hello toto"), 0644); err != nil { + if err := os.WriteFile(filepath.Join(src, "toto"), []byte("hello toto"), 0o644); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(src, "lolo"), []byte("hello lolo"), 0644); err != nil { + if err := os.WriteFile(filepath.Join(src, "lolo"), []byte("hello lolo"), 0o644); err != nil { t.Fatal(err) } stream, err := archive.Tar(src, archive.Uncompressed) @@ -75,7 +75,7 @@ func TestChrootUntarWithHugeExcludesList(t *testing.T) { if err := os.Mkdir(src, 0o700); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(src, "toto"), []byte("hello toto"), 0644); err != nil { + if err := os.WriteFile(filepath.Join(src, "toto"), []byte("hello toto"), 0o644); err != nil { t.Fatal(err) } stream, err := archive.Tar(src, archive.Uncompressed) @@ -110,7 +110,7 @@ func prepareSourceDirectory(numberOfFiles int, targetPath string, makeSymLinks b fileData := []byte("fooo") for n := 0; n < numberOfFiles; n++ { fileName := fmt.Sprintf("file-%d", n) - if err := os.WriteFile(filepath.Join(targetPath, fileName), fileData, 0700); err != nil { + if err := os.WriteFile(filepath.Join(targetPath, fileName), fileData, 0o700); err != nil { return 0, err } if makeSymLinks { @@ -288,7 +288,7 @@ func TestChrootUntarPath(t *testing.T) { buf := new(bytes.Buffer) buf.ReadFrom(stream) tarfile := filepath.Join(tmpdir, "src.tar") - if err := os.WriteFile(tarfile, buf.Bytes(), 0644); err != nil { + if err := os.WriteFile(tarfile, buf.Bytes(), 0o644); err != nil { t.Fatal(err) } if err := UntarPath(tarfile, dest); err != nil { @@ -355,7 +355,7 @@ func TestChrootApplyDotDotFile(t *testing.T) { if err := os.Mkdir(src, 0o700); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(src, "..gitme"), []byte(""), 0644); err != nil { + if err := os.WriteFile(filepath.Join(src, "..gitme"), []byte(""), 0o644); err != nil { t.Fatal(err) } stream, err := archive.Tar(src, archive.Uncompressed) diff --git a/pkg/chrootarchive/archive_unix_test.go b/pkg/chrootarchive/archive_unix_test.go index 3cac35384c..6a3ca974fc 100644 --- a/pkg/chrootarchive/archive_unix_test.go +++ b/pkg/chrootarchive/archive_unix_test.go @@ -34,7 +34,7 @@ func TestUntarWithMaliciousSymlinks(t *testing.T) { // Add a file into a directory above root // Ensure that we can't access this file while tarring. - err = os.WriteFile(filepath.Join(dir, "host-file"), []byte("I am a host file"), 0644) + err = os.WriteFile(filepath.Join(dir, "host-file"), []byte("I am a host file"), 0o644) assert.NilError(t, err) // Create some data which which will be copied into the "container" root into @@ -44,7 +44,7 @@ func TestUntarWithMaliciousSymlinks(t *testing.T) { data := filepath.Join(dir, "data") err = os.Mkdir(data, 0o755) assert.NilError(t, err) - err = os.WriteFile(filepath.Join(data, "local-file"), []byte("pwn3d"), 0644) + err = os.WriteFile(filepath.Join(data, "local-file"), []byte("pwn3d"), 0o644) assert.NilError(t, err) safe := filepath.Join(root, "safe") @@ -99,7 +99,7 @@ func TestTarWithMaliciousSymlinks(t *testing.T) { // Add a file into a directory above root // Ensure that we can't access this file while tarring. - err = os.WriteFile(filepath.Join(dir, "host-file"), hostFileData, 0644) + err = os.WriteFile(filepath.Join(dir, "host-file"), hostFileData, 0o644) assert.NilError(t, err) safe := filepath.Join(root, "safe")