Repository navigation
Preserve requested sticky bits when creating BSD directories - #382
Conversation
EdSchouten
left a comment
EdSchouten
left a comment
There was a problem hiding this comment.
Just some minor remarks :)
| // Mkdir is the equivalent of os.Mkdir(). | ||
| // On macOS and FreeBSD, local directories install ModeSticky | ||
| // non-atomically and require search access to the new directory. | ||
| // An error while installing the bit leaves the directory in place. |
There was a problem hiding this comment.
I'd argue that this sort of is a quality of implementation issue; I'd rather not document a bunch of OS specifics in this source file.
There was a problem hiding this comment.
Removed the OS-specific notes from the shared interface and kept the fallback limitations beside the local implementation.
| if perm&os.ModeSticky == 0 || runtime.GOOS == "linux" || runtime.GOOS == "android" { | ||
| return nil | ||
| } | ||
|
|
||
| // BSD mkdirat() discards the sticky bit. Open the directory without | ||
| // following symlinks, so stat and chmod act on the same directory. | ||
| // Preserve the kernel's umask and inherited permission bits. | ||
| fd, err := unix.Openat(d.fd, name.String(), unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC|oflagSearch, 0) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer unix.Close(fd) | ||
| var stat unix.Stat_t | ||
| if err := unix.Fstat(fd, &stat); err != nil { | ||
| return err | ||
| } | ||
| return unix.Fchmod(fd, uint32(stat.Mode)&0o7777|unix.S_ISVTX) |
There was a problem hiding this comment.
Nit: Could you please write this if-statement the other way around?
if perm & os.ModeSticky != 0 && ... {
}To me it seems a bit more intuitive to have the not-so-common case inside the if-statement. That would also compose better if we also need even more quirks to support other types of mode bits.
There was a problem hiding this comment.
Changed this to a positive sticky-bit condition, with the BSD fallback inside the block and a successful return afterward. The Linux and Android exclusions are preserved.
| } | ||
| for _, mask := range []string{"0", "022", "077", "0400", "0100", "0700", "0777"} { | ||
| t.Run(mask, func(t *testing.T) { | ||
| command := exec.Command(os.Args[0], "-test.run=^TestLocalDirectoryMkdirStickyUmask$") |
There was a problem hiding this comment.
Hmmm, sorry. I should have called this out before, but I guess I overlooked this.
I'm not a bit fan of this approach. When we invoke Go tests through Bazel using bazel test, then there is no guarantee that the binaries we get are compatible with go test. As in, rules_go can implement the go_test() rule any way it wants and generate different types of entry points.
Is there no way this can be implemented without launching subprocesses?
There was a problem hiding this comment.
Yes. Both tests now use sequential subtests without launching processes. The umask is changed only around Mkdir; the descriptor test restores its soft limit and closes its descriptors before reporting results. The same umask, exhaustion, partial-creation and leak checks remain. Native macOS normal/race tests, the four cross-builds, formatting and golint pass.
2 participants
On macOS and FreeBSD, mkdirat() drops S_ISVTX, so Mkdir(name, 0777 | os.ModeSticky) succeeds with an ordinary directory. Install the requested bit through a no-follow directory descriptor after creation, preserving the permission bits returned by fstat(). The kernel still applies the umask and inherited directory permissions.
The fallback is non-atomic and requires search access to the new directory. Failures leave the created entry in place. The descriptor keeps stat/chmod on one inode; it does not guarantee that the name still refers to the directory just created.
Regression tests cover ordinary and sticky modes, search-only and non-searchable modes, varied umasks, existing entries, inherited permissions, and descriptor exhaustion/leaks. They fail with the fix removed.
Validation: filesystem tests on macOS, Linux amd64 and Windows amd64; race tests on macOS and Linux; Linux amd64/arm64, Windows amd64 and FreeBSD amd64 cross-builds; upstream formatting, generation and golint. Native FreeBSD testing is unavailable.
Fixes #381.