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: permit Rename to read-only file on Windows #38287

Open
calmh opened this issue Apr 7, 2020 · 9 comments
Open

os: permit Rename to read-only file on Windows #38287

calmh opened this issue Apr 7, 2020 · 9 comments

Comments

@calmh
Copy link
Contributor

@calmh calmh commented Apr 7, 2020

On Windows 10, in Go 1.13, it used to be possible to rename over a read only file (like it is in Unix). In Go 1.14 it is not. I'm not seeing anything specific about this in the release notes, was this an intentional change?

func TestRenameRO(t *testing.T) {
	os.Remove("a")
	os.Remove("b")
	defer os.Remove("a")
	defer os.Remove("b")

	if err := ioutil.WriteFile("a", []byte("some data"), 0644); err != nil { // rw
		t.Fatal(err)
	}
	if err := ioutil.WriteFile("b", []byte("some data"), 0444); err != nil { // ro
		t.Fatal(err)
	}
	if err := os.Rename("a", "b"); err != nil {
		t.Error(err)
	}
}
PS C:\Users\JakobBorg\dev\test> go1.13.9 test
PASS
ok      _/C_/Users/JakobBorg/dev/test   0.048s
PS C:\Users\JakobBorg\dev\test> go1.14.1 test
--- FAIL: TestRenameRO (0.01s)
    rename_test.go:22: rename a b: Access is denied.
FAIL
exit status 1
FAIL    _/C_/Users/JakobBorg/dev/test   0.046s
PS C:\Users\JakobBorg\dev\test> 
@mattn
Copy link
Member

@mattn mattn commented Apr 7, 2020

This probably seems to be side effect in 16f0f9c but I think this is not regression.

syscall: respect permission bits on file opening on Windows

On Windows, os.Chmod and syscall.Chmod toggle the FILE_ATTRIBUTES_
READONLY flag depending on the permission bits. That's a bit odd but I
guess some compromises were made at some point and this is what was
chosen to map to a Unix concept that Windows doesn't really have in the
same way. That's fine. However, the logic used in Chmod was forgotten
from os.Open and syscall.Open, which then manifested itself in various
places, most recently, go modules' read-only behavior.

This makes syscall.Open consistent with syscall.Chmod and adds a test
for the permission behavior using ioutil. By testing the behavior
instead of explicitly testing for the attribute bits we care about, we
make sure this doesn't regress in unforeseen ways in the future, as well
as ensuring the test works on platforms other than Windows.

In the process, we fix some tests that never worked and relied on broken
behavior, as well as tests that were disabled on Windows due to the
broken behavior and had TODO notes.

Fixes #35033

@mattn
Copy link
Member

@mattn mattn commented Apr 7, 2020

Before go1.13 (before 16f0f9c), ioutil.WriteFile does not respect permission bits on Windows. So the file was always 0666 eventhough if 0444 was specified. This is related on #26295

@mattn
Copy link
Member

@mattn mattn commented Apr 7, 2020

I confirmed below fix this.

diff --git a/src/os/file_windows.go b/src/os/file_windows.go
index 96f934d039..581d382f75 100644
--- a/src/os/file_windows.go
+++ b/src/os/file_windows.go
@@ -313,10 +313,29 @@ func Remove(name string) error {
 
 func rename(oldname, newname string) error {
 	e := windows.Rename(fixLongPath(oldname), fixLongPath(newname))
-	if e != nil {
-		return &LinkError{"rename", oldname, newname, e}
+	if e == nil {
+		return nil
 	}
-	return nil
+
+	p, e1 := syscall.UTF16PtrFromString(fixLongPath(newname))
+	if e1 != nil {
+		return e1
+	}
+
+	a, e1 := syscall.GetFileAttributes(p)
+	if e1 != nil {
+		e = e1
+	} else {
+		if a&syscall.FILE_ATTRIBUTE_READONLY != 0 {
+			if e1 = syscall.SetFileAttributes(p, a&^syscall.FILE_ATTRIBUTE_READONLY); e1 == nil {
+				if e = windows.Rename(fixLongPath(oldname), fixLongPath(newname)); e == nil {
+					return nil
+				}
+			}
+		}
+	}
+
+	return &LinkError{"rename", oldname, newname, e}
 }
 
 // Pipe returns a connected pair of Files; reads from r return bytes written to w.
@calmh
Copy link
Contributor Author

@calmh calmh commented Apr 7, 2020

Yeah, the bug is in WriteFile, not Rename. Ugh.

@calmh calmh closed this Apr 7, 2020
@mattn
Copy link
Member

@mattn mattn commented Apr 7, 2020

Ah, I'm sorry of my short explaining. I'm thinking 16f0f9c was not regression but os.Rename can be improved like os.Remove.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 7, 2020

Change https://golang.org/cl/227457 mentions this issue: os: fix Rename for file with read only attribute on Windows

@ianlancetaylor ianlancetaylor reopened this Apr 7, 2020
@ianlancetaylor ianlancetaylor changed the title os: Regression in Rename over read only file on Windows on 1.14 os: permit Rename to read-only file on Windows Apr 7, 2020
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Apr 7, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 7, 2020

On Unix, rename to a read-only file is permitted. The directory must be writable.

So the question is whether os.Rename on Windows should make the effort to make this work the same way, by changing the file to not be read-only. I guess we often try to do that on Windows, so it makes sense to me, but I don't feel strongly about it.

Any other opinions?

CC @alexbrainman @mattn

@mattn
Copy link
Member

@mattn mattn commented Apr 8, 2020

I agree with iant.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Apr 11, 2020

Any other opinions?

This bug does not affect me. But the fix will affect me - os.Rename will have to do more to implement this logic. We always adjust windows code to match whatever Unix syscalls implements.

Adding @zx2c4 since he is the author of 16f0f9c

Thank you.

Alex

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
5 participants
You can’t perform that action at this time.