From cf1e138ab19bd0ace57d9ba418de514dd7a57b70 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 5 Oct 2022 10:47:28 +0200 Subject: [PATCH 1/6] pkg/directory: Size(): add back type-casts to account for platform differences I noticed the comment above this code, but didn't see a corresponding type-cast. Looking at this file's history, I found that these were removed as part of 2f5f0af3fdb7e9ee607a0e178dbe2af6e10cccf4, which looks to have overlooked some deliberate type-casts. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 0a861e68df30a5d1e72bac99fe259149507cf354) Signed-off-by: Sebastiaan van Stijn --- pkg/directory/directory_unix.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/directory/directory_unix.go b/pkg/directory/directory_unix.go index eeedff18a4..a8bc41093f 100644 --- a/pkg/directory/directory_unix.go +++ b/pkg/directory/directory_unix.go @@ -40,12 +40,12 @@ func Size(ctx context.Context, dir string) (size int64, err error) { // Check inode to handle hard links correctly inode := fileInfo.Sys().(*syscall.Stat_t).Ino - // inode is not a uint64 on all platforms. Cast it to avoid issues. - if _, exists := data[inode]; exists { + //nolint:unconvert // inode is not an uint64 on all platforms. + if _, exists := data[uint64(inode)]; exists { return nil } - // inode is not a uint64 on all platforms. Cast it to avoid issues. - data[inode] = struct{}{} + + data[uint64(inode)] = struct{}{} //nolint:unconvert // inode is not an uint64 on all platforms. size += s From 3a946f52913e70700e90904e809fdab6ac94c9a8 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 5 Oct 2022 13:59:22 +0200 Subject: [PATCH 2/6] pkg/system: remove Umask() utility It was only used in a couple of places, and in most places shouldn't be used as those locations were in unix/linux-only files, so didn't need the wrapper. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 4347080b46b9143dd1b046b93543b8ed30f55ea6) Signed-off-by: Sebastiaan van Stijn --- pkg/archive/archive_linux_test.go | 10 ++++------ pkg/archive/diff.go | 9 ++------- pkg/archive/diff_unix.go | 22 ++++++++++++++++++++++ pkg/archive/diff_windows.go | 6 ++++++ pkg/chrootarchive/diff_unix.go | 9 +++------ pkg/system/umask.go | 14 -------------- pkg/system/umask_windows.go | 7 ------- 7 files changed, 37 insertions(+), 40 deletions(-) create mode 100644 pkg/archive/diff_unix.go create mode 100644 pkg/archive/diff_windows.go delete mode 100644 pkg/system/umask.go delete mode 100644 pkg/system/umask_windows.go diff --git a/pkg/archive/archive_linux_test.go b/pkg/archive/archive_linux_test.go index efff0dddcf..640929298f 100644 --- a/pkg/archive/archive_linux_test.go +++ b/pkg/archive/archive_linux_test.go @@ -86,9 +86,8 @@ func checkFileMode(t *testing.T, path string, perm os.FileMode) { } func TestOverlayTarUntar(t *testing.T) { - oldmask, err := system.Umask(0) - assert.NilError(t, err) - defer system.Umask(oldmask) + restore := overrideUmask(0) + defer restore() src, err := os.MkdirTemp("", "docker-test-overlay-tar-src") assert.NilError(t, err) @@ -125,9 +124,8 @@ func TestOverlayTarUntar(t *testing.T) { } func TestOverlayTarAUFSUntar(t *testing.T) { - oldmask, err := system.Umask(0) - assert.NilError(t, err) - defer system.Umask(oldmask) + restore := overrideUmask(0) + defer restore() src, err := os.MkdirTemp("", "docker-test-overlay-tar-src") assert.NilError(t, err) diff --git a/pkg/archive/diff.go b/pkg/archive/diff.go index 62409d827e..8eeccb608b 100644 --- a/pkg/archive/diff.go +++ b/pkg/archive/diff.go @@ -229,13 +229,8 @@ func applyLayerHandler(dest string, layer io.Reader, options *TarOptions, decomp dest = filepath.Clean(dest) // We need to be able to set any perms - if runtime.GOOS != "windows" { - oldmask, err := system.Umask(0) - if err != nil { - return 0, err - } - defer system.Umask(oldmask) - } + restore := overrideUmask(0) + defer restore() if decompress { decompLayer, err := DecompressStream(layer) diff --git a/pkg/archive/diff_unix.go b/pkg/archive/diff_unix.go new file mode 100644 index 0000000000..d7f806445e --- /dev/null +++ b/pkg/archive/diff_unix.go @@ -0,0 +1,22 @@ +//go:build !windows +// +build !windows + +package archive + +import "golang.org/x/sys/unix" + +// overrideUmask sets current process's file mode creation mask to newmask +// and returns a function to restore it. +// +// WARNING for readers stumbling upon this code. Changing umask in a multi- +// threaded environment isn't safe. Don't use this without understanding the +// risks, and don't export this function for others to use (we shouldn't even +// be using this ourself). +// +// FIXME(thaJeztah): we should get rid of these hacks if possible. +func overrideUmask(newMask int) func() { + oldMask := unix.Umask(newMask) + return func() { + unix.Umask(oldMask) + } +} diff --git a/pkg/archive/diff_windows.go b/pkg/archive/diff_windows.go new file mode 100644 index 0000000000..d28f5b2dfd --- /dev/null +++ b/pkg/archive/diff_windows.go @@ -0,0 +1,6 @@ +package archive + +// overrideUmask is a no-op on windows. +func overrideUmask(newmask int) func() { + return func() {} +} diff --git a/pkg/chrootarchive/diff_unix.go b/pkg/chrootarchive/diff_unix.go index e1bf74d1d5..fcc02f675e 100644 --- a/pkg/chrootarchive/diff_unix.go +++ b/pkg/chrootarchive/diff_unix.go @@ -16,7 +16,7 @@ import ( "github.com/containerd/containerd/pkg/userns" "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/reexec" - "github.com/docker/docker/pkg/system" + "golang.org/x/sys/unix" ) type applyLayerResponse struct { @@ -42,11 +42,8 @@ func applyLayer() { } // We need to be able to set any perms - oldmask, err := system.Umask(0) - defer system.Umask(oldmask) - if err != nil { - fatal(err) - } + oldmask := unix.Umask(0) + defer unix.Umask(oldmask) if err := json.Unmarshal([]byte(os.Getenv("OPT")), &options); err != nil { fatal(err) diff --git a/pkg/system/umask.go b/pkg/system/umask.go deleted file mode 100644 index d4a15cbedc..0000000000 --- a/pkg/system/umask.go +++ /dev/null @@ -1,14 +0,0 @@ -//go:build !windows -// +build !windows - -package system // import "github.com/docker/docker/pkg/system" - -import ( - "golang.org/x/sys/unix" -) - -// Umask sets current process's file mode creation mask to newmask -// and returns oldmask. -func Umask(newmask int) (oldmask int, err error) { - return unix.Umask(newmask), nil -} diff --git a/pkg/system/umask_windows.go b/pkg/system/umask_windows.go deleted file mode 100644 index fc62388c38..0000000000 --- a/pkg/system/umask_windows.go +++ /dev/null @@ -1,7 +0,0 @@ -package system // import "github.com/docker/docker/pkg/system" - -// Umask is not supported on the windows platform. -func Umask(newmask int) (oldmask int, err error) { - // should not be called on cli code path - return 0, ErrNotSupportedPlatform -} From 9d86e1d204eaf0419e85cc68c31daa2cbdc807f2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 5 Oct 2022 16:20:33 +0200 Subject: [PATCH 3/6] pkg/system: move GetExitCode() to pkg/idtools, and un-export This utility was only used in a single place, and had no external consumers. Move it to where it's used. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 07b1aa822cc7b371ef5925940d8ea8cfb54de57e) Signed-off-by: Sebastiaan van Stijn --- pkg/idtools/idtools_unix.go | 15 ++++++++++++++- pkg/system/exitcode.go | 19 ------------------- 2 files changed, 14 insertions(+), 20 deletions(-) delete mode 100644 pkg/system/exitcode.go diff --git a/pkg/idtools/idtools_unix.go b/pkg/idtools/idtools_unix.go index aed1a41480..03846b0307 100644 --- a/pkg/idtools/idtools_unix.go +++ b/pkg/idtools/idtools_unix.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "os" + "os/exec" "path/filepath" "strconv" "sync" @@ -199,7 +200,7 @@ func callGetent(database, key string) (io.Reader, error) { } out, err := execCmd(getentCmd, database, key) if err != nil { - exitCode, errC := system.GetExitCode(err) + exitCode, errC := getExitCode(err) if errC != nil { return nil, err } @@ -217,6 +218,18 @@ func callGetent(database, key string) (io.Reader, error) { return bytes.NewReader(out), nil } +// getExitCode returns the ExitStatus of the specified error if its type is +// exec.ExitError, returns 0 and an error otherwise. +func getExitCode(err error) (int, error) { + exitCode := 0 + if exiterr, ok := err.(*exec.ExitError); ok { + if procExit, ok := exiterr.Sys().(syscall.WaitStatus); ok { + return procExit.ExitStatus(), nil + } + } + return exitCode, fmt.Errorf("failed to get exit code") +} + // setPermissions performs a chown/chmod only if the uid/gid don't match what's requested // Normally a Chown is a no-op if uid/gid match, but in some cases this can still cause an error, e.g. if the // dir is on an NFS share, so don't call chown unless we absolutely must. diff --git a/pkg/system/exitcode.go b/pkg/system/exitcode.go deleted file mode 100644 index 4ba8fe35bf..0000000000 --- a/pkg/system/exitcode.go +++ /dev/null @@ -1,19 +0,0 @@ -package system // import "github.com/docker/docker/pkg/system" - -import ( - "fmt" - "os/exec" - "syscall" -) - -// GetExitCode returns the ExitStatus of the specified error if its type is -// exec.ExitError, returns 0 and an error otherwise. -func GetExitCode(err error) (int, error) { - exitCode := 0 - if exiterr, ok := err.(*exec.ExitError); ok { - if procExit, ok := exiterr.Sys().(syscall.WaitStatus); ok { - return procExit.ExitStatus(), nil - } - } - return exitCode, fmt.Errorf("failed to get exit code") -} From e0b105623e2c02edbc176d902eedf39f78f05fda Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 5 Oct 2022 16:21:04 +0200 Subject: [PATCH 4/6] pkg/system: unconvert Signed-off-by: Sebastiaan van Stijn (cherry picked from commit ab677c41ea215cbb2f81c15a6d8a7ba505aff05a) Signed-off-by: Sebastiaan van Stijn --- pkg/system/stat_windows.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/system/stat_windows.go b/pkg/system/stat_windows.go index b2456cb887..0ff3af2fa1 100644 --- a/pkg/system/stat_windows.go +++ b/pkg/system/stat_windows.go @@ -20,12 +20,12 @@ func (s StatT) Size() int64 { // Mode returns file's permission mode. func (s StatT) Mode() os.FileMode { - return os.FileMode(s.mode) + return s.mode } // Mtim returns file's last modification time. func (s StatT) Mtim() time.Time { - return time.Time(s.mtim) + return s.mtim } // Stat takes a path to a file and returns From 92b96ac2ed97c8324b4b4629098cb4ae4bcde69f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 5 Oct 2022 11:09:35 +0200 Subject: [PATCH 5/6] pkg/directory: minor refactor of Size() - separate exported function from implementation, to allow for GoDoc to be maintained in a single location. - don't use named return variables (no "bare" return, and potentially shadowing variables) - reverse the `os.IsNotExist(err) && d != dir` condition, putting the "lighter" `d != dir` first. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit bd6217bb74644d245ce6271138f4c660415fa0fb) Signed-off-by: Sebastiaan van Stijn --- pkg/directory/directory.go | 6 ++++++ pkg/directory/directory_unix.go | 13 +++++++------ pkg/directory/directory_windows.go | 13 +++++++------ 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/pkg/directory/directory.go b/pkg/directory/directory.go index f0c45303cd..998b93fd8c 100644 --- a/pkg/directory/directory.go +++ b/pkg/directory/directory.go @@ -1,6 +1,7 @@ package directory // import "github.com/docker/docker/pkg/directory" import ( + "context" "os" "path/filepath" ) @@ -22,3 +23,8 @@ func MoveToSubdir(oldpath, subdir string) error { } return nil } + +// Size walks a directory tree and returns its total size in bytes. +func Size(ctx context.Context, dir string) (int64, error) { + return calcSize(ctx, dir) +} diff --git a/pkg/directory/directory_unix.go b/pkg/directory/directory_unix.go index a8bc41093f..4592205f63 100644 --- a/pkg/directory/directory_unix.go +++ b/pkg/directory/directory_unix.go @@ -10,14 +10,15 @@ import ( "syscall" ) -// Size walks a directory tree and returns its total size in bytes. -func Size(ctx context.Context, dir string) (size int64, err error) { +// calcSize walks a directory tree and returns its total size in bytes. +func calcSize(ctx context.Context, dir string) (int64, error) { + var size int64 data := make(map[uint64]struct{}) - err = filepath.Walk(dir, func(d string, fileInfo os.FileInfo, err error) error { + err := filepath.Walk(dir, func(d string, fileInfo os.FileInfo, err error) error { if err != nil { - // if dir does not exist, Size() returns the error. // if dir/x disappeared while walking, Size() ignores dir/x. - if os.IsNotExist(err) && d != dir { + // if dir does not exist, Size() returns the error. + if d != dir && os.IsNotExist(err) { return nil } return err @@ -51,5 +52,5 @@ func Size(ctx context.Context, dir string) (size int64, err error) { return nil }) - return + return size, err } diff --git a/pkg/directory/directory_windows.go b/pkg/directory/directory_windows.go index f07f241880..fc72ff62e6 100644 --- a/pkg/directory/directory_windows.go +++ b/pkg/directory/directory_windows.go @@ -6,13 +6,14 @@ import ( "path/filepath" ) -// Size walks a directory tree and returns its total size in bytes. -func Size(ctx context.Context, dir string) (size int64, err error) { - err = filepath.Walk(dir, func(d string, fileInfo os.FileInfo, err error) error { +// calcSize walks a directory tree and returns its total calcSize in bytes. +func calcSize(ctx context.Context, dir string) (int64, error) { + var size int64 + err := filepath.Walk(dir, func(d string, fileInfo os.FileInfo, err error) error { if err != nil { - // if dir does not exist, Size() returns the error. // if dir/x disappeared while walking, Size() ignores dir/x. - if os.IsNotExist(err) && d != dir { + // if dir does not exist, Size() returns the error. + if d != dir && os.IsNotExist(err) { return nil } return err @@ -38,5 +39,5 @@ func Size(ctx context.Context, dir string) (size int64, err error) { return nil }) - return + return size, err } From 5e15ce3a4a0f019fca92a52cf8108b29bd809e2e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 5 Oct 2022 11:11:26 +0200 Subject: [PATCH 6/6] pkg/directory: remove unused MoveToSubdir() utility This utility was added in 442b45628ee12ebd8e8bd08497896d5fa8eec4bd as part of user-namespaces, and first used in 44e1023a93a0107d63d5400695cbbc6da498a425 to set up the daemon root, and move the existing content; https://github.com/docker/docker/blob/44e1023a93a0107d63d5400695cbbc6da498a425/daemon/daemon_experimental.go#L68-L71 A later iteration no longer _moved_ the existing root directory, and removed the use of `directory.MoveToSubdir()` e8532023f20498e6eb1ce5c079dc8a09aeae3061 It looks like there's no external consumers of this utility, so we should be save to remove it. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 26659d5eb83330269ef634713435a995caa1e2e6) Signed-off-by: Sebastiaan van Stijn --- pkg/directory/directory.go | 24 +---------------- pkg/directory/directory_test.go | 48 --------------------------------- 2 files changed, 1 insertion(+), 71 deletions(-) diff --git a/pkg/directory/directory.go b/pkg/directory/directory.go index 998b93fd8c..7b8d74a356 100644 --- a/pkg/directory/directory.go +++ b/pkg/directory/directory.go @@ -1,28 +1,6 @@ package directory // import "github.com/docker/docker/pkg/directory" -import ( - "context" - "os" - "path/filepath" -) - -// MoveToSubdir moves all contents of a directory to a subdirectory underneath the original path -func MoveToSubdir(oldpath, subdir string) error { - infos, err := os.ReadDir(oldpath) - if err != nil { - return err - } - for _, info := range infos { - if info.Name() != subdir { - oldName := filepath.Join(oldpath, info.Name()) - newName := filepath.Join(oldpath, subdir, info.Name()) - if err := os.Rename(oldName, newName); err != nil { - return err - } - } - } - return nil -} +import "context" // Size walks a directory tree and returns its total size in bytes. func Size(ctx context.Context, dir string) (int64, error) { diff --git a/pkg/directory/directory_test.go b/pkg/directory/directory_test.go index ec9c97e699..3bfa1e0fd7 100644 --- a/pkg/directory/directory_test.go +++ b/pkg/directory/directory_test.go @@ -3,9 +3,6 @@ package directory // import "github.com/docker/docker/pkg/directory" import ( "context" "os" - "path/filepath" - "reflect" - "sort" "testing" ) @@ -144,51 +141,6 @@ func TestSizeFileAndNestedDirectoryNonempty(t *testing.T) { } } -// Test migration of directory to a subdir underneath itself -func TestMoveToSubdir(t *testing.T) { - var outerDir, subDir string - var err error - - if outerDir, err = os.MkdirTemp(os.TempDir(), "TestMoveToSubdir"); err != nil { - t.Fatalf("failed to create directory: %v", err) - } - - if subDir, err = os.MkdirTemp(outerDir, "testSub"); err != nil { - t.Fatalf("failed to create subdirectory: %v", err) - } - - // write 4 temp files in the outer dir to get moved - filesList := []string{"a", "b", "c", "d"} - for _, fName := range filesList { - if file, err := os.Create(filepath.Join(outerDir, fName)); err != nil { - t.Fatalf("couldn't create temp file %q: %v", fName, err) - } else { - file.WriteString(fName) - file.Close() - } - } - - if err = MoveToSubdir(outerDir, filepath.Base(subDir)); err != nil { - t.Fatalf("Error during migration of content to subdirectory: %v", err) - } - // validate that the files were moved to the subdirectory - infos, err := os.ReadDir(subDir) - if err != nil { - t.Fatal(err) - } - if len(infos) != 4 { - t.Fatalf("Should be four files in the subdir after the migration: actual length: %d", len(infos)) - } - var results []string - for _, info := range infos { - results = append(results, info.Name()) - } - sort.Strings(results) - if !reflect.DeepEqual(filesList, results) { - t.Fatalf("Results after migration do not equal list of files: expected: %v, got: %v", filesList, results) - } -} - // Test a non-existing directory func TestSizeNonExistingDirectory(t *testing.T) { if _, err := Size(context.Background(), "/thisdirectoryshouldnotexist/TestSizeNonExistingDirectory"); err == nil {