cc @golang/windows

Windows docs have a special entry for dealing with whitespaces in files and folder names: https://docs.microsoft.com/en-us/troubleshoot/windows-client/shell-experience/file-folder-name-whitespace-characters.

The relevant part is this:

File and Folder names that begin or end with the ASCII Space (0x20) will be saved without these characters. File and Folder names that end with the ASCII Period (0x2E) character will also be saved without this character. All other trailing or leading whitespace characters are retained.

Looking at how MkdirAll is implemented on windows, it seems that we are not removing leading and trailing whitespaces from non-leaf folders. This leads to failure mode:

1. MkdirAll("C:\temp\folder \this one fails", 0644)
2. MkdirAll("C:\temp", 0644)
3. MkdirAll("C:\temp\folder ", 0644) // But Windows created "C:\temp\folder"!!
4. Mkdir("C:\temp\folder \this one fails", 0644) // Wops, parent folder does not exist

how much normalization should we do vs having the user specify \\?\ when they need weird paths?

Normalizing this should be trivial, but I would argue against doing so. It would fix the MkdirAll, yet the underlying problem will still be there: the white-spaced path won't exist even though MkdirAll succeed, so any other operation on that path is going to fail.

We either always normalize all paths or we don't, else we will just be moving the failure to a later point, making it worst.

As @seankhliao mentioned, Windows already supports trailing whitespaces when the path is prefixed with \\?\, so the workaround is there.

Additional data point: .Net does not support this either for the sake of interoperability. dotnet/runtime#23292 (comment).

I am also against normalizing this. If Windows changes its behavior in the future, Go will have to follow the changes.

Normalizing this should be trivial, but I would argue against doing so. It would fix the MkdirAll, yet the underlying problem will still be there: the white-spaced path won't exist even though MkdirAll succeed, so any other operation on that path is going to fail.

We either always normalize all paths or we don't, else we will just be moving the failure to a later point, making it worst.

As @seankhliao mentioned, Windows already supports trailing whitespaces when the path is prefixed with \\?\, so the workaround is there.

Additional data point: .Net does not support this either for the sake of interoperability. dotnet/runtime#23292 (comment).

Creating the folder with \?\ works with MkdirAll. Can we make this the default behavior on windows? Otherwise the programmer needs to prefix the MkdirAll call if needed. I wonder if anyone even knows about that special \?\ thing.

I wonder if anyone even knows about that special \\?\ thing.

From Windows docs:

For file I/O, the "\?" prefix to a path string tells the Windows APIs to disable all string parsing and to send the string that follows it straight to the file system.

We either always normalize all paths or we don't, else we will just be moving the failure to a later point, making it worse.

I agree that we should not normalize paths here, but could we at least improve the failure mode here?

(Perhaps something in the call stack could notice the leading or trailing space in the intermediate paths and fail with a clearer error message.)

I agree that we should not normalize paths here, but could we at least improve the failure mode here?

Improving the error message seems like a good idea, but I would try to avoid adding logic to detect leading/trailing whitespaces or periods and instead rely on giving general tips based on windows error codes.

For example, MkdirAll logic ensures that intermediate directories exist by calling CreateDirectory on every non-existing intermediate directory. So, if syscall.CreateDirectory returns ERROR_PATH_NOT_FOUND (see error description in docs), it means that either a concurrent process deleted the directory or that Windows did not honor the folder path in a previous CreateDirectory call.

Change https://go.dev/cl/458136 mentions this issue: os: handle trailing spaces case on MkdirAll

@bcmills @qmuntal I realized that syscall.ERROR_PATH_NOT_FOUND is Windows OS specific and it will not compile on different platforms. How could I avoid this or wrap it properly?

@vladtenlive Use os.IsNotExist(err).

Or I guess that we now recommend the equivalent errors.Is(err, fs.ErrNotExist).

@ianlancetaylor fixed, thank you
https://go-review.googlesource.com/c/go/+/458136

I would try to avoid adding logic to detect leading/trailing whitespaces or periods and instead rely on giving general tips based on windows error codes.
…
if syscall.CreateDirectory returns ERROR_PATH_NOT_FOUND (see error description in docs), it means that either a concurrent process deleted the directory or that Windows did not honor the folder path in a previous CreateDirectory call.

I think that leaves us back where we started: if we can't distinguish between “a concurrent process deleted the directory“ and “Windows did not honor the folder path in a previous CreateDirectory call”, then the existing ERROR_PATH_NOT_FOUND message seems like about the best we can do.

Normalizing this should be trivial, but I would argue against doing so. It would fix the MkdirAll, yet the underlying problem will still be there: the white-spaced path won't exist even though MkdirAll succeed, so any other operation on that path is going to fail.

Note that many of the functions in the os package already call the fixLongPath helper function, which converts paths of length ≥ 248 to the equivalent \\?\ form provided that os.canUseLongPaths is also false (see #3358, #41734).

So under certain conditions today, Mkdir (and MkdirAll) will actually succeed in creating the space-containing paths as requested.

In other conditions, Mkdir silently creates a path that differs by a trailing character, and MkdirAll creates a path that differs from some parent directory by a trailing character, then fails. Neither of those behaviors seems like what the user intends, and neither is particularly useful.

I think that, rather than letting the OS do as it likes and possibly fail in ways that are subtle and difficult to predict, we should explicitly choose to either always succeed or always fail for these paths. That is, we should choose either:

a. Change os.Mkdir (and probably also os.OpenFile with O_CREATE as well?) to check path components for non-UNC paths for trailing ASCII spaces and periods, and explicitly reject paths that end in an ASCII space or period.

• As a side effect, that would cause os.MkdirAll to refuse to create such paths.
• That would be consistent with https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions, which says in no uncertain terms: “Do not end a file or directory name with a space or a period.”
• Users who intentionally create such files or directories could switch to using the \\?\ form to manipulate them.
• Opening such files and/or directories for reading would not be rejected explicitly, but other existing bugs may prevent their use. (For example, os.ReadDir at HEAD seems to fail for such a directory.)

or:

b. Change (and rename) fixLongPath to check for such paths and escape them to their corresponding \\?\ forms.

• This would allow Go programs to create files and directories with names with trailing space or period characters if the underlying filesystem permits them, which would make the behavior of Go programs on Windows more consistent with their behavior on Unix platforms.
  • However, it would not fix other functions like os.ReadDir that fail for such paths today, so the improved consistency would be limited at best. (Unless we fully open the can of worms and try to fix everything to work with these paths...)
• It would also not fix the trailing-space problem for relative paths until os: handle relative paths in fixLongPath on Windows #41734 is also addressed.

On balance, I would prefer to make Mkdir and OpenFile reject these paths explicitly.

(CC @neild @zx2c4 for Windows path normalization, the gift that keeps on giving)

I would suggest the following (https://go.dev/play/p/T63O30FOM8Y?v=gotip) as a regression test. It passes today on Linux.

// Regression test for https://go.dev/issue/54040:
// Mkdir for a path ending with a trailing ASCII space (or period)
// should either return a non-nil error or create a usable directory.
func TestMkdirTrailingSpace(t *testing.T) {
	parent := t.TempDir()

	dir := filepath.Join(parent, "ends in space ")
	err := os.Mkdir(dir, 0777)
	if err != nil {
		if runtime.GOOS == "windows" && !strings.HasPrefix(dir, `\\?\`) {
			t.Skipf("skipping: Windows does not support paths with trailing spaces")
		}

		t.Fatal(err)
	}

	t.Logf("Mkdir(%q, 0777)", dir)
	t.Cleanup(func() {
		err := os.Remove(dir)
		if err == nil {
			t.Logf("Remove(%q)", dir)
		} else {
			t.Error(err)
		}
	})

	fi, err := os.Lstat(dir)
	if err == nil {
		t.Logf("Lstat(%q).Name() = %q", dir, fi.Name())
	} else {
		t.Error(err)
	}

	fi, err = os.Stat(dir)
	if err == nil {
		t.Logf("Stat(%q).Name() = %q", dir, fi.Name())
	} else {
		t.Error(err)
	}

	ents, err := os.ReadDir(dir)
	if err == nil {
		t.Logf("ReadDir(%q) = %v", dir, ents)
	} else {
		t.Error(err)
	}

	sub := filepath.Join(dir, "sub")
	err = os.Mkdir(sub, 0777)
	if err == nil {
		t.Logf("Mkdir(%q, 0777)", sub)
		err := os.Remove(sub)
		if err == nil {
			t.Logf("Remove(%q)", sub)
		} else {
			t.Error(err)
		}
	} else {
		t.Error(err)
	}
}

Change os.Mkdir (and probably also os.OpenFile with O_CREATE as well?) to check path components for non-UNC paths for trailing ASCII spaces and periods, and explicitly reject paths that end in an ASCII space or period.

This option sounds reasonable to me. (Although rather than "for non-UNC paths", say "for paths not starting with \\?\--UNC paths are something different.)

Thank you, @bcmills, for such a great explanation with the possible options.
I will start working on approach a., check it with the regression test you provided, and share the results here for further discussion.

a. Change os.Mkdir (and probably also os.OpenFile with O_CREATE as well?) to check path components for non-UNC paths for trailing ASCII spaces and periods, and explicitly reject paths that end in an ASCII space or period.

Change https://go.dev/cl/618496 mentions this issue: syscall: check for valid Windows path in Open and Mkdir

The more I think about the "option a" detailed in #54040 (comment) (and implemented in CL 618496), the less I like it, as it is a breaking change. Until today, one could do something like the following (pseudo)code snippet and assume Windows compatibility layer to chime by creating and opening a file without the trailing space.

const name = "test.txt "
os.Create(name)
os.Open(name)

After CL 618496, Go will error out in os.Create before letting Windows do its best effort.

IMO fixing the original issue (os.MkdirAll not working well when the path contains trailing spaces) doesn't worth introducing a breaking change, even if the new behavior would be more consistent and robust.

Also, thanks to the work done in #67002, we will soon have some additional primitives (mkdirat, openat, ...) that will allow us to reimplement Mkdirall in a way that won't trigger the issue reported in here. That is, os.MkdirAll("foo /bar") will successfully create "foo/bar" (which is weird, but it is what one expects on Windows).

Having said all this, I propose to revert CL 618496 and wait till Go 1.25 to implement a better fix.

@neild @gdams @golang/windows

Having said all this, I propose to revert CL 618496 and wait till Go 1.25 to implement a better fix.

I agree, I'll revert the CL now via CL 629595