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: RemoveAll does not remove directories when ancestor directory lacks permissions #29983

Closed
neild opened this issue Jan 29, 2019 · 9 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@neild
Copy link
Contributor

neild commented Jan 29, 2019

This is a regression in Go 1.12 introduced by https://go-review.googlesource.com/c/137442 ("add support for long path names on unix RemoveAll").

mkdir -p a/1/x
mkdir -p a/2/x
chmod 0555 a

On Go 1.11, os.RemoveAll("a") removes a/1/x and a/2/x and returns "remove a/1: permission denied".
On Go 1.12, os.RemoveAll("a") returns "permission denied" and does not remove anything.

@bradfitz
Copy link
Contributor

/cc @ianlancetaylor

@bradfitz bradfitz added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Jan 29, 2019
@bradfitz bradfitz added this to the Go1.12 milestone Jan 29, 2019
@bradfitz
Copy link
Contributor

This might be acceptable and just need docs. The current docs in the release notes only says:

RemoveAll now supports paths longer than 4096 characters on most Unix systems.

And, whoa, we no longer have docs on RemoveAll:

https://tip.golang.org/pkg/os/#RemoveAll

That needs to be fixed too.

@neild
Copy link
Contributor Author

neild commented Jan 29, 2019

The docs for RemoveAll say:

// RemoveAll removes path and any children it contains.
// It removes everything it can but returns the first error
// it encounters. If the path does not exist, RemoveAll
// returns nil (no error).

So, not acceptable: RemoveAll no longer removes everything it can.

@neild
Copy link
Contributor Author

neild commented Jan 29, 2019

The new RemoveAll is also skipping the call to testlog.Open.

@neild
Copy link
Contributor Author

neild commented Jan 29, 2019

I'm also dubious about this distinction:

https://go.googlesource.com/go/+/master/src/os/removeall_at.go#132
https://go.googlesource.com/go/+/master/src/os/file_unix.go#207

The older RemoveAll uses os.Open to open directories, which includes a Darwin-specific retry loop around syscall.Open. The new RemoveAll uses unix.OpenAt with no retry loop. Perhaps that retry loop isn't necessary with OpenAt, but the lack of any commentary makes me wonder.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/160180 mentions this issue: os: treat EACCES as a permission error in RemoveAll

@ianlancetaylor
Copy link
Contributor

I believe that it's fine to skip the call to testlog.Open. It's only necessary to call testlog.Open if a change in the file contents affects whether a test should be cached. In this case it does not.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/160181 mentions this issue: os: make openFdAt act like openFileNolog

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/160182 mentions this issue: os: add docs for _at version of RemoveAll

gopherbot pushed a commit that referenced this issue Jan 30, 2019
- add EINTR loop on Darwin
- return PathError on error
- call newFile rather than NewFile

This tries to minimize the possibility of any future changes.
It would be nice to put openFdAt in the same file as openFileNolog,
but build tags forbid.

Updates #29983

Change-Id: I866002416d6473fbfd80ff6ef09b2bc4607f2934
Reviewed-on: https://go-review.googlesource.com/c/160181
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
gopherbot pushed a commit that referenced this issue Jan 30, 2019
Updates #29983

Change-Id: Ifdf8aa9c92e053374e301a4268d85e277c15f0b5
Reviewed-on: https://go-review.googlesource.com/c/160182
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
nebulabox pushed a commit to nebulabox/go that referenced this issue Feb 18, 2019
- add EINTR loop on Darwin
- return PathError on error
- call newFile rather than NewFile

This tries to minimize the possibility of any future changes.
It would be nice to put openFdAt in the same file as openFileNolog,
but build tags forbid.

Updates golang#29983

Change-Id: I866002416d6473fbfd80ff6ef09b2bc4607f2934
Reviewed-on: https://go-review.googlesource.com/c/160181
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
nebulabox pushed a commit to nebulabox/go that referenced this issue Feb 18, 2019
Updates golang#29983

Change-Id: Ifdf8aa9c92e053374e301a4268d85e277c15f0b5
Reviewed-on: https://go-review.googlesource.com/c/160182
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
nebulabox pushed a commit to nebulabox/go that referenced this issue Feb 18, 2019
Fixes golang#29983

Change-Id: I24077bde991e621c23d00973b2a77bb3a18e4ae7
Reviewed-on: https://go-review.googlesource.com/c/160180
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
nebulabox pushed a commit to nebulabox/go that referenced this issue Feb 20, 2019
- add EINTR loop on Darwin
- return PathError on error
- call newFile rather than NewFile

This tries to minimize the possibility of any future changes.
It would be nice to put openFdAt in the same file as openFileNolog,
but build tags forbid.

Updates golang#29983

Change-Id: I866002416d6473fbfd80ff6ef09b2bc4607f2934
Reviewed-on: https://go-review.googlesource.com/c/160181
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
nebulabox pushed a commit to nebulabox/go that referenced this issue Feb 20, 2019
Updates golang#29983

Change-Id: Ifdf8aa9c92e053374e301a4268d85e277c15f0b5
Reviewed-on: https://go-review.googlesource.com/c/160182
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
nebulabox pushed a commit to nebulabox/go that referenced this issue Feb 20, 2019
Fixes golang#29983

Change-Id: I24077bde991e621c23d00973b2a77bb3a18e4ae7
Reviewed-on: https://go-review.googlesource.com/c/160180
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Jan 30, 2020
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. release-blocker
Projects
None yet
Development

No branches or pull requests

4 participants