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

os: opening an existing file with O_CREATE|O_TRUNC and permission 0 changes file to be read-only on Windows #38225

Closed
cmMcKeevek opened this issue Apr 2, 2020 · 20 comments

Comments

@cmMcKeevek
Copy link

@cmMcKeevek cmMcKeevek commented Apr 2, 2020

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

$ go version
go version go1.14.1 windows/amd64

Does this issue reproduce with the latest release?

As far as I can tell, 1.14.1 is the latest version, so 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\Kevin\AppData\Local\go-build
set GOENV=C:\Users\Kevin\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GONOPROXY=gitlab.com/CampMinder
set GONOSUMDB=gitlab.com/CampMinder
set GOOS=windows
set GOPATH=C:\Users\Kevin\go
set GOPRIVATE=gitlab.com/CampMinder
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=
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 -fmessage-length=0 -fdebug-prefix-map=C:\Users\Kevin\AppData\Local\Temp\go-build932902826=/tmp/go-build -gno-record-gcc-switches

What did you do?

package main

import (
	"log"
	"os"
	"os/exec"
)

func canWrite(filepath string) (bool, error) {
	file, err := os.OpenFile(filepath, os.O_WRONLY, 0666)
	if err != nil {
		if os.IsPermission(err) {
			return false, err
		}
	}
	file.Close()
	return true, nil
}

func main() {
	filename := "test.go"

	// Clean up previous run
	_, err := os.Stat(filename)
	if err == nil {
		os.Remove(filename)
	}

	// Create file
	f, err := os.Create(filename)
	if err != nil {
		log.Fatalf("error creating file, %s\n", err)
	}
	defer f.Close()

	// Check file was created without read-only
	result, err := canWrite(filename)
	if err != nil {
		log.Printf("error checking file %s\n", err)
	}
	log.Printf("Is %s writable? : [%v]\n", filename, result)

	// Populate test file with code that goimports will change
	_, err = f.WriteString(`
	package main

	func main() {
		fmt.Printf("%s", time.Now())
	}
	`)
	if err != nil {
		log.Fatal(err)
	}

	// Check that the file is still not read-only
	result, err = canWrite(filename)
	if err != nil {
		log.Printf("error checking file %s\n", err)
	}
	log.Printf("Is %s writable? : [%v]\n", filename, result)

	// Run goimports to change the file with `-w`
	cmd := exec.Command("goimports", "-w", filename)
	cmd.Stdout = os.Stdout
	cmd.Stderr = os.Stderr
	err = cmd.Run()
	if err != nil {
		log.Fatalf("goimports failed with %s\n", err)
	}

	// Check that the file is now, erroneously, read-only
	result, err = canWrite(filename)
	if err != nil {
		log.Printf("error checking file %s\n", err)
	}
	log.Printf("Is %s writable? : [%v]\n", filename, result)
}

What did you expect to see?

I expected that the file remain writable after goimports -w

What did you see instead?

goimports -w leaves the formatted file with the read-only flag set.

Bonus information

I suspect the problem is right here
https://github.com/golang/tools/blob/9fc00b0a7ff6189dbfbf7326432f0857c366fd6a/cmd/goimports/goimports.go#L163

and that it was caused by this
https://golang.org/doc/go1.14#windows

@gopherbot gopherbot added this to the Unreleased milestone Apr 2, 2020
@gopherbot gopherbot added the Tools label Apr 2, 2020
@cmMcKeevek cmMcKeevek changed the title x/tools/cmd/goimports: x/tools/cmd/goimports: -w leaves all edited files Read-Only Apr 3, 2020
@andybons andybons changed the title x/tools/cmd/goimports: -w leaves all edited files Read-Only x/tools/cmd/goimports: -w leaves all edited files Read-Only on Windows Apr 3, 2020
@andybons
Copy link
Member

@andybons andybons commented Apr 3, 2020

@heschik
Copy link
Contributor

@heschik heschik commented Apr 7, 2020

I think you're right; the file now has the R (read only) attribute. I think this might be a Go bug; the documentation for ioutil.WriteFile and io.CreateFile state, or at least strongly imply, that the file mask only matters when the file is created. Here, it already exists. So why are the attributes being changed?

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Apr 7, 2020

It's been a few months since I looked at that code, but last I looked at it, for non writable modes, instead of twiddling ACLs, the Go mapping is to instead set the readonly file attribute.

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Apr 7, 2020

Here it is:

if perm&S_IWRITE == 0 {
attrs = FILE_ATTRIBUTE_READONLY
}

It's then reflected in the other direction with .Mode().

@heschik
Copy link
Contributor

@heschik heschik commented Apr 8, 2020

Right, I understand that it's intention to set readonly under at least some circumstances. That's your https://golang.org/cl/202439. My question is whether it's intentional to apply them when opening an existing file, rather than creating a new one.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Apr 16, 2020

My question is whether it's intentional to apply them when opening an existing file, rather than creating a new one.

I did not know ioutil.WriteFile should use perm parameter differently if it is applied to a new or existing file. Where does it say that?

Alex

@heschik
Copy link
Contributor

@heschik heschik commented Apr 17, 2020

From the WriteFile docs: "If the file does not exist, WriteFile creates it with permissions perm (before umask); otherwise WriteFile truncates it before writing." That would appear to me to say that perm is only used when creating the file.

From OpenFile: "If the file does not exist, and the O_CREATE flag is passed, it is created with mode perm (before umask)." Again, perm is not mentioned as used except during file creation.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Apr 19, 2020

From the WriteFile docs: "If the file does not exist, WriteFile creates it with permissions perm (before umask); otherwise WriteFile truncates it before writing." That would appear to me to say that perm is only used when creating the file.

I disagree. That is not how I read this documentation. But I won't argue with you.

Anyway, I don't see how we can implement what you require.

Looking at windows version of syscall.Open. We should use createmode to determine, if FILE_ATTRIBUTE_READONLY should be included.

We should include FILE_ATTRIBUTE_READONLY, if createmode is CREATE_NEW.

I don't see how to deal with CREATE_ALWAYS. CREATE_ALWAYS deals with both exiting and non-existing files. We want FILE_ATTRIBUTE_READONLY set for new files, but not for existing files.

Here is CreateFile Windows API documentation.

https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew

Alex

@heschik
Copy link
Contributor

@heschik heschik commented Apr 20, 2020

@ianlancetaylor touched the doc last, so I'm curious if he has an opinion. But anyway, fine; I'll change goimports to stat the file and use its permissions for the WriteFile call.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 20, 2020

I think that for ioutil.WriteFile and for os.OpenFile the permission argument should only be used when the file is newly created. We should not change the permission of an existing file. I think that is the intent of the documentation and I think it's how those functions work on Unix.

Therefore, for O_CREAT | O_TRUNC without O_EXCL, I think that syscall.Open should first call GetFileInformationByHandle, and, if the file exists, preserve FILE_ATTRIBUTE_READONLY.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 21, 2020

Change https://golang.org/cl/229297 mentions this issue: cmd/goimports: set correct permissions on Windows

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Apr 22, 2020

I think that for ioutil.WriteFile and for os.OpenFile the permission argument should only be used when the file is newly created. We should not change the permission of an existing file. I think that is the intent of the documentation and I think it's how those functions work on Unix.

I understand what we are trying to achieve. @heschik already explained it to me.

Therefore, for O_CREAT | O_TRUNC without O_EXCL, I think that syscall.Open should first call GetFileInformationByHandle, and, if the file exists, preserve FILE_ATTRIBUTE_READONLY.

GetFileInformationByHandle requires file handle, so you will need to use CreateFile first, then GetFileInformationByHandle, then CloseHandle, then you will call existing CreateFile. That is 4 calls, instead of 1 for every single opened file.

And what about the race between GetFileInformationByHandle and second CreateFile? What happens, if another program or thread changes file attribute?

I don't think having FILE_ATTRIBUTE_READONLY mapped to UNIX attribute is worth the trouble. Maybe we should just revert https://golang.org/cl/202439 ?

@zx2c4 what do you think?

Change https://golang.org/cl/229297 mentions this issue: cmd/goimports: set correct permissions on Windows

@heschik Thank you for preparing this CL. But, lets see, if we can find general solution.

Alex

@heschik
Copy link
Contributor

@heschik heschik commented Apr 22, 2020

Since this behavior was released in 1.14, we need a fix in goimports regardless of future behavior. I changed the CL to update this bug rather than close it.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 23, 2020

@alexbrainman I don't think it's too bad. We only need to check the existing file attribute in the case of O_CREAT | O_TRUNC and perm&S_IWRITE == 0. In other words, we never need to check the attribute for os.Open or os.Create, only for some cases of os.OpenFile.

And actually I think it's even simpler. In the problematic case, we should call CreateFile with FILE_ATTRIBUTES_NORMAL and OPEN_EXISTING. If that succeeds, we are done. We don't actually need to call GetFileInformationByHandle; I was wrong about that. If the first CreateFile fails, then we try again with FILE_ATTRIBUTE_READONLY and CREATE_ALWAYS.

So I think this just adds one extra system call for an unusual case.

gopherbot pushed a commit to golang/tools that referenced this issue Apr 23, 2020
As of Go 1.14, WriteFile on Windows will set read-only on existing files
if you pass 0 for perms. Pass the pre-existing permissions.

Updates golang/go#38225.

Change-Id: I3174469efd4dc4c7eacc8522386a0712cfa39d11
Reviewed-on: https://go-review.googlesource.com/c/tools/+/229297
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented May 19, 2020

@andybons this issue is about broken os package. It should not be labeled as tools. And we should fix the issue before releasing go 1.15, because we broke this code in go 1.14.

Thank you.

Alex

@bcmills bcmills modified the milestones: Unreleased, Go1.15 May 19, 2020
@andybons andybons removed the Tools label May 19, 2020
@gopherbot gopherbot added the Tools label May 19, 2020
@ianlancetaylor ianlancetaylor changed the title x/tools/cmd/goimports: -w leaves all edited files Read-Only on Windows os: opening an existing file with O_CREATE|O_TRUNC and permission 0 changes file to be read-only on Windows May 20, 2020
@ianlancetaylor ianlancetaylor removed the Tools label May 20, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 20, 2020

@gopherbot Please open a backport to 1.14. This is a regression that was introduced in 1.14.

@gopherbot
Copy link

@gopherbot gopherbot commented May 20, 2020

Backport issue(s) opened: #39158 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot
Copy link

@gopherbot gopherbot commented May 20, 2020

Change https://golang.org/cl/234534 mentions this issue: syscall: preserve Windows file permissions for O_CREAT|O_TRUNC

@xunxinyuan
Copy link

@xunxinyuan xunxinyuan commented May 20, 2020

ioutil.WriteFile("D:/test.txt", []byte("test data"), os.ModeAppend)

The above code automatically sets the test.txt file to read only

#39125

@gopherbot gopherbot closed this in 567556d May 20, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented May 20, 2020

Change https://golang.org/cl/234686 mentions this issue: [release-branch.go1.14] syscall: preserve Windows file permissions for O_CREAT|O_TRUNC

gopherbot pushed a commit that referenced this issue May 23, 2020
…r O_CREAT|O_TRUNC

On Windows, calling syscall.Open(file, O_CREAT|O_TRUNC, 0) for a file
that already exists would change the file to be read-only.
That is not how the Unix syscall.Open behaves, so avoid it on
Windows by calling CreateFile twice if necessary.

For #38225
Fixes #39158

Change-Id: I70097fca8863df427cc8a97b9376a9ffc69c6318
Reviewed-on: https://go-review.googlesource.com/c/go/+/234534
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
(cherry picked from commit 567556d)
Reviewed-on: https://go-review.googlesource.com/c/go/+/234686
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.