From 4472e9b7f876bc0ec03289aece60590a4f46e7be Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 21 Dec 2024 14:15:21 +0100 Subject: [PATCH 1/8] pkg/system: deprecate MkdirAll and remove custom volume GUID handling commit 86d1223a29907ffc6afba557b5138cfad7816bb4 introduced a custom version of `os.MkdirAll` for Windows to account for situations where the path to create would start with a Windows volume name (GUID path), for example, `"\\?\Volume{4c1b02c1-d990-11dc-99ae-806e6f6e6963}\`. At the time that patch was added we were using [go1.4.2], which did not have special handling for Windows in [MkdirAll], therefore would recognize such paths as regular paths, trying to create them, which would fail. This code was later updated in 46ec4c1ae2700ed638072fd7fb326afc10eded20 to provide ACL (DACL) support on Windows. Further updates were made in cfef1b11e571ad2469cfa9631180db398f106468 and 55ceb5047c304d3ac8ecd15801a4b0472832158e to allow for an early return when detecting a volume GUID path, and the code was re-aligned with the latest (go1.19.2) implementation in f058afc861c2f56bf9e97472e99df65c6493e694, which brought in the platform-specific [fixRootDirectory] handling introduced in go1.11. While that enhancement detected UNC volume-paths (`\\?c\`, `//?/c:`), it did not yet support volume GUID paths. go1.22, through [golang.org/cl/86295] added support for this, and `os.MkdirAll` now natively detects volume GUID paths, making our own implementation for this redundant. This patch: - Deprecates pkg/system.MkdirAll in favor of os.MkdirAll, which now provides the same functionality on go1.22 and up. - Renames the (non-exported) `mkdirall` function to `mkdirAllWithACL`, and synchronises `it` with the [implementation in go1.23.4], bringing in the changes from [golang.org/cl/86295] and [golang.org/cl/582499]. - Adds a fast path to `MkdirAllWithACL` if no ACL / SDDL is provided. It's worth noting that we currently still support go1.22, and that the implementation changed in go1.23; those changes ([golang.org/cl/581517] and [golang.org/cl/566556]) were lateral moves, therefore should be identical to the implementation in go1.22, and we can safely use the implementation provided by [filepath.VolumeName] on either go1.22 or go1.23. [go1.4.2]: https://github.com/moby/moby/blob/86d1223a29907ffc6afba557b5138cfad7816bb4/Dockerfile#L77 [MkdirAll]: https://github.com/golang/go/blob/go1.4.2/src/os/path.go#L19-L60 [fixRootDirectory]: https://github.com/golang/go/commit/b86e76681366447798c94abb959bb60875bcc856 [golang.org/cl/86295]: https://github.com/golang/go/commit/cd589c8a73415afbf94a8976f20cbed9d4061ba6 [golang.org/cl/582499]: https://github.com/golang/go/commit/5616ab602566b0daa87dfd250a76c61960c4b634 [golang.org/cl/581517]: https://github.com/golang/go/commit/ad22356ec660844ec43ccbe9a834845f1a6f7cf8 [golang.org/cl/566556]: https://github.com/golang/go/commit/ceef0633b3c5bbf5d17a12d6e663c136b30b3f36 [1]: https://github.com/golang/go/blob/go1.23.4/src/os/path.go#L12-L66 [filepath.VolumeName]: https://pkg.go.dev/path/filepath#VolumeName Signed-off-by: Sebastiaan van Stijn --- pkg/system/filesys.go | 8 ++++ pkg/system/filesys_unix.go | 8 +--- pkg/system/filesys_windows.go | 73 ++++++++++++++--------------------- 3 files changed, 39 insertions(+), 50 deletions(-) diff --git a/pkg/system/filesys.go b/pkg/system/filesys.go index ce5990c914..41c2a366a6 100644 --- a/pkg/system/filesys.go +++ b/pkg/system/filesys.go @@ -17,3 +17,11 @@ import ( func IsAbs(path string) bool { return filepath.IsAbs(path) || strings.HasPrefix(path, string(os.PathSeparator)) } + +// MkdirAll creates a directory named path along with any necessary parents, +// with permission specified by attribute perm for all dir created. +// +// Deprecated: [os.MkdirAll] now natively supports Windows GUID volume paths, and should be used instead. This alias will be removed in the next release. +func MkdirAll(path string, perm os.FileMode) error { + return os.MkdirAll(path, perm) +} diff --git a/pkg/system/filesys_unix.go b/pkg/system/filesys_unix.go index f01f9385e1..a0dd260638 100644 --- a/pkg/system/filesys_unix.go +++ b/pkg/system/filesys_unix.go @@ -5,12 +5,6 @@ package system // import "github.com/docker/docker/pkg/system" import "os" // MkdirAllWithACL is a wrapper for os.MkdirAll on unix systems. -func MkdirAllWithACL(path string, perm os.FileMode, sddl string) error { - return os.MkdirAll(path, perm) -} - -// MkdirAll creates a directory named path along with any necessary parents, -// with permission specified by attribute perm for all dir created. -func MkdirAll(path string, perm os.FileMode) error { +func MkdirAllWithACL(path string, perm os.FileMode, _ string) error { return os.MkdirAll(path, perm) } diff --git a/pkg/system/filesys_windows.go b/pkg/system/filesys_windows.go index 92e972ea2e..a293ea08c9 100644 --- a/pkg/system/filesys_windows.go +++ b/pkg/system/filesys_windows.go @@ -2,7 +2,7 @@ package system // import "github.com/docker/docker/pkg/system" import ( "os" - "regexp" + "path/filepath" "syscall" "unsafe" @@ -12,11 +12,6 @@ import ( // SddlAdministratorsLocalSystem is local administrators plus NT AUTHORITY\System. const SddlAdministratorsLocalSystem = "D:P(A;OICI;GA;;;BA)(A;OICI;GA;;;SY)" -// volumePath is a regular expression to check if a path is a Windows -// volume path (e.g., "\\?\Volume{4c1b02c1-d990-11dc-99ae-806e6f6e6963}" -// or "\\?\Volume{4c1b02c1-d990-11dc-99ae-806e6f6e6963}\"). -var volumePath = regexp.MustCompile(`^\\\\\?\\Volume{[a-z0-9-]+}\\?$`) - // MkdirAllWithACL is a custom version of os.MkdirAll modified for use on Windows // so that it is both volume path aware, and can create a directory with // an appropriate SDDL defined ACL. @@ -25,26 +20,23 @@ func MkdirAllWithACL(path string, _ os.FileMode, sddl string) error { if err != nil { return &os.PathError{Op: "mkdirall", Path: path, Err: err} } - return mkdirall(path, sa) + return mkdirAllWithACL(path, sa) } -// MkdirAll is a custom version of os.MkdirAll that is volume path aware for -// Windows. It can be used as a drop-in replacement for os.MkdirAll. -func MkdirAll(path string, _ os.FileMode) error { - return mkdirall(path, nil) -} - -// mkdirall is a custom version of os.MkdirAll modified for use on Windows -// so that it is both volume path aware, and can create a directory with -// a DACL. -func mkdirall(path string, perm *windows.SecurityAttributes) error { - if volumePath.MatchString(path) { - return nil +// mkdirAllWithACL is a custom version of os.MkdirAll with DACL support on Windows. +// It is fully identical to [os.MkdirAll] if no DACL is provided. +// +// Code in this function is based on the implementation in [go1.23.4]. +// +// Copyright 2009 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. +// +// [go1.23.4]: https://github.com/golang/go/blob/go1.23.4/src/os/path.go#L12-L66 +func mkdirAllWithACL(path string, perm *windows.SecurityAttributes) error { + if perm == nil { + return os.MkdirAll(path, 0) } - - // The rest of this method is largely copied from os.MkdirAll and should be kept - // as-is to ensure compatibility. - // Fast path: if we can tell whether path is a directory or file, stop with success or error. dir, err := os.Stat(path) if err == nil { @@ -55,19 +47,25 @@ func mkdirall(path string, perm *windows.SecurityAttributes) error { } // Slow path: make sure parent exists and then call Mkdir for path. - i := len(path) - for i > 0 && os.IsPathSeparator(path[i-1]) { // Skip trailing path separator. + + // Extract the parent folder from path by first removing any trailing + // path separator and then scanning backward until finding a path + // separator or reaching the beginning of the string. + i := len(path) - 1 + for i >= 0 && os.IsPathSeparator(path[i]) { i-- } - - j := i - for j > 0 && !os.IsPathSeparator(path[j-1]) { // Scan backward over element. - j-- + for i >= 0 && !os.IsPathSeparator(path[i]) { + i-- + } + if i < 0 { + i = 0 } - if j > 1 { - // Create parent. - err = mkdirall(fixRootDirectory(path[:j-1]), perm) + // If there is a parent directory, and it is not the volume name, + // recurse to ensure parent directory exists. + if parent := path[:i]; len(parent) > len(filepath.VolumeName(path)) { + err = mkdirAllWithACL(parent, perm) if err != nil { return err } @@ -111,17 +109,6 @@ func mkdirWithACL(name string, sa *windows.SecurityAttributes) error { return nil } -// fixRootDirectory fixes a reference to a drive's root directory to -// have the required trailing slash. -func fixRootDirectory(p string) string { - if len(p) == len(`\\?\c:`) { - if os.IsPathSeparator(p[0]) && os.IsPathSeparator(p[1]) && p[2] == '?' && os.IsPathSeparator(p[3]) && p[5] == ':' { - return p + `\` - } - } - return p -} - func makeSecurityAttributes(sddl string) (*windows.SecurityAttributes, error) { var sa windows.SecurityAttributes sa.Length = uint32(unsafe.Sizeof(sa)) From bc61b3193563b08e04baf12dcb5f7417dae8fe6e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 21 Dec 2024 14:25:11 +0100 Subject: [PATCH 2/8] pkg/idtools: remove uses of deprecated system.MkdirAll Signed-off-by: Sebastiaan van Stijn --- pkg/idtools/idtools_windows.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/idtools/idtools_windows.go b/pkg/idtools/idtools_windows.go index 32953f4563..5b7c2ad771 100644 --- a/pkg/idtools/idtools_windows.go +++ b/pkg/idtools/idtools_windows.go @@ -2,8 +2,6 @@ package idtools // import "github.com/docker/docker/pkg/idtools" import ( "os" - - "github.com/docker/docker/pkg/system" ) const ( @@ -15,10 +13,10 @@ const ( ContainerUserSidString = "S-1-5-93-2-2" ) -// This is currently a wrapper around MkdirAll, however, since currently +// This is currently a wrapper around [os.MkdirAll] since currently // permissions aren't set through this path, the identity isn't utilized. // Ownership is handled elsewhere, but in the future could be support here // too. func mkdirAs(path string, _ os.FileMode, _ Identity, _, _ bool) error { - return system.MkdirAll(path, 0) + return os.MkdirAll(path, 0) } From e783bb5c6951f327472813086b13fc4b384b7436 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 21 Dec 2024 14:28:34 +0100 Subject: [PATCH 3/8] builder/dockerfile: remove uses of deprecated system.MkdirAll Signed-off-by: Sebastiaan van Stijn --- builder/dockerfile/copy.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/builder/dockerfile/copy.go b/builder/dockerfile/copy.go index 06f02388f5..73dbb36a91 100644 --- a/builder/dockerfile/copy.go +++ b/builder/dockerfile/copy.go @@ -502,11 +502,7 @@ func copyDirectory(archiver *archive.Archiver, source, dest string, identity *id func copyFile(archiver *archive.Archiver, source, dest string, identity *idtools.Identity) error { if identity == nil { - // Use system.MkdirAll here, which is a custom version of os.MkdirAll - // modified for use on Windows to handle volume GUID paths. These paths - // are of the form \\?\Volume{}\. An example would be: - // \\?\Volume{dae8d3ac-b9a1-11e9-88eb-e8554b2ba1db}\bin\busybox.exe - if err := system.MkdirAll(filepath.Dir(dest), 0o755); err != nil { + if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { return err } } else { From c02c2a3a796a9a4594207bc4fa3e2707728e8d65 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 21 Dec 2024 14:30:56 +0100 Subject: [PATCH 4/8] cmd/dockerd: remove uses of deprecated system.MkdirAll Signed-off-by: Sebastiaan van Stijn --- cmd/dockerd/daemon.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cmd/dockerd/daemon.go b/cmd/dockerd/daemon.go index a0b5e4949d..e3a96864c4 100644 --- a/cmd/dockerd/daemon.go +++ b/cmd/dockerd/daemon.go @@ -54,7 +54,6 @@ import ( "github.com/docker/docker/pkg/plugingetter" "github.com/docker/docker/pkg/rootless" "github.com/docker/docker/pkg/sysinfo" - "github.com/docker/docker/pkg/system" "github.com/docker/docker/plugin" "github.com/docker/docker/runconfig" "github.com/docker/go-connections/tlsconfig" @@ -143,14 +142,14 @@ func (cli *daemonCLI) start(ctx context.Context) (err error) { return err } - if err := system.MkdirAll(cli.Config.ExecRoot, 0o700); err != nil { + if err := os.MkdirAll(cli.Config.ExecRoot, 0o700); err != nil { return err } potentiallyUnderRuntimeDir := []string{cli.Config.ExecRoot} if cli.Pidfile != "" { - if err = system.MkdirAll(filepath.Dir(cli.Pidfile), 0o755); err != nil { + if err = os.MkdirAll(filepath.Dir(cli.Pidfile), 0o755); err != nil { return errors.Wrap(err, "failed to create pidfile directory") } if err = pidfile.Write(cli.Pidfile, os.Getpid()); err != nil { From 84bb6e5afb3ec5b88ca644dbfb55a3b5e485de87 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 21 Dec 2024 14:32:25 +0100 Subject: [PATCH 5/8] container: remove uses of deprecated system.MkdirAll Signed-off-by: Sebastiaan van Stijn --- container/container_windows.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/container/container_windows.go b/container/container_windows.go index 077b74369c..fd8707a615 100644 --- a/container/container_windows.go +++ b/container/container_windows.go @@ -9,7 +9,6 @@ import ( containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/events" swarmtypes "github.com/docker/docker/api/types/swarm" - "github.com/docker/docker/pkg/system" ) const ( @@ -46,7 +45,7 @@ func (container *Container) CreateSecretSymlinks() error { if err != nil { return err } - if err := system.MkdirAll(filepath.Dir(resolvedPath), 0); err != nil { + if err := os.MkdirAll(filepath.Dir(resolvedPath), 0); err != nil { return err } if err := os.Symlink(filepath.Join(containerInternalSecretMountPath, r.SecretID), resolvedPath); err != nil { @@ -96,7 +95,7 @@ func (container *Container) CreateConfigSymlinks() error { if err != nil { return err } - if err := system.MkdirAll(filepath.Dir(resolvedPath), 0); err != nil { + if err := os.MkdirAll(filepath.Dir(resolvedPath), 0); err != nil { return err } if err := os.Symlink(filepath.Join(containerInternalConfigsDirPath, configRef.ConfigID), resolvedPath); err != nil { From 05ec7326674fefb2c47ec18fa1ffed69e56f00b5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 21 Dec 2024 14:33:04 +0100 Subject: [PATCH 6/8] libcontainerd/supervisor: remove uses of deprecated system.MkdirAll Signed-off-by: Sebastiaan van Stijn --- libcontainerd/supervisor/remote_daemon.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libcontainerd/supervisor/remote_daemon.go b/libcontainerd/supervisor/remote_daemon.go index 8998f36f7f..7611291971 100644 --- a/libcontainerd/supervisor/remote_daemon.go +++ b/libcontainerd/supervisor/remote_daemon.go @@ -16,7 +16,6 @@ import ( "github.com/containerd/log" "github.com/docker/docker/pkg/pidfile" "github.com/docker/docker/pkg/process" - "github.com/docker/docker/pkg/system" "github.com/moby/buildkit/util/grpcerrors" "github.com/pelletier/go-toml" "github.com/pkg/errors" @@ -93,7 +92,7 @@ func Start(ctx context.Context, rootDir, stateDir string, opts ...DaemonOpt) (Da } } - if err := system.MkdirAll(stateDir, 0o700); err != nil { + if err := os.MkdirAll(stateDir, 0o700); err != nil { return nil, err } From 1e060d3315f88e1697836aecac5499f4d28c9fbe Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 21 Dec 2024 14:38:23 +0100 Subject: [PATCH 7/8] daemon/graphdriver/windows: remove uses of deprecated system.MkdirAll Signed-off-by: Sebastiaan van Stijn --- daemon/graphdriver/windows/windows.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/daemon/graphdriver/windows/windows.go b/daemon/graphdriver/windows/windows.go index 9000e81a9a..77947df197 100644 --- a/daemon/graphdriver/windows/windows.go +++ b/daemon/graphdriver/windows/windows.go @@ -31,7 +31,6 @@ import ( "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/ioutils" "github.com/docker/docker/pkg/longpath" - "github.com/docker/docker/pkg/system" "github.com/docker/go-units" "github.com/moby/sys/reexec" "github.com/pkg/errors" @@ -103,7 +102,7 @@ func InitFilter(home string, options []string, _ idtools.IdentityMapping) (graph // Setting file-mode is a no-op on Windows, so passing "0" to make it more // transparent that the filemode passed has no effect. - if err = system.MkdirAll(home, 0); err != nil { + if err = os.MkdirAll(home, 0); err != nil { return nil, errors.Wrapf(err, "windowsfilter failed to create '%s'", home) } From c759fb20d64a07d3df0aa56aa620c45fb443a793 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 21 Dec 2024 14:39:43 +0100 Subject: [PATCH 8/8] daemon: remove uses of deprecated system.MkdirAll Signed-off-by: Sebastiaan van Stijn --- daemon/daemon.go | 3 +-- daemon/runtime_unix.go | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index 6212b4e43c..6fa2e13838 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -66,7 +66,6 @@ import ( "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/plugingetter" "github.com/docker/docker/pkg/sysinfo" - "github.com/docker/docker/pkg/system" "github.com/docker/docker/plugin" pluginexec "github.com/docker/docker/plugin/executor/containerd" refstore "github.com/docker/docker/reference" @@ -810,7 +809,7 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S return nil, fmt.Errorf("Unable to get the full path to the TempDir (%s): %s", tmp, err) } if isWindows { - if err := system.MkdirAll(realTmp, 0); err != nil { + if err := os.MkdirAll(realTmp, 0); err != nil { return nil, fmt.Errorf("Unable to create the TempDir (%s): %s", realTmp, err) } os.Setenv("TEMP", realTmp) diff --git a/daemon/runtime_unix.go b/daemon/runtime_unix.go index 383f20dfc6..acb058faf2 100644 --- a/daemon/runtime_unix.go +++ b/daemon/runtime_unix.go @@ -22,7 +22,6 @@ import ( "github.com/docker/docker/errdefs" "github.com/docker/docker/libcontainerd/shimopts" "github.com/docker/docker/pkg/ioutils" - "github.com/docker/docker/pkg/system" "github.com/opencontainers/runtime-spec/specs-go/features" "github.com/pkg/errors" ) @@ -94,7 +93,7 @@ func initRuntimesDir(cfg *config.Config) error { if err := os.RemoveAll(runtimeDir); err != nil { return err } - return system.MkdirAll(runtimeDir, 0o700) + return os.MkdirAll(runtimeDir, 0o700) } func setupRuntimes(cfg *config.Config) (runtimes, error) {