pkg/system: deprecate MkdirAll and remove custom volume GUID handling

thaJeztah · thaJeztah · commit 4472e9b7f876 · 2024-12-21T14:19:01.000+01:00
commit 86d1223 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 46ec4c1 to provide ACL (DACL) support on Windows. Further updates were made in cfef1b1 and 55ceb50 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 f058afc, 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]: golang/go@b86e766 [golang.org/cl/86295]: golang/go@cd589c8 [golang.org/cl/582499]: golang/go@5616ab6 [golang.org/cl/581517]: golang/go@ad22356 [golang.org/cl/566556]: golang/go@ceef063 [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 <github@gone.nl>
diff --git 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
@@ -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
@@ -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)
-}
-
-// 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)
+	return mkdirAllWithACL(path, sa)
 }
 
-// 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))
