os: ReadFile on directories does not return EISDIR in windows

[dop251]

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

$ go version
go version 1.15.6 windows/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output

$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\vagrant\AppData\Local\go-build
set GOENV=C:\Users\vagrant\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\vagrant\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\vagrant\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=c:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Users\vagrant\go\wintest\go.mod
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\vagrant\AppData\Local\Temp\go-build505615858=/tmp/go-build -gno-record-gcc-switches

What did you do?

https://play.golang.org/p/GwvDhx3PqK-

What did you expect to see?

All good

What did you see instead?

Unexpected error: read /: The handle is invalid

[egonelbre]

Internally it ends up doing code that is equivalent to:

#include <windows.h>
#include <stdio.h>

void main() {
	WIN32_FIND_DATA data;
	HANDLE h = FindFirstFileA("\\", &data);

	char buf[100];
	BOOL r = ReadFile(h, &buf[0], 100, 0, 0);
	DWORD err = GetLastError();
	printf("%d\n", err);
}

Which does return the invalid handle error.

[dop251]

I believe it should be fixed. Opening a directory in read/write mode does produce the correct error because there is a special handling for it:

go/src/os/file_windows.go

Line 177 in 4d27c4c

r, errd := openDir(name)

What's missing is returning of the same error when a Read() is attempted on a directory file.

[gopherbot]

Change https://golang.org/cl/279852 mentions this issue: os: fix openFileNolog when open dir in windows

[gopherbot]

Change https://golang.org/cl/374394 mentions this issue: os: return EISDIR when ReadFile is attempted on a directory.

[alexbrainman]

I believe it should be fixed. Opening a directory in read/write mode does produce the correct error ...

@dop251 I am not convinced. Where is it documented that ioutil.ReadFile("/") should return syscall.EISDIR?

Thank you.

Alex

[dop251]

To clarify, it applies not just to ReadFile(), but to any Read(). I don't think it's explicitly documented (at least not in Go), however:

1. It makes it consistent with POSIX OSes thus improving the portability of Go source code.
2. syscall.EISDIR is already returned when you try to open a directory in read/write mode in Windows.
3. It's better than the current The handle is invalid error.

[alexbrainman]

1. It makes it consistent with POSIX OSes thus improving the portability of Go source code.

ioutil.ReadFile is not documented to return syscall.EISDIR. Go never tried implementing any POSIX semantics. So you should not assume anything above what is documented when porting your program from OS to OS.

2. syscall.EISDIR is already returned when you try to open a directory in read/write mode in Windows.

I don't see how that is relevant to ioutil.ReadFile behaviour. Each function has its own implementation.

3. It's better than the current The handle is invalid error.

The handle is invalid is ERROR_INVALID_HANDLE. ioutil.ReadFile is implemented by calling Windows API, that is why you see Windows API errors.

Anyway I am still unconvinced we should change current implementation just so we return syscall.EISDIR. But others might disagree with me.

Alex

[dop251]

The fact there is a cross-platform os package in Go already means there is some form of emulation and that the behavior should be as consistent as possible across platforms. In my view, anyway.

As for documentation, neither does it say that opening a directory in read/write mode should return syscall.EISDIR, and yet it does, and there is a specific code in file_windows.go for that. Moreover, syscall.EISDIR does not exist in Windows at all, it's been added as an application error in Go, so why not use it when appropriate? Adding it to the documentation shouldn't be too difficult.

In any case I agree it's not clear-cut and you can argue either way. The change is also breaking (if there is a Windows-specific code somewhere that expects ERROR_INVALID_HANDLE in this situation it will break). I still think this is a reasonable change. The question is who makes the decision?

[alexbrainman]

Adding it to the documentation shouldn't be too difficult.

Why would we do that? What is the benefit?

The change is also breaking

Indeed it does. And we would have to add more code and comments explaining the reason why the code is written that way. I don't think this is all worth to just please you.

I still think this is a reasonable change. The question is who makes the decision?

I don't know. Sorry.

Alex

[dop251]

The benefit is obvious: less platform-specific code required. For example take the original issue that was the reason for me to open this: dop251/goja_nodejs#21. As it stands now, I have to add some Windows-specific code to handle the situation. If Read() consistently returned the same error across all platforms, it would just work.