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
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows release-blocker
Milestone

Comments

@cmMcKeevek
Copy link

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 This label describes issues relating to any tools in the x/tools repository. 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 andybons added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 3, 2020
@andybons
Copy link
Member

andybons commented Apr 3, 2020

@zx2c4 @alexbrainman

@heschi
Copy link
Contributor

heschi 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 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 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().

@heschi
Copy link
Contributor

heschi 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

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

@heschi
Copy link
Contributor

heschi 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

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

@heschi
Copy link
Contributor

heschi 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

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

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

@alexbrainman
Copy link
Member

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

@heschi
Copy link
Contributor

heschi 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

@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

@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 This label describes issues relating to any tools in the x/tools repository. label May 19, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. 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 This label describes issues relating to any tools in the x/tools repository. label May 20, 2020
@ianlancetaylor
Copy link
Contributor

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

@gopherbot
Copy link

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

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

@xunxinyuan
Copy link

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

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

#39125

@gopherbot
Copy link

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>
@golang golang locked and limited conversation to collaborators May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows release-blocker
Projects
None yet
Development

No branches or pull requests

9 participants