os: MkdirAll returns error with 'NUL' on Windows

[djdv]

What version of Go are you using (go version)?

go1.10 windows/amd64,
go version devel +b63b0f2b75 Tue Mar 27 08:16:50 2018 +0000 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOOS=windows GOARCH=amd64

What did you do?

Tried to use os.MkdirAll on "NUL": os.MkdirAll("NUL", 0777)

What did you expect to see?

no error returned, as is the case with: os.Mkdir("NUL", 0777).

What did you see instead?

An os.PathError: The system cannot find the path specified.

Additional

Similar to #24482

https://golang.org/src/os/path.go?s=705:716#L24
Is the problem area, since NUL is not considered a directory, MkDirAll returns a PathError.
In the Windows command interpreter md NUL and md intermediate\paths\NUL create no paths and return no errors.
However md NUL\path\beyond will return an error.

Attn: @alexbrainman

[tklauser]

/cc @alexbrainman

[as]

nul is the /dev/null of Windows. There are other "devices" that can't exist without causing problems: mkdir aux and mkdir con, for example, return The directory name is invalid. and set %ERRORLEVEL% to a non-zero value.

These files can be created, but they will break many programs that try to open the directories containing such filenames. In my opinion, the error is the correct thing to do and Windows is wrong.

[djdv]

@as
Regardless of which is right or wrong, there is an inconsistency in Go either way. With the same target (NUL), os.Mkdir succeeds while os.MkdirAll does not.

Given that this is the os pkg, I would expect it to mirror the platform native behaviour, if md/mkdir on Windows succeeds with this target, I expect os.Mkdir to also succeed here (which it currently does). If os.Mkdir succeeds, I expect os.MkdirAll to also succeed with the same target (which it currently does not).

I expect an error with other reserved names (such as CON), but not NUL, which is the subject here.

[as]

@djdv I don't agree that the behavior of cmd is platform native behavior. mkdir is a shell builtin in cmd.exe, the powershell mkdir returns an error:

PS C:\Users\as> md nul
md : The directory specified, 'nul', is not a subdirectory of 'C:\Users\as'.
At line:1 char:1
+ md nul
+ ~~~~~~
    + CategoryInfo          : InvalidArgument: (C:\Users\as\nul:String) [New-Item], ArgumentException
    + FullyQualifiedErrorId : CreateDirectoryArgumentError,Microsoft.PowerShell.Commands.NewItemCommand

[djdv]

@as
Golang seems to be calling CreateDirectory / CreateDirectoryW for mkdir

go/src/syscall/syscall_windows.go

Lines 410 to 416 in 360c191

	func Mkdir(path string, mode uint32) (err error) {
	pathp, err := UTF16PtrFromString(path)
	if err != nil {
	return err
	}
	return CreateDirectory(pathp, nil)
	}

The Windows API succeeds here.

#include <windows.h>

int main(void) {
	return CreateDirectory(L"NUL", NULL);
}

https://msdn.microsoft.com/en-us/library/windows/desktop/aa363855(v=vs.85).aspx

If the function succeeds, the return value is nonzero.

I'm receiving a return of 1 (call succeeded) and a GetLastError of 0 (Success).

[as]

I tested all the alleged device files on windows. NUL is the only one that mkdir returns successfully for.

What I don't understand is why this behavior is necessary. os.MkdirAll calls os.Stat() on the given path, it needs to do this to walk the tree and determine which directories need to be created.

os.Stat fails on device files like NUL. Why should there be a special case for this? The result is invalid regardless because the directory NUL can't be created.

It seems more sane to run os.Stat on the target and output an error for the os.Mkdir case than hack os.MkdirAll to return an incorrect success because windows did it for mkdir.

What problem would this change solve for you that justifies adding the proposed behavior?

[djdv]

@as

I tested all the alleged device files on windows. NUL is the only one that mkdir returns successfully for.

I'm not proposing that Go handle all Windows device files, only NUL is the subject here. For clarity, I am not making a proposition, I am reporting this inconsistency within the os package, stating the behaviour of the platform API, and awaiting a response.

What I don't understand is why this behavior is necessary

The intention of the os pkg seems to be to interface with the OS, this behaviour would fall in line with the OS API convention. I fail to see the purpose of defining os.DevNull on Windows but not allowing it to be used in 1 specific os functions (, a function that natively would succeed). Mkdir, Stat, Open, Write, etc. all work with NUL as the target or handle, it is simply MkdirAll that has the issue.

os.MkdirAll calls os.Stat() on the given path, it needs to do this to walk the tree and determine which directories need to be created.

I see no problem here, as it is now, the full path can be stat'ed and recursively created via os.Mkdir, the current implementation fails early, before attempting it.

os.Stat fails on device files like NUL.

It does not, os.Stat("NUL") will return a valid FileInfo struct for the null-device, the problem lies in the fast path receiving false from dir.IsDir(), assuming creation would fail, and (erroneously) asserting an error based on that.

go/src/os/path.go

Lines 20 to 28 in dafca7d

	func MkdirAll(path string, perm FileMode) error {
	// Fast path: if we can tell whether path is a directory or file, stop with success or error.
	dir, err := Stat(path)
	if err == nil {
	if dir.IsDir() {
	return nil
	}
	return &PathError{"mkdir", path, syscall.ENOTDIR}
	}

It's true that NUL is not an existing directory, but that does not mean calls to create it (or its parents) will fail, the error is artificially inserted by Go. In actuality, system calls to CreateDir will not fail when supplied with NUL as a target.

Since there is (to my knowledge) no Windows system call to create directories recursively, that seems to be left up to implementation. I think a sane expectation for Go's os.MkdirAll(`C:\some\intermediate\absolute\path\NUL`) is to create the parent paths up to NUL and finally pass NUL itself to os.Mkdir which in turn will succeed, leaving you with a structure of C:\some\intermediate\absolute\path. This would require no change other than not erroring early on paths with a base of NUL.

The big issue here is that these are valid Windows paths that should NOT be causing an error, but currently are. The use cases for NUL on Windows are the same as any other null-device, you can use it to discard output, on Windows, this includes files and directories. On non-Windows there are constructs like /dev/null and /var/empty which serve a similar purpose as NUL.

[as]

The use cases for NUL on Windows are the same as any other null-device, you can use it to discard output, on Windows, this includes files and directories.

Sorry, this isn't true. Creating a directory isn't the same as writing to an open file. If I run mkdir /dev/null on Linux, macOS, or plan9, the operation will fail, it will not treat the directory creation process as output to pipe into /dev/null.

Since there is (to my knowledge) no Windows system call to create directories recursively, that seems to be left up to implementation. I think a sane expectation for Go's os.MkdirAll(C:\some\intermediate\absolute\path\NUL) is to create the parent paths up to NUL and finally pass NUL itself to os.Mkdir

Which is just a footgun, because after a successful call to os.MkdirAll, the next step is usually to create a file in the directory. Are you saying that os.MkdirAll should return no error if it hasn't successfully created the entire directory tree?

[djdv]

@as

Sorry, this isn't true.

I did not mean to imply /dev/null works with mkdir on *nix, I'm just stating the platform behaviour for Windows and similarities with other platforms. On Windows this device handles both of these operations regardless of if the intent was to create or open a file, or to create a directory. Again the topic is the NUL device on Windows, not other platforms, those are just the closest analogs.

I feel like

Which is just a footgun, because after a successful call to os.MkdirAll, the next step is usually to create a file in the directory.

diverges from the actual issue, the footgun in question doesn't relate to os.MkdirAll it relates to the lack of processing errors and making assumptions. Even in events where directories are made, file creation can't always be assured. Making the assumption that something will succeed just because something else before it did, seems like a bad practice to me.

It's moot either way since it's an existing issue

outPath := `NUL`
err := os.Mkdir(outPath, 0755`)
if err == nil {
    os.Create(`outPath\file.ext`) // this will hit
}

This can easily be rectified by taking advantage of the error returned by os.Create in one way or another.

In addition the error returned currently makes more sense in the Create context than it does MkdirAll.
Assume outPath == NUL, I see no reason as to why os.MkdirAll(outPath, 0755) returns

The system cannot find the path specified

It makes much more sense to me that os.Create(filepath.Join(outPath, "file.ext")) return that error.

---

What's important is not how /dev/null works, or users who don't check their errors, we're dealing with an inconsistency in the os pkg and have to decide which path to take to resolve it. Either allow NUL in MkdirAll on Windows to succeed or to return an error specifically disallowing it.

No matter what decision is made, a special case has to be put in place to allow or prevent this, either in MkdirAll(allow) or both MkdirAll & Mkdir(prevent).

If I were to make a proposal I would say that MkdirAll should not fail early with NUL and instead pass through to Mkdir, which in turn hands it over to the system, where finally it will return the platform appropriate value. The onus to check input and errors for separate os calls is placed on the user, which I don't think is unfair or unusual to expect.

My reason for supporting that behaviour is that it's consistent with the underlying system which seems to fit the purpose of pkg os, to act as an interface between Go and the OS.
This would allow for setting output paths simply to NUL to discard file and directory creation which can be useful if a user wishes to run some procedure without output being generated on disk. I don't think "that's different than how Unix does it" and "some users make assumptions", are strong enough arguments against this when we're dealing with the os pkg. There are many differences between platforms and this is 1 of them, instead of blocking it, we should let it pass, as it falls in line with the conventions of the underlying system, if you're targeting that system, this is the expectation. As for users ignoring errors I don't think that's a fair discussion to have.

The current behaviour is purely coincidental, there is no guard against null-devices specifically, it just happens to fail on NUL by virtue of it not being a directory, the error returned doesn't even make sense in this context.

---

As for implementation on either front, it may be easiest to utilize ModeCharDevice.
We can check against it and compare the path (or more importantly the base of the path) to the DevNull constant, this check would have to be pulled out into a file with build constraints, so that it only passes through on platforms that allow this, something along the lines of if NullAllowed(path)... but I have placed this inline as an example below.

Allowing the system to decide the return value, could be handled with

 func MkdirAll(path string, perm FileMode) error { 
 	// Fast path: if we can tell whether path is a directory or file, stop with success or error. 
 	dir, err := Stat(path) 
 	if err == nil { 
 		if dir.IsDir() { 
 			return nil 
 		} 
		if dir.Mode() != ModeCharDevice || pathBase != DevNull {
			return &PathError{"mkdir", path, syscall.ENOTDIR} 
		}
		//... continue on to recursive portion
} 

Denying this would still require the same checks to be in place, but inverted, for both Mkdir and MkdirAll. In addition a better error message should be returned. Something along the lines of "Will not create paths ending in %q", DevNull.

i.e.

if dir.Mode() == ModeCharDevice && pathBase == DevNull {
 return PathErrDevnull
}

[alexbrainman]

I was surprised to see that os.Mkdir("NUL") indeed succeeds, but it returns whatever Windows CreateDirectory returns. That call does not create NUL directory - it looks like it does nothing.

I don't care if os.Mkdir("NUL") succeeds or fails.

But, I suppose, Go should be consistent and have os.Mkdir and os.MkdirAll both succeed or both fail with nice error message. I cannot decide which, so leaving it for others.

Alex

[djdv]

@alexbrainman
Thanks for the input.

I suppose, Go should be consistent and have os.Mkdir and os.MkdirAll both succeed or both fail with nice error message.

Agreed.

leaving it for others

Who has experience/authority of Windows issues here?
I've seen @mattn in a lot of Windows related things.
If anyone developing on Windows is reading this and has opinions, please voice them.

I'm in favour of succeeding.
@as seems to be in favour of erroring

[mattn]

If you use UNC path to create directory, you'll get it. Personally, I think this Windows behavior is NOT correct. But Windows API returns it as normal, we have to obey it. Impersonate the error code or add the workaround for the path are not proper way. os package is not a package to hide differences between OSs. The aim is to provide a consistent API. What Go can do is only providing way to use UNC paths and non-UNC paths both on Windows.

package main

import (
	"fmt"
	"log"
	"os"
	"path/filepath"
)

func main() {
	wd, err := os.Getwd()
	if err != nil {
		log.Fatalf("Getwd: %v", err)
	}

	dir := `\\.\` + filepath.Join(wd, "NUL")
	err = os.MkdirAll(dir, 0755)
	if err != nil {
		log.Fatalf("MkdirAll: %v", err)
	}

	fi, err := os.Stat(dir)
	if err != nil {
		log.Fatalf("Stat: %v", err)
	}
	fmt.Println(fi.IsDir()) // true

	err = os.Remove(dir)
	if err != nil {
		log.Fatalf("Remove: %v", err)
	}

	dir = `\\.\` + filepath.Join(wd, "NUL", "CON", "PRT")
	err = os.MkdirAll(dir, 0755)
	if err != nil {
		log.Fatalf("MkdirAll: %v", err)
	}
	f, err := os.Create(`\\.\` + filepath.Join(wd, "NUL", "CON", "PRT", "Hi-Gopher.txt"))
	if err != nil {
		log.Fatalf("Create: %v", err)
	}
	_, err = f.Write([]byte("I love golang!"))
	if err != nil {
		log.Fatalf("Write: %v", err)
	}
	err = f.Close()
	if err != nil {
		log.Fatalf("Close: %v", err)
	}

	err = os.RemoveAll(`\\.\` + filepath.Join(wd, "NUL"))
	if err != nil {
		log.Fatalf("RemoveAll: %v", err)
	}
}

[as]

Even in events where directories are made, file creation can't always be assured.

Under what practical conditions would you expect touch to fail

mkdir foo && touch foo/bar

[alexbrainman]

If you use UNC path to create directory, you'll get it. Personally, I think this Windows behavior is NOT correct. But Windows API returns it as normal, we have to obey it.

We are not discussing UNC paths, this issue is only about "NUL" file.

@djdv @mattn @as and anyone else who cares,

should we

1. make os.Mkdir("NUL") fail

or

2. make os.MkdirAll("NUL") succeed

? And why?

Thank you.

Alex

[mattn]

So we should remove this part?

diff --git a/src/os/path.go b/src/os/path.go
index 5c5350670d..81033ea5e0 100644
--- a/src/os/path.go
+++ b/src/os/path.go
@@ -18,14 +18,7 @@ import (
 // If path is already a directory, MkdirAll does nothing
 // and returns nil.
 func MkdirAll(path string, perm FileMode) error {
-	// Fast path: if we can tell whether path is a directory or file, stop with success or error.
-	dir, err := Stat(path)
-	if err == nil {
-		if dir.IsDir() {
-			return nil
-		}
-		return &PathError{"mkdir", path, syscall.ENOTDIR}
-	}
+	var err error
 
 	// Slow path: make sure parent exists and then call Mkdir for path.
 	i := len(path)

FYI MkdirAll seems having this part from 66f5e89 (2009).

[alexbrainman]

So we should remove this part?

We cannot remove that part. That part makes MkdirAll faster.

Alex

[djdv]

I posted this above, I modified the code a bit.

As for implementation on either front, it may be easiest to utilize ModeCharDevice.
We can check against it and compare the path (or more importantly the base of the path) to the DevNull constant, this check would have to be pulled out into a file with build constraints, so that it only passes through on platforms that allow this, something along the lines of if NullAllowed(path)... but I have placed this inline as an example below.
Allowing the system to decide the return value, could be handled with

 func MkdirAll(path string, perm FileMode) error { 
 	// Fast path: if we can tell whether path is a directory or file, stop with success or error. 
 	dir, err := Stat(path) 
 	if err == nil { 
 		if dir.IsDir() { 
 			return nil 
 		} 
 		// if not a char device, if not devnul, return error, otherwise proceed
		if dir.Mode()&ModeCharDevice == 0 || !isDevNul(pathBase) {
			return &PathError{"mkdir", path, syscall.ENOTDIR} 
		}
		//... continue on to recursive portion
} 

I'm not sure if this is a good condition to branch off of but I think adding some condition here may be the solution. As long as that condition is only in the Windows implementation.

[alexbrainman]

I'm not sure if this is a good condition to branch off ...

Why are we discussing solution? We have not decided what to do yet. See my question here #24556 (comment)

Alex

[djdv]

I believe in either case we'll need a similar approach. This was meant more as a response to @mattn's proposal.
To demonstrate that we can modify the fast path rather than remove it completely.
Regardless of our decision I think we're all in agreement that the fast path should not be removed if it can be avoided.

Edit:
Consensus thus far seems to be
Abstain:
@alexbrainman

make os.Mkdir("NUL") fail:
@as
Rationale: It may be confusing to return success in this scenario, directory creation was a success but no directory exists.

make os.MkdirAll("NUL") succeed:
@djdv, @mattn
Rationale: Consistent with the platform's API. If an error is to be returned, it should be returned by the system here.

If I've misinterpreted the responses/opinions, please correct me.

[ianlancetaylor]

I don't know if we need to do anything here. But if we do something, I think the only correct change would be to make os.Mkdir("NUL", mode) return an error. From what people say above that call does not do anything, and also does not return an error. That is not ideal, regardless of what the Windows command interpreter does.

[alexbrainman]

I think the only correct change would be to make os.Mkdir("NUL", mode) return an error. From what people say above that call does not do anything, and also does not return an error. That is not ideal, regardless of what the Windows command interpreter does.

It is not the Windows command interpreter, it is Windows CreateDirectory API returns success.

https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-createdirectoryw

I built this little test

diff --git a/src/os/path_test.go b/src/os/path_test.go
index 6cb25bcaa7..69fafc8723 100644
--- a/src/os/path_test.go
+++ b/src/os/path_test.go
@@ -126,3 +126,10 @@ func TestMkdirAllAtSlash(t *testing.T) {
        }
        RemoveAll("/_go_os_test")
 }
+
+func TestMkdirDevNull(t *testing.T) {
+       err := MkdirAll(DevNull, 0777)
+       t.Errorf("os.MkdirAll(os.DevNull) error: %q %#v", err, err)
+       err = Mkdir(DevNull, 0777)
+       t.Errorf("os.Mkdir(os.DevNull) error: %q %#v", err, err)
+}

and it outputs this on Linux

$ go test -v -run=TestMkdirDev
=== RUN   TestMkdirDevNull
--- FAIL: TestMkdirDevNull (0.00s)
    path_test.go:132: os.MkdirAll(os.DevNull) error: "mkdir /dev/null: not a directory" &os.PathError{Op:"mkdir", Path:"/dev/null", Err:0x14}
    path_test.go:134: os.Mkdir(os.DevNull) error: "mkdir /dev/null: file exists" &os.PathError{Op:"mkdir", Path:"/dev/null", Err:0x11}
FAIL
exit status 1
FAIL    os      0.006s

and this on Windows

C:\> go test -v -run=TestMkdirDev
=== RUN   TestMkdirDevNull
--- FAIL: TestMkdirDevNull (0.01s)
    path_test.go:132: os.MkdirAll(os.DevNull) error: "mkdir NUL: The system cann
ot find the path specified." &os.PathError{Op:"mkdir", Path:"NUL", Err:0x3}
    path_test.go:134: os.Mkdir(os.DevNull) error: %!q(<nil>) <nil>
FAIL

Err:0x3 is ERROR_PATH_NOT_FOUND. Perhaps os.Mkdir(os.DevNull) should also return ERROR_PATH_NOT_FOUND. ERROR_PATH_NOT_FOUND is even mentioned in https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-createdirectoryw

I do not see downsides of changing os.Mkdir(os.DevNull) to return ERROR_PATH_NOT_FOUND.

I will do this change unless others object.

Alex

[gopherbot]

Change https://golang.org/cl/186139 mentions this issue: os: fix Mkdir to return error with 'NUL' on Windows

[iwdgo]

This fix aligns the behavior of Mkdir and MkdirAll when os.DevNull is the argument:

err = os.Mkdir(os.DevNull, 0777)
if err != nil {
    fmt.Printf("%q\n%#v\n", err, err)
}

The returned error message:

"mkdir NUL: The system cannot find the path specified."
&os.PathError{Op:"mkdir", Path:"NUL", Err:0x3}