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

os: Case-only renames not possible on case insensitive filesystems #35222

Closed
AudriusButkevicius opened this issue Oct 28, 2019 · 5 comments
Closed

os: Case-only renames not possible on case insensitive filesystems #35222

AudriusButkevicius opened this issue Oct 28, 2019 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@AudriusButkevicius
Copy link
Contributor

AudriusButkevicius commented Oct 28, 2019 •
edited

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

tip

Does this issue reproduce with the latest release?

yes

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

darwin/amd64 but affects whole of unix with case insensitive filesystems

What did you do?

Case-only directory renames on case-insensitive filesystems on unix are not possible, due to destination existence check as per:

go/src/os/file_unix.go

Lines 23 to 36 in 59a6847

fi, err := Lstat(newname)
if err == nil && fi.IsDir() {
// There are two independent errors this function can return:
// one for a bad oldname, and one for a bad newname.
// At this point we've determined the newname is bad.
// But just in case oldname is also bad, prioritize returning
// the oldname error because that's what we did historically.
if _, err := Lstat(oldname); err != nil {
if pe, ok := err.(*PathError); ok {
err = pe.Err
}
return &LinkError{"rename", oldname, newname, err}
}
return &LinkError{"rename", oldname, newname, syscall.EEXIST}

Please note, it seems this only affects directories.

The two different stat calls for differently cased directories will return stat information about the same object in the filesystem, which as a result fails that check, yet the move syscall would not be destructive in this case.

Sadly you cannot compare the names returned by the Stat calls, as the name in the returned struct is set to match what the caller provided.

A possible solution to this would be to check that the names are not the same, and if they are not, verify that the rename would not be a destructive action using os.SameFile.

If that returns true, proceed with the syscall.

If you think this is an acceptable solution, I am happy to submit a CL.

What did you expect to see?

Successful rename (same way like mv command succeeds)

What did you see instead?

syscall.EEXIST error
@AudriusButkevicius AudriusButkevicius mentioned this issue Oct 28, 2019
Merged
@ianlancetaylor
Copy link
Contributor

Your suggested solution sounds reasonable, with a test case.

@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 28, 2019
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Oct 28, 2019
@slrz
Copy link

slrz commented Oct 29, 2019

The same check also prevents renaming over an empty directory which the actual system call would allow. Given the wording of the os.Rename docs, this could be intentional. Not sure it's a good thing, as this issue clearly demonstrates. Piling on yet more racy checks to work around the unintended side effects seems contraproductive.

What about removing the newpath existence check in os and just letting the OS handle it?

@ianlancetaylor
Copy link
Contributor

See #19647 for some background.

@nekr0z
Copy link

nekr0z commented Oct 31, 2019

May I add that there's currently behaviour inconsistency: the same rename on a case-insensitive filesystem in Windows will work flawlessly and produce no error.

AudriusButkevicius added a commit to AudriusButkevicius/go that referenced this issue Oct 31, 2019
@AudriusButkevicius
os: allow case only renames on case-insensitive filesystems
39a966e 
Fixes golang#35222
AudriusButkevicius added a commit to AudriusButkevicius/go that referenced this issue Oct 31, 2019
@AudriusButkevicius
os: allow case only renames on case-insensitive filesystems
c390325 
Fixes golang#35222
AudriusButkevicius added a commit to AudriusButkevicius/go that referenced this issue Oct 31, 2019
@AudriusButkevicius
os: allow case only renames on case-insensitive filesystems
3a935d5 
Fixes golang#35222
@AudriusButkevicius AudriusButkevicius mentioned this issue Oct 31, 2019
Closed
@gopherbot
Copy link

Change https://golang.org/cl/204601 mentions this issue: os: allow case only renames on case-insensitive filesystems

AudriusButkevicius added a commit to AudriusButkevicius/go that referenced this issue Oct 31, 2019
@AudriusButkevicius
os: allow case only renames on case-insensitive filesystems
954a016 
Fixes golang#35222
@golang golang locked and limited conversation to collaborators Oct 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

os: allow case only renames on case-insensitive filesystems AudriusButkevicius/go
5 participants
@AudriusButkevicius @nekr0z @ianlancetaylor @slrz @gopherbot