code review 102640045: os: handle the sticky bit separately for *BSD

os: handle the sticky bit separately for *BSD

open(2) and mkdir(2) won't set the sticky bit on *BSD.
This behavior is mentioned on sticky(8).
see also: https://github.com/dotcloud/docker/pull/6587

Fixes issue 8383.

[unclejack, 2014-07-11 22:07:36]

Can you review this CL, please?

[dfc, 2014-07-12 00:19:57]

On 2014/07/11 22:07:36, unclejack wrote:
> Can you review this CL, please?

You have to `hg mail` it first.

[unclejack, 2014-07-12 08:48:36]

On 2014/07/12 00:19:57, dfc wrote:
> You have to `hg mail` it first.

Unfortunately, it's not mine.

[kazuyoshi, 2014-07-14 13:27:16]

Hello golang-codereviews@googlegroups.com (cc: dave@cheney.net,
golang-codereviews@googlegroups.com, unclejacksons@gmail.com),

I'd like you to review this change to
https://code.google.com/p/go/

[iant, 2014-07-14 14:45:51]

It would be pretty easy to add a const that tells Openfile and Mkdir whether
they actually need to test this, and have the const be true on systems that
require it.  Is it worth doing?

For example, look at supportsCloseOnExec.

https://codereview.appspot.com/102640045/diff/40001/src/pkg/os/file.go
File src/pkg/os/file.go (right):

https://codereview.appspot.com/102640045/diff/40001/src/pkg/os/file.go#newcod...
src/pkg/os/file.go:207: // FreeBSD and NetBSD's open(2) can't handle the sticky
bit correctly
s/open/mkdir/ , I assume.

[kazuyoshi, 2014-07-15 13:01:23]

https://codereview.appspot.com/102640045/diff/40001/src/pkg/os/file.go
File src/pkg/os/file.go (right):

https://codereview.appspot.com/102640045/diff/40001/src/pkg/os/file.go#newcod...
src/pkg/os/file.go:207: // FreeBSD and NetBSD's open(2) can't handle the sticky
bit correctly
On 2014/07/14 14:45:51, iant wrote:
> s/open/mkdir/ , I assume.

Done.

[kazuyoshi, 2014-07-15 13:02:42]

On 2014/07/14 14:45:51, iant wrote:
> It would be pretty easy to add a const that tells Openfile and Mkdir whether
> they actually need to test this, and have the const be true on systems that
> require it.  Is it worth doing?
> 
> For example, look at supportsCloseOnExec.
> 
> https://codereview.appspot.com/102640045/diff/40001/src/pkg/os/file.go
> File src/pkg/os/file.go (right):
> 
>
https://codereview.appspot.com/102640045/diff/40001/src/pkg/os/file.go#newcod...
> src/pkg/os/file.go:207: // FreeBSD and NetBSD's open(2) can't handle the
sticky
> bit correctly
> s/open/mkdir/ , I assume.

Thanks for reviewing. I've added supportsCreateWithStickyBit as the constant.

[iant, 2014-07-15 21:05:37]

I suspect this will break the build on dragonfly because
supportsCreateWithStickyBit won't be defined anywhere.

Rather than try to fit this in with other files, I think we should introduce two
new files, one setting the new constant to false, one setting it to true, with
+build netbsd freebsd on one and +build !netbsd,!freebsd on the other.

[kazuyoshi, 2014-07-16 07:59:49]

Hello golang-codereviews@googlegroups.com, iant@golang.org (cc: dave@cheney.net,
golang-codereviews@googlegroups.com, unclejacksons@gmail.com),

Please take another look.

[kazuyoshi, 2014-07-16 08:02:23]

On 2014/07/16 07:59:49, kazuyoshi wrote:
> Hello mailto:golang-codereviews@googlegroups.com, mailto:iant@golang.org (cc:
mailto:dave@cheney.net,
> mailto:golang-codereviews@googlegroups.com, mailto:unclejacksons@gmail.com),
> 
> Please take another look.

In addition, this behaviour is also exists on OpenBSD, Dragonfly and Darwin.

http://www.freebsd.org/cgi/man.cgi?query=sticky&apropos=0&sektion=8&manpath=O...
http://leaf.dragonflybsd.org/cgi/web-man?command=sticky&section=ANY
https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPag...

[dfc, 2014-07-17 08:35:34]

I'd like to see a bug on the issue tracker for this, golang.org/issue

[kazuyoshi, 2014-07-17 14:05:37]

Hello golang-codereviews@googlegroups.com, iant@golang.org, dave@cheney.net (cc:
golang-codereviews@googlegroups.com, unclejacksons@gmail.com),

Please take another look.

[iant, 2014-07-17 21:34:58]

The description should say exactly

Fixes bug 8383.


That exact test is recognized by the code review system.

[iant, 2014-07-17 21:36:45]

https://codereview.appspot.com/102640045/diff/100001/src/pkg/os/sticky_bsd.go
File src/pkg/os/sticky_bsd.go (right):

https://codereview.appspot.com/102640045/diff/100001/src/pkg/os/sticky_bsd.go...
src/pkg/os/sticky_bsd.go:5: // +build freebsd netbsd openbsd darwin dragonfly
The two new +build lines are inconsistent.  One needs to be the exact negation
of the other, or there will be compilation errors on some systems.

[kazuyoshi, 2014-07-18 11:44:29]

Hello golang-codereviews@googlegroups.com, iant@golang.org, dave@cheney.net (cc:
golang-codereviews@googlegroups.com, unclejacksons@gmail.com),

Please take another look.

[kazuyoshi, 2014-11-01 21:22:53]

ping?

On 2014/07/18 11:44:29, kazuyoshi wrote:
> Hello mailto:golang-codereviews@googlegroups.com, mailto:iant@golang.org,
mailto:dave@cheney.net (cc:
> mailto:golang-codereviews@googlegroups.com, mailto:unclejacksons@gmail.com),
> 
> Please take another look.

[iant, 2014-11-01 22:11:30]

Very sorry, I completely lost track of this.

https://codereview.appspot.com/102640045/diff/120001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/102640045/diff/120001/src/pkg/os/file_unix.go#...
src/pkg/os/file_unix.go:80: if !supportsCreateWithStickyBit && e == nil &&
perm&ModeSticky != 0 {
It seems to me that this will do the wrong thing if the file is not being
created.  We don't want to change the mode of an existing file.  Unfortunately I
think we need to write something like

    chmod := false
    if !supportsCreateWithStickyBit && flag&O_CREATE != 0 && perm&ModeSticky !=
0 {
        if _, err := Stat(name); IsNotExist(err) {
            chmod = true
        }
    }

    r, e := syscall.Open(...)

    if chmod && e == nil {
        e = Chmod(name, perm)
    }

This is very imperfect but I don't see what else we can do.

[kazuyoshi, 2014-12-17 16:10:23]

On 2014/11/01 22:11:30, iant wrote:
> Very sorry, I completely lost track of this.
> 
> https://codereview.appspot.com/102640045/diff/120001/src/pkg/os/file_unix.go
> File src/pkg/os/file_unix.go (right):
> 
>
https://codereview.appspot.com/102640045/diff/120001/src/pkg/os/file_unix.go#...
> src/pkg/os/file_unix.go:80: if !supportsCreateWithStickyBit && e == nil &&
> perm&ModeSticky != 0 {
> It seems to me that this will do the wrong thing if the file is not being
> created.  We don't want to change the mode of an existing file.  Unfortunately
I
> think we need to write something like
> 
>     chmod := false
>     if !supportsCreateWithStickyBit && flag&O_CREATE != 0 && perm&ModeSticky
!=
> 0 {
>         if _, err := Stat(name); IsNotExist(err) {
>             chmod = true
>         }
>     }
> 
>     r, e := syscall.Open(...)
> 
>     if chmod && e == nil {
>         e = Chmod(name, perm)
>     }
> 
> This is very imperfect but I don't see what else we can do.

The change is submitted as https://go-review.googlesource.com/#/c/1673/ and
merged! Thanks.