os: add support for long path names on unix RemoveAll #27871

ostenbom · 2018-09-26T08:49:00Z

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 several bsds.

Fixes #27029

Co-authored-by: Giuseppe Capizzi gcapizzi@pivotal.io
Co-authored-by: Julia Nedialkova yulia.nedyalkova@sap.com

gopherbot · 2018-09-26T08:58:52Z

This PR (HEAD: 3043fe3) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/137442 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-09-26T08:59:38Z

Message from Gerrit User 5976:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T09:05:58Z

Message from Gerrit User 12446:

Uploaded patch set 2: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T09:05:58Z

Message from Gerrit User 24096:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T09:18:15Z

This PR (HEAD: e12cec7) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/137442 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-09-26T09:34:13Z

Message from Gerrit User 19560:

Patch Set 3:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T09:41:38Z

Message from Gerrit User 28776:

Patch Set 3:

(2 comments)

Patch Set 3:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T10:17:13Z

This PR (HEAD: df255c8) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/137442 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-09-26T10:19:46Z

Message from Gerrit User 28776:

Patch Set 4:

(1 comment)

Fixed to use existing syscall package constants. AT_REMOVEDIR was actually _AT_REMOVEDIR, so had to keep the 0x200 constant.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T11:30:07Z

Message from Gerrit User 19560:

Patch Set 4: Run-TryBot+1

Patch Set 4:

(1 comment)

Fixed to use existing syscall package constants. AT_REMOVEDIR was actually _AT_REMOVEDIR, so had to keep the 0x200 constant.

Thanks

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T11:30:08Z

Message from Gerrit User 5976:

Patch Set 4:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=d0a82701

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T11:35:20Z

Message from Gerrit User 5976:

Patch Set 4:

Build is still in progress...
This change failed on freebsd-amd64-11_1:
See https://storage.googleapis.com/go-build-log/d0a82701/freebsd-amd64-11_1_6e850e38.log

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/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T11:37:57Z

Message from Gerrit User 5976:

Patch Set 4: TryBot-Result-1

2 of 19 TryBots failed:
Failed on freebsd-amd64-11_1: https://storage.googleapis.com/go-build-log/d0a82701/freebsd-amd64-11_1_6e850e38.log
Failed on openbsd-amd64-62: https://storage.googleapis.com/go-build-log/d0a82701/openbsd-amd64-62_8ae02b2c.log

Consult https://build.golang.org/ to see whether they are new failures.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T11:52:09Z

Message from Gerrit User 28776:

Patch Set 4:

Need some advice on removing "." and "..".

The reason for the freebsd/openbsd failures is because we added a TestRemoveAllDot test, testing for the removal of "." and ".." (usually not allowed on unix), which expects an error.

Do you agree with this reasoning? If so, we should just skip these tests on !linux,!darwin until it's ported.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T12:06:09Z

This PR (HEAD: f616868) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/137442 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-09-26T12:08:18Z

Message from Gerrit User 28776:

Patch Set 5:

Fixed by implementing for freebsd & openbsd

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T12:13:43Z

Message from Gerrit User 5889:

Patch Set 5:

Patch Set 5:

Fixed by implementing for freebsd & openbsd

Were you going to enable it for them?

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T12:22:56Z

This PR (HEAD: 3cafab3) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/137442 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-09-26T12:24:29Z

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/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T12:25:11Z

Message from Gerrit User 28776:

Patch Set 6:

Patch Set 5:

Patch Set 5:

Fixed by implementing for freebsd & openbsd

Were you going to enable it for them?

Yep, always forgetting build flags. Have enabled for them now.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T12:25:52Z

Message from Gerrit User 19560:

Patch Set 7:

Patch Set 5:

Fixed by implementing for freebsd & openbsd

I think you can also cover netbsd and dragonfly in the same way. We don't have TryBots for these but I guess TestRemoveAllDot would still fail later on the builders.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T13:25:51Z

This PR (HEAD: 9ab706c) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/137442 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-09-26T13:29:20Z

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/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-09-26T13:35:05Z

Message from Gerrit User 28776:

Patch Set 8:

Patch Set 7:

Patch Set 5:

Fixed by implementing for freebsd & openbsd

I think you can also cover netbsd and dragonfly in the same way. We don't have TryBots for these but I guess TestRemoveAllDot would still fail later on the builders.

Yes you're right. Should be done for most unix systems now - maybe time for another trybot?

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

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, freebsd and openbsd. Fixes golang#27029 Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io> Co-authored-by: Julia Nedialkova <yulia.nedyalkova@sap.com>

googlebot · 2018-10-30T00:40:02Z

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.)

gopherbot · 2018-10-30T00:40:15Z

Message from Gerrit User 29433:

Patch Set 19:

Patch Set 19:

(1 comment)

Cleaned up the uintptr consts and used syscall.SYS_FSTATAT(64) references on supported architectures.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T00:40:16Z

Message from Gerrit User 29433:

Patch Set 20:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T00:40:17Z

Message from Gerrit User 19560:

Patch Set 20:

(4 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T00:40:17Z

Message from Gerrit User 29433:

Patch Set 20:

(1 comment)

Ah, now I see the (new)fstatat(64) overlap, thanks Tobias. I managed to reduce all of the sysnum_linux_* files down to 3 using the 3 different calls. Again should come through when a googler thumbs up on the github PR.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T00:40:18Z

Message from Gerrit User 19560:

Patch Set 20:

Patch Set 20:

(1 comment)

Ah, now I see the (new)fstatat(64) overlap, thanks Tobias. I managed to reduce all of the sysnum_linux_* files down to 3 using the 3 different calls. Again should come through when a googler thumbs up on the github PR.

Nice, thanks a lot for working on these issues.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T00:40:28Z

This PR (HEAD: b30a621) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/137442 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-10-30T06:18:14Z

Message from Gerrit User 19560:

Patch Set 21: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T06:18:39Z

Message from Gerrit User 5976:

Patch Set 21:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=907b827e

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T06:23:13Z

Message from Gerrit User 5976:

Patch Set 21:

Build is still in progress...
This change failed on windows-amd64-2016:
See https://storage.googleapis.com/go-build-log/907b827e/windows-amd64-2016_a63ee959.log

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/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T06:26:09Z

Message from Gerrit User 5976:

Patch Set 21: TryBot-Result-1

1 of 19 TryBots failed:
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/907b827e/windows-amd64-2016_a63ee959.log

Consult https://build.golang.org/ to see whether they are new failures.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T07:39:15Z

Message from Gerrit User 19560:

Patch Set 21:

Patch Set 21: TryBot-Result-1

1 of 19 TryBots failed:
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/907b827e/windows-amd64-2016_a63ee959.log

Consult https://build.golang.org/ to see whether they are new failures.

TryBot failure looks unrelated:

--- FAIL: TestReadSubKeyNames (0.00s)
registry_test.go:39: Access is denied.
FAIL
FAIL internal/syscall/windows/registry 0.151s

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T14:58:10Z

Message from Gerrit User 5206:

Patch Set 21: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T15:05:28Z

Message from Gerrit User 5206:

Patch Set 21: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T15:09:42Z

Message from Gerrit User 5206:

Patch Set 22: Patch Set 21 was rebased

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T15:09:43Z

Message from Gerrit User 5206:

Patch Set 22: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T15:09:44Z

Message from Gerrit User 5976:

Patch Set 22:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=8853b41f

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T15:11:22Z

Message from Gerrit User 5976:

Patch Set 21: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T15:14:45Z

Message from Gerrit User 5976:

Patch Set 22:

Build is still in progress...
This change failed on freebsd-amd64-11_1:
See https://storage.googleapis.com/go-build-log/8853b41f/freebsd-amd64-11_1_4f1cf3bb.log

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/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T15:17:34Z

Message from Gerrit User 5976:

Patch Set 22: TryBot-Result-1

1 of 19 TryBots failed:
Failed on freebsd-amd64-11_1: https://storage.googleapis.com/go-build-log/8853b41f/freebsd-amd64-11_1_4f1cf3bb.log

Consult https://build.golang.org/ to see whether they are new failures.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

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 several bsds. Fixes #27029 Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io> Co-authored-by: Julia Nedialkova <yulia.nedyalkova@sap.com> Change-Id: Id9fcdf4775962b021b7ff438dc51ee6d16bb5f56 GitHub-Last-Rev: b30a621 GitHub-Pull-Request: #27871 Reviewed-on: https://go-review.googlesource.com/c/137442 Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot · 2018-10-30T15:19:21Z

Message from Gerrit User 5206:

Patch Set 22: Code-Review+2

OK, looks good. Whew. Thanks for your patience on this.

Please don’t reply on this GitHub thread. Visit golang.org/cl/137442.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-10-30T15:23:32Z

This PR is being closed because golang.org/cl/137442 has been merged.

googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Sep 26, 2018

ostenbom force-pushed the os-removeall-long-path-names branch from 3043fe3 to e12cec7 Compare September 26, 2018 09:17

ostenbom force-pushed the os-removeall-long-path-names branch from e12cec7 to df255c8 Compare September 26, 2018 10:16

ostenbom force-pushed the os-removeall-long-path-names branch from df255c8 to f616868 Compare September 26, 2018 12:05

ostenbom force-pushed the os-removeall-long-path-names branch from f616868 to 3cafab3 Compare September 26, 2018 12:22

ostenbom force-pushed the os-removeall-long-path-names branch from 3cafab3 to 9ab706c Compare September 26, 2018 13:25

ostenbom force-pushed the os-removeall-long-path-names branch from 666fc8b to 886197c Compare October 29, 2018 14:18

ostenbom force-pushed the os-removeall-long-path-names branch from 886197c to b30a621 Compare October 29, 2018 14:28

ianlancetaylor added cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. and removed cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. labels Oct 30, 2018

gopherbot closed this Oct 30, 2018

ostenbom mentioned this pull request Oct 30, 2018

os: add support for long path names on unix RemoveAll #28494

Closed

os: add support for long path names on unix RemoveAll #27871

os: add support for long path names on unix RemoveAll #27871

Conversation

ostenbom commented Sep 26, 2018 • edited Loading

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Sep 26, 2018

googlebot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Oct 30, 2018

ostenbom commented Sep 26, 2018 •

edited

Loading