Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/sys/windows: wrong value of S_IFMT mask on Windows/plan9 #32270

Open
cyrilcourvoisier opened this issue May 27, 2019 · 8 comments
Open

x/sys/windows: wrong value of S_IFMT mask on Windows/plan9 #32270

cyrilcourvoisier opened this issue May 27, 2019 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone
Unreleased

Comments

@cyrilcourvoisier
Copy link

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

$ go version
go version go1.12.5 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 GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Cyco\AppData\Local\go-build
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\projects\go
set GOPROXY=
set GORACE=
set GOROOT=C:\Go
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
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\Cyco\AppData\Local\Temp\go-build338162002=/tmp/go-build -gno-record-gcc-switches

It seems that value of syscall.S_IFMT mask is wrong on Windows/Plan9. It is defined as 0x1f000 while it is defined as 0xf000 on other platforms.

I had previously entered an issue on the sftp package page:
pkg/sftp#291

The code is testing the result of an SSH_FXP_STAT request on an AIX server. It tests the filemode bits with syscall.S_IFMT mask. The results should be S_IFDIR, but the mask tests an extra bit that is used as S_IFJOURNAL on AIX, hence failing to get the correst value.

input filemode bits are                  1 0100 0011 1111 1111‬
syscall.S_IFMT mask bits are             1 1111 0000 0000 0000

The different values tested are:

 S_IFBLK                                 0 0110 0000 0000 0000
 S_IFCHR                                 0 0010 0000 0000 0000
 S_IFDIR                                 0 0100 0000 0000 0000
 S_IFIFO                                 0 0001 0000 0000 0000
 S_IFLNK                                 0 1010 0000 0000 0000
 S_IFREG                                 0 1000 0000 0000 0000
 S_IFSOCK                                0 1100 0000 0000 0000

As you can see, none of the values match the input, and the leftmost bit is not used by any value.
The input would match S_IFDIR if the S_IFMT mask didn't test the leftmost bit.
@puellanivis
Copy link

As I noted in the issue over on pkg/sftp both of the values for S_IFMT contain:

	// Invented values to support what package os expects.

So, it would seem likely that they can be fixed with no issues.

@julieqiu julieqiu changed the title Wrong value of S_IFMT mask on Windows/plan9 x/sys/windows: wrong value of S_IFMT mask on Windows/plan9 May 28, 2019
@gopherbot gopherbot added this to the Unreleased milestone May 28, 2019
@julieqiu
Copy link
Member

/cc @alexbrainman @bradfitz

@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 28, 2019
@alexbrainman
Copy link
Member

As I noted in the issue over on pkg/sftp both of the values for S_IFMT contain:

	// Invented values to support what package os expects.

So, it would seem likely that they can be fixed with no issues.

S_IFMT is meaningless on Windows (there is no such constant anywhere in Windows API).

I suspect windows version of syscall.S_IFMT was added when windows port of Go was created. We, probably, could not compiled os package because it was referring to some Unix names - including S_IFMT. And we just copied these consts from another OS source file, like from syscall_linux.go. Just to make things build. Later on we added windows specific code in os package, but syscall.S_IFMT was left behind as is.

I think no one should be using syscall.S_IFMT on windows, because it has no meaning.

I doubt we could change syscall.S_IFMT value, because of https://golang.org/doc/go1compat

Leaving for others to decide what to do here.

Alex

@puellanivis
Copy link

RE: changing the syscall.S_IFMT for Windows:

The syscall package is therefore outside the purview of the guarantees made here.

The os.FileMode mimics POSIX file permissions on all operating systems. S_IFMT does show up in at least some windows documentation, https://docs.microsoft.com/en-us/cpp/c-runtime-library/stat-structure-st-mode-field-constants?view=vs-2019 which makes sense since it would be expected by C apps to have some sort of fstat available, for at least minimal POSIX support (which Windows does claim).

For plan9, it appears as well, for instance in NFS: https://github.com/cao-xx/plan-9/blob/b27a35643e4ee0ede5ed91ecb3a4ed98f8164454/sys/src/cmd/9nfs/nfs.h#L39 with the value of 0170000 which is 0xF000.

@alexbrainman
Copy link
Member

The syscall package is therefore outside the purview of the guarantees made here.

This only apply to changes by the OS. Otherwise syscall package is frozen.

S_IFMT does show up in at least some windows documentation, https://docs.microsoft.com/en-us/cpp/c-runtime-library/stat-structure-st-mode-field-constants?view=vs-2019 which makes sense since it would be expected by C apps to have some sort of fstat available,

That does not apply to Go. fstat is only available, if you use Microsoft C compiler.

Alex

@puellanivis
Copy link

puellanivis commented May 31, 2019 •
edited

Additionally, from the golang documentation:

Specification errors. If it becomes necessary to address an inconsistency or incompleteness in the specification, resolving the issue could affect the meaning or legality of existing programs. We reserve the right to address such issues, including updating the implementations. Except for security issues, no incompatible changes to the specification would be made.

That does not apply to Go.

The purpose of those resources it to point out that anytime these two OSes use S_IFMT, it is set to the value 0xF000. And neither of OSes declare a value for the other S_IF* values in syscall which use the 0x10000 bit. It will break zero code to make this change.

@puellanivis
Copy link

Let me rephrase my argument. On both plan9 and windows:
fmt.Printf("%#x", syscall.S_IFIFO | syscall.S_IFCHR | syscall.S_IFDIR | syscall.S_IFBLK | syscall.S_IFREG | syscall.S_IFLNK | syscall.S_IFSOCK) will print 0xf000. The value of S_IFMT is clearly inconsistent (too wide) with all of the values that it bounds.

It would be so much simpler in this argument, if POSIX.1-2017 had defined numeric values for these the same as they did for the various permission bits. "... in recognition that nearly 20 years after the first publication of this standard..." nearly every POSIX implementation agrees on what these values should be. (This project being apparently the sole outlier.) And that many network standards depend upon the interoperability of these values.

@alexbrainman
Copy link
Member

Let me rephrase my argument.

@puellanivis I don't make decisions here. You don't need to convince me. Go ahead and send a change https://golang.org/doc/contribute.html and see what happens.

Alex

@Castaglia Castaglia mentioned this issue Sep 9, 2020
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
Go Compiler / Runtime
Status: Triage Backlog
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@bradfitz @julieqiu @gopherbot @alexbrainman @puellanivis @cyrilcourvoisier