-
Notifications
You must be signed in to change notification settings - Fork 18k
os: add support for long path names on unix RemoveAll #28494
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: add support for long path names on unix RemoveAll #28494
Conversation
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
Consent verified earlier in #27871 |
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) |
This PR (HEAD: 240cc3d) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/146020 to see it. Tip: You can toggle comments from me using the |
Message from Gerrit User 5340: Patch Set 1: Run-TryBot+1 Code-Review+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5976: Patch Set 1: TryBots beginning. Status page: https://farmer.golang.org/try?commit=b8300245 Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5976: Patch Set 1: Build is still in progress... Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
240cc3d
to
f1fe8cc
Compare
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
This PR (HEAD: f1fe8cc) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/146020 to see it. Tip: You can toggle comments from me using the |
f1fe8cc
to
f267de5
Compare
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) |
Message from Gerrit User 5976: Patch Set 1: TryBot-Result-1 1 of 19 TryBots failed: Consult https://build.golang.org/ to see whether they are new failures. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 29433: Patch Set 2: Forgot to skip long path test for freebsd, this patch should be good. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 19560: Patch Set 2: Run-TryBot+1 (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5976: Patch Set 2: TryBots beginning. Status page: https://farmer.golang.org/try?commit=897dc2b8 Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5976: Patch Set 2: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 29433: Patch Set 2: (1 comment) Remove at_sysnum_freebsd.go? Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
This PR (HEAD: f267de5) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/146020 to see it. Tip: You can toggle comments from me using the |
Message from Gerrit User 12446: Uploaded patch set 3: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5206: Patch Set 3: (1 comment) According to https://build.golang.org, the earlier patch failed on Darwin. We should use the old non-at code on Darwin for now, unless you have reason to believe that the problem is fixed. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 29433: Patch Set 3:
I have changed the Darwin syscall number to be the fstatat64 version, which works on my Darwin machine and in the x/says package from the looks of it, so I think we are ok Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
On unix systems, long enough path names will fail when performing syscalls like `Lstat`. The current RemoveAll uses several of these syscalls, and so will fail for long paths. This can be risky, as it can let users "hide" files from the system or otherwise make long enough paths for programs to fail. By using `Unlinkat` and `Openat` syscalls instead, RemoveAll is safer on unix systems. Initially implemented for linux, darwin and openbsd. Not yet implemented on freebsd due to fstatat 64-bit inode compatibility issues. Fixes golang#27029 Co-authored-by: Giuseppe Capizzi <[email protected]> Co-authored-by: Julia Nedialkova <[email protected]>
This PR (HEAD: 9235489) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/146020 to see it. Tip: You can toggle comments from me using the |
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) |
Message from Gerrit User 29433: Patch Set 3: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 12446: Uploaded patch set 4: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5206: Patch Set 4: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 29433: Patch Set 4:
The commit message has changed: https://github.com/golang/go/pull/28494/commits Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5206: Patch Set 5: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5206: Patch Set 5: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5976: Patch Set 5: TryBots beginning. Status page: https://farmer.golang.org/try?commit=77363085 Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 12446: Uploaded patch set 6: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5976: Patch Set 5: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5206: Patch Set 6: +andybons help: how do we fix the commit message? Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5065: Patch Set 6:
See the bottom of https://github.com/golang/go/wiki/CommitMessage#github-pull-requests Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 12446: Uploaded patch set 7: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 19560: Patch Set 7: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 12446: Uploaded patch set 8: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 12446: Uploaded patch set 9: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 29433: Patch Set 9: (1 comment)
Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 19560: Patch Set 9: Run-TryBot+1 Thanks Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5976: Patch Set 9: TryBots beginning. Status page: https://farmer.golang.org/try?commit=aac8f9ab Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 5976: Patch Set 9: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
Message from Gerrit User 19560: Patch Set 9: Code-Review+1 LGTM. Leaving to Ian or Brad for final review. Please don’t reply on this GitHub thread. Visit golang.org/cl/146020. |
On unix systems, long enough path names will fail when performing syscalls like `Lstat`. The current RemoveAll uses several of these syscalls, and so will fail for long paths. This can be risky, as it can let users "hide" files from the system or otherwise make long enough paths for programs to fail. By using `Unlinkat` and `Openat` syscalls instead, RemoveAll is safer on unix systems. Initially implemented for linux, darwin, dragonfly, netbsd and openbsd. Not yet implemented on freebsd due to fstatat 64-bit inode compatibility issues. Fixes #27029 Co-authored-by: Giuseppe Capizzi <[email protected]> Co-authored-by: Julia Nedialkova <[email protected]> Change-Id: I978a6a4986878fe076d3c7af86e7927675624a96 GitHub-Last-Rev: 9235489 GitHub-Pull-Request: #28494 Reviewed-on: https://go-review.googlesource.com/c/146020 Run-TryBot: Tobias Klauser <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Tobias Klauser <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
This PR is being closed because golang.org/cl/146020 has been merged. |
On unix systems, long enough path names will fail when performing syscalls
like
Lstat
. The current RemoveAll uses several of these syscalls, and sowill fail for long paths. This can be risky, as it can let users "hide"
files from the system or otherwise make long enough paths for programs
to fail. By using
Unlinkat
andOpenat
syscalls instead, RemoveAll issafer on unix systems. Initially implemented for linux, darwin, dragonfly,
netbsd and openbsd. Not yet implemented on freebsd due to fstatat 64-bit
inode compatibility issues.
Fixes #27029
Co-authored-by: Giuseppe Capizzi [email protected]
Co-authored-by: Julia Nedialkova [email protected]