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: Case-only renames not possible on case insensitive filesystems #35222

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

Comments

@AudriusButkevicius
Copy link
Contributor

@AudriusButkevicius AudriusButkevicius commented Oct 28, 2019

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

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 28, 2019

Your suggested solution sounds reasonable, with a test case.

@ianlancetaylor ianlancetaylor added this to the Backlog milestone Oct 28, 2019
@slrz

This comment has been minimized.

Copy link

@slrz 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

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 29, 2019

See #19647 for some background.

@nekr0z

This comment has been minimized.

Copy link

@nekr0z 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
Fixes golang#35222
AudriusButkevicius added a commit to AudriusButkevicius/go that referenced this issue Oct 31, 2019
Fixes golang#35222
AudriusButkevicius added a commit to AudriusButkevicius/go that referenced this issue Oct 31, 2019
Fixes golang#35222
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 31, 2019

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
Fixes golang#35222
@gopherbot gopherbot closed this in 4a09a9b Nov 1, 2019
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.

5 participants
You can’t perform that action at this time.