From e02fdc4cfaf3a82cd15d0f7200000e43887a819a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 23 Jul 2026 12:23:23 +0200 Subject: [PATCH] user: align Windows mkdir behavior with Unix The Windows implementation of mkdirAs was originally introduced as a minimal wrapper around os.MkdirAll because ownership is not supported on Windows and the uid/gid parameters were only needed to keep the daemon starting. As a result, MkdirAndChown would also create missing parent directories, unlike the Unix implementation. Use os.Mkdir when mkAll is false to preserve the documented semantics, while retaining the simplified Windows implementation for ownership and permissions. Originally introduced in [moby/moby@bfe252b78184][1], which added a Windows-specific implementation to work around limitations on that platform; > A recent change to use pkg\idtools causes a chown to be on the > startup path of the daemon. Chown is not supported on Windows, > hence the daemon would not start. [1]: https://github.com/moby/moby/commit/bfe252b78184d22d74a555a82aafc9e6dae3babc Signed-off-by: Sebastiaan van Stijn --- user/idtools_test.go | 60 +++++++++++++++++++++++++++++++++++++++ user/idtools_unix_test.go | 16 ----------- user/idtools_windows.go | 25 ++++++++++++---- 3 files changed, 79 insertions(+), 22 deletions(-) create mode 100644 user/idtools_test.go diff --git a/user/idtools_test.go b/user/idtools_test.go new file mode 100644 index 00000000..5e47950f --- /dev/null +++ b/user/idtools_test.go @@ -0,0 +1,60 @@ +package user_test + +import ( + "errors" + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/moby/sys/user" +) + +// TestMkdirAndChownNonDir checks that MkdirAndChown returns a correct error in case +// a directory which it is about to create already exists but is a file (rather +// than a directory). +func TestMkdirAndChownNonDir(t *testing.T) { + file, err := os.CreateTemp(t.TempDir(), t.Name()) + if err != nil { + t.Fatalf("Couldn't create temp dir: %v", err) + } + _ = file.Close() + + expected := syscall.ENOTDIR + err = user.MkdirAndChown(file.Name(), 0o755, 0, 0) + if !errors.Is(err, expected) { + t.Fatalf("expected error: %v, got: %v", expected, err) + } +} + +// TestMkdirAndChownExistingDir checks that MkdirAndChown does not return an error +// if the target directory already exists. +func TestMkdirAndChownExistingDir(t *testing.T) { + dirName := t.TempDir() + err := user.MkdirAndChown(dirName, 0, 0, 0, user.WithOnlyNew) + if err != nil { + t.Fatal(err) + } +} + +// TestMkdirAndChownMissingParent checks that MkdirAndChown errors if the parent +// directory doesn't exist and doesn't create any of the parent directories. +func TestMkdirAndChownMissingParent(t *testing.T) { + dirName := t.TempDir() + if err := user.MkdirAndChown(filepath.Join(dirName, "usr", "bin", "subdir"), 0, 0, 0, user.WithOnlyNew); err == nil { + t.Fatal("Trying to create a directory with Mkdir where the parent doesn't exist should have failed") + } + + _, err := os.Stat(filepath.Join(dirName, "usr")) + if err == nil || !os.IsNotExist(err) { + t.Fatal("parent directory should not have been created", err) + } + _, err = os.Stat(filepath.Join(dirName, "usr", "bin")) + if err == nil || !os.IsNotExist(err) { + t.Fatal("parent directory should not have been created", err) + } + _, err = os.Stat(filepath.Join(dirName, "usr", "bin", "subdir")) + if err == nil || !os.IsNotExist(err) { + t.Fatal("directory should not have been created", err) + } +} diff --git a/user/idtools_unix_test.go b/user/idtools_unix_test.go index 5e0bcce5..5482aa4e 100644 --- a/user/idtools_unix_test.go +++ b/user/idtools_unix_test.go @@ -376,22 +376,6 @@ func TestToContainer(t *testing.T) { } } -// TestMkdirIsNotDir checks that MkdirAndChown returns a correct error in case -// a directory which it is about to create already exists but is a file (rather -// than a directory). -func TestMkdirIsNotDir(t *testing.T) { - file, err := os.CreateTemp(t.TempDir(), t.Name()) - if err != nil { - t.Fatalf("Couldn't create temp dir: %v", err) - } - - expected := "mkdir " + file.Name() + ": not a directory" - err = MkdirAndChown(file.Name(), 0o755, 0, 0) - if err == nil || err.Error() != expected { - t.Fatalf("expected error: %v, got: %v", expected, err) - } -} - func requiresRoot(t *testing.T) { if os.Getuid() != 0 { t.Skip("skipping test that requires root") diff --git a/user/idtools_windows.go b/user/idtools_windows.go index d83ec902..3973d559 100644 --- a/user/idtools_windows.go +++ b/user/idtools_windows.go @@ -2,12 +2,25 @@ package user import ( "os" + "syscall" ) -// 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, _, _ int, _ bool, _ ...MkdirOpt) error { - return os.MkdirAll(path, 0) +// mkdirAs creates path, optionally creating any missing parent directories. +// +// On Windows this is currently a thin wrapper around os.Mkdir and +// os.MkdirAll. Unlike the Unix implementation, ownership and permission +// bits are not applied. +func mkdirAs(path string, _ os.FileMode, _, _ int, mkAll bool, _ ...MkdirOpt) error { + if mkAll { + return os.MkdirAll(path, 0) + } + stat, err := os.Stat(path) + if err == nil { + if !stat.IsDir() { + return &os.PathError{Op: "mkdir", Path: path, Err: syscall.ENOTDIR} + } + return nil + } + + return os.Mkdir(path, 0) }