From d2ce1bcaeea9dee644a01097e1dfc21ce5b75632 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 10 Jul 2018 17:56:06 -0700 Subject: [PATCH 1/5] sysx/xattr_darwin.go: rm duplicate Fchmodat def Commit 38f54c7c6 "Add fchmodat system calls for darwin" mistakenly added Fchmodat() for Darwin in two places. The error is hidden as the generated files (sysx/xattr_darwin_{i386,amd64}.go were not re-generated since. Fixes: 38f54c7c6 "Add fchmodat system calls for darwin" Cc: Derek McGowan Signed-off-by: Kir Kolyshkin --- sysx/xattr_darwin.go | 1 - 1 file changed, 1 deletion(-) diff --git a/sysx/xattr_darwin.go b/sysx/xattr_darwin.go index 1164a7d1..58da4307 100644 --- a/sysx/xattr_darwin.go +++ b/sysx/xattr_darwin.go @@ -8,7 +8,6 @@ package sysx //sys setxattr(path string, attr string, data []byte, flags int) (err error) //sys removexattr(path string, attr string, options int) (err error) //sys listxattr(path string, dest []byte, options int) (sz int, err error) -//sys Fchmodat(dirfd int, path string, mode uint32, flags int) (err error) const ( xattrNoFollow = 0x01 From 2b69c164288e4f1a840ceb85c05f93751178e795 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 10 Jul 2018 19:45:38 -0700 Subject: [PATCH 2/5] sysx.Fchmodat(): remove As of Go 1.10 (probably earlier) Fchmodat() is available from golang.org/x/sys/unix directly, so ditch custom implementations. This assumes there are no external users of github.com/containerd/continuity/sysx. If there are any, they should switch to golang.org/x/sys/unix. Signed-off-by: Kir Kolyshkin --- driver/driver_unix.go | 3 ++- sysx/chmod_darwin.go | 18 ------------------ sysx/chmod_darwin_386.go | 25 ------------------------- sysx/chmod_darwin_amd64.go | 25 ------------------------- sysx/chmod_freebsd.go | 17 ----------------- sysx/chmod_freebsd_amd64.go | 25 ------------------------- sysx/chmod_linux.go | 12 ------------ sysx/chmod_solaris.go | 11 ----------- 8 files changed, 2 insertions(+), 134 deletions(-) delete mode 100644 sysx/chmod_darwin.go delete mode 100644 sysx/chmod_darwin_386.go delete mode 100644 sysx/chmod_darwin_amd64.go delete mode 100644 sysx/chmod_freebsd.go delete mode 100644 sysx/chmod_freebsd_amd64.go delete mode 100644 sysx/chmod_linux.go delete mode 100644 sysx/chmod_solaris.go diff --git a/driver/driver_unix.go b/driver/driver_unix.go index 67493ade..6d9747a6 100644 --- a/driver/driver_unix.go +++ b/driver/driver_unix.go @@ -11,6 +11,7 @@ import ( "github.com/containerd/continuity/devices" "github.com/containerd/continuity/sysx" + "golang.org/x/sys/unix" ) func (d *driver) Mknod(path string, mode os.FileMode, major, minor int) error { @@ -35,7 +36,7 @@ func (d *driver) Lchmod(path string, mode os.FileMode) (err error) { } } - return sysx.Fchmodat(0, path, uint32(mode), sysx.AtSymlinkNofollow) + return unix.Fchmodat(0, path, uint32(mode), unix.AT_SYMLINK_NOFOLLOW) } // Getxattr returns all of the extended attributes for the file at path p. diff --git a/sysx/chmod_darwin.go b/sysx/chmod_darwin.go deleted file mode 100644 index e3ae2b7b..00000000 --- a/sysx/chmod_darwin.go +++ /dev/null @@ -1,18 +0,0 @@ -package sysx - -const ( - // AtSymlinkNoFollow defined from AT_SYMLINK_NOFOLLOW in - AtSymlinkNofollow = 0x20 -) - -const ( - - // SYS_FCHMODAT defined from golang.org/sys/unix - SYS_FCHMODAT = 467 -) - -// These functions will be generated by generate.sh -// $ GOOS=darwin GOARCH=386 ./generate.sh chmod -// $ GOOS=darwin GOARCH=amd64 ./generate.sh chmod - -//sys Fchmodat(dirfd int, path string, mode uint32, flags int) (err error) diff --git a/sysx/chmod_darwin_386.go b/sysx/chmod_darwin_386.go deleted file mode 100644 index 5a8cf5b5..00000000 --- a/sysx/chmod_darwin_386.go +++ /dev/null @@ -1,25 +0,0 @@ -// mksyscall.pl -l32 chmod_darwin.go -// MACHINE GENERATED BY THE COMMAND ABOVE; DO NOT EDIT - -package sysx - -import ( - "syscall" - "unsafe" -) - -// THIS FILE IS GENERATED BY THE COMMAND AT THE TOP; DO NOT EDIT - -func Fchmodat(dirfd int, path string, mode uint32, flags int) (err error) { - var _p0 *byte - _p0, err = syscall.BytePtrFromString(path) - if err != nil { - return - } - _, _, e1 := syscall.Syscall6(SYS_FCHMODAT, uintptr(dirfd), uintptr(unsafe.Pointer(_p0)), uintptr(mode), uintptr(flags), 0, 0) - use(unsafe.Pointer(_p0)) - if e1 != 0 { - err = errnoErr(e1) - } - return -} diff --git a/sysx/chmod_darwin_amd64.go b/sysx/chmod_darwin_amd64.go deleted file mode 100644 index 3287d1d5..00000000 --- a/sysx/chmod_darwin_amd64.go +++ /dev/null @@ -1,25 +0,0 @@ -// mksyscall.pl chmod_darwin.go -// MACHINE GENERATED BY THE COMMAND ABOVE; DO NOT EDIT - -package sysx - -import ( - "syscall" - "unsafe" -) - -// THIS FILE IS GENERATED BY THE COMMAND AT THE TOP; DO NOT EDIT - -func Fchmodat(dirfd int, path string, mode uint32, flags int) (err error) { - var _p0 *byte - _p0, err = syscall.BytePtrFromString(path) - if err != nil { - return - } - _, _, e1 := syscall.Syscall6(SYS_FCHMODAT, uintptr(dirfd), uintptr(unsafe.Pointer(_p0)), uintptr(mode), uintptr(flags), 0, 0) - use(unsafe.Pointer(_p0)) - if e1 != 0 { - err = errnoErr(e1) - } - return -} diff --git a/sysx/chmod_freebsd.go b/sysx/chmod_freebsd.go deleted file mode 100644 index b64a708b..00000000 --- a/sysx/chmod_freebsd.go +++ /dev/null @@ -1,17 +0,0 @@ -package sysx - -const ( - // AtSymlinkNoFollow defined from AT_SYMLINK_NOFOLLOW in - AtSymlinkNofollow = 0x200 -) - -const ( - - // SYS_FCHMODAT defined from golang.org/sys/unix - SYS_FCHMODAT = 490 -) - -// These functions will be generated by generate.sh -// $ GOOS=freebsd GOARCH=amd64 ./generate.sh chmod - -//sys Fchmodat(dirfd int, path string, mode uint32, flags int) (err error) diff --git a/sysx/chmod_freebsd_amd64.go b/sysx/chmod_freebsd_amd64.go deleted file mode 100644 index 5a271abb..00000000 --- a/sysx/chmod_freebsd_amd64.go +++ /dev/null @@ -1,25 +0,0 @@ -// mksyscall.pl chmod_freebsd.go -// MACHINE GENERATED BY THE COMMAND ABOVE; DO NOT EDIT - -package sysx - -import ( - "syscall" - "unsafe" -) - -// THIS FILE IS GENERATED BY THE COMMAND AT THE TOP; DO NOT EDIT - -func Fchmodat(dirfd int, path string, mode uint32, flags int) (err error) { - var _p0 *byte - _p0, err = syscall.BytePtrFromString(path) - if err != nil { - return - } - _, _, e1 := syscall.Syscall6(SYS_FCHMODAT, uintptr(dirfd), uintptr(unsafe.Pointer(_p0)), uintptr(mode), uintptr(flags), 0, 0) - use(unsafe.Pointer(_p0)) - if e1 != 0 { - err = errnoErr(e1) - } - return -} diff --git a/sysx/chmod_linux.go b/sysx/chmod_linux.go deleted file mode 100644 index 89df6d38..00000000 --- a/sysx/chmod_linux.go +++ /dev/null @@ -1,12 +0,0 @@ -package sysx - -import "syscall" - -const ( - // AtSymlinkNoFollow defined from AT_SYMLINK_NOFOLLOW in /usr/include/linux/fcntl.h - AtSymlinkNofollow = 0x100 -) - -func Fchmodat(dirfd int, path string, mode uint32, flags int) error { - return syscall.Fchmodat(dirfd, path, mode, flags) -} diff --git a/sysx/chmod_solaris.go b/sysx/chmod_solaris.go deleted file mode 100644 index 3ba6e5ed..00000000 --- a/sysx/chmod_solaris.go +++ /dev/null @@ -1,11 +0,0 @@ -package sysx - -import "golang.org/x/sys/unix" - -const ( - AtSymlinkNofollow = unix.AT_SYMLINK_NOFOLLOW -) - -func Fchmodat(dirfd int, path string, mode uint32, flags int) error { - return unix.Fchmodat(dirfd, path, mode, flags) -} From 9ab0ec639e278f8f9a3c3c3a1ffecc199b885865 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 10 Jul 2018 19:28:29 -0700 Subject: [PATCH 3/5] Lchmod(): simplify and optimize A quote from fchmodat(2) man page (this one is for Linux, others should be similar): > If pathname is relative and dirfd is the special value > AT_FDCWD, then pathname is interpreted relative to the current working > directory of the calling process (like chmod()). > > If pathname is absolute, then dirfd is ignored. So, instead of calling filepath.Abs() let's use AT_FDCWD and let the kernel figure things out for us. While at it, fix the comment. Signed-off-by: Kir Kolyshkin --- driver/driver_unix.go | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/driver/driver_unix.go b/driver/driver_unix.go index 6d9747a6..d9b6cb7e 100644 --- a/driver/driver_unix.go +++ b/driver/driver_unix.go @@ -6,7 +6,6 @@ import ( "errors" "fmt" "os" - "path/filepath" "sort" "github.com/containerd/continuity/devices" @@ -27,16 +26,9 @@ func (d *driver) Mkfifo(path string, mode os.FileMode) error { return devices.Mknod(path, mode, 0, 0) } -// Lchmod changes the mode of an file not following symlinks. -func (d *driver) Lchmod(path string, mode os.FileMode) (err error) { - if !filepath.IsAbs(path) { - path, err = filepath.Abs(path) - if err != nil { - return - } - } - - return unix.Fchmodat(0, path, uint32(mode), unix.AT_SYMLINK_NOFOLLOW) +// Lchmod changes the mode of a file not following symlinks. +func (d *driver) Lchmod(path string, mode os.FileMode) error { + return unix.Fchmodat(unix.AT_FDCWD, path, uint32(mode), unix.AT_SYMLINK_NOFOLLOW) } // Getxattr returns all of the extended attributes for the file at path p. From 94af8008a7b687f8748385cf34f5a46c333ec511 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 10 Jul 2018 19:55:08 -0700 Subject: [PATCH 4/5] Lchmod(): fix for Linux/Go 1.11 Linux does not support file mode for symlinks. It does not support flags argument for fchmodat() syscall either (the only flag described by POSIX is AT_SYMLINK_NOFOLLOW to allow changing mode on symlink). This is probably the reason why the standard os package from Go does not provide Lchmod(). Go < 1.11 errorneously assumed the flags argument is supported on Linux, and implemented syscall.Fchmodat() with flags being passed on to the kernel. The kernel ignored the flags (as the kernel syscall doesn't even have flags argument). The result was changing the mode on the file that symlink points to (i.e. ignoring AT_SYMLINK_NOFOLLOW). The above bug was fixed in Go-1.11beta1 (see [1]), and since the fix, if any flags are set, an error is returned. This rendered Lchmod() useless on Linux + Go 1.11, as now it always returns an error. The best possible fix I can think of, given the constraints of keeping the API intact, is implemented in this commit. In short, chown() is not called for symlinks on Linux. [1] https://go-review.googlesource.com/c/go/+/118658 Signed-off-by: Kir Kolyshkin --- driver/driver_unix.go | 6 ------ driver/lchmod_linux.go | 19 +++++++++++++++++++ driver/lchmod_unix.go | 14 ++++++++++++++ 3 files changed, 33 insertions(+), 6 deletions(-) create mode 100644 driver/lchmod_linux.go create mode 100644 driver/lchmod_unix.go diff --git a/driver/driver_unix.go b/driver/driver_unix.go index d9b6cb7e..c7d4e6ba 100644 --- a/driver/driver_unix.go +++ b/driver/driver_unix.go @@ -10,7 +10,6 @@ import ( "github.com/containerd/continuity/devices" "github.com/containerd/continuity/sysx" - "golang.org/x/sys/unix" ) func (d *driver) Mknod(path string, mode os.FileMode, major, minor int) error { @@ -26,11 +25,6 @@ func (d *driver) Mkfifo(path string, mode os.FileMode) error { return devices.Mknod(path, mode, 0, 0) } -// Lchmod changes the mode of a file not following symlinks. -func (d *driver) Lchmod(path string, mode os.FileMode) error { - return unix.Fchmodat(unix.AT_FDCWD, path, uint32(mode), unix.AT_SYMLINK_NOFOLLOW) -} - // Getxattr returns all of the extended attributes for the file at path p. func (d *driver) Getxattr(p string) (map[string][]byte, error) { xattrs, err := sysx.Listxattr(p) diff --git a/driver/lchmod_linux.go b/driver/lchmod_linux.go new file mode 100644 index 00000000..39ffe9cc --- /dev/null +++ b/driver/lchmod_linux.go @@ -0,0 +1,19 @@ +package driver + +import ( + "os" + + "golang.org/x/sys/unix" +) + +// Lchmod changes the mode of a file not following symlinks. +func (d *driver) Lchmod(path string, mode os.FileMode) error { + // On Linux, file mode is not supported for symlinks, + // and fchmodat() does not support AT_SYMLINK_NOFOLLOW, + // so symlinks need to be skipped entirely. + if st, err := os.Stat(path); err == nil && st.Mode()&os.ModeSymlink != 0 { + return nil + } + + return unix.Fchmodat(unix.AT_FDCWD, path, uint32(mode), 0) +} diff --git a/driver/lchmod_unix.go b/driver/lchmod_unix.go new file mode 100644 index 00000000..1b539f78 --- /dev/null +++ b/driver/lchmod_unix.go @@ -0,0 +1,14 @@ +// +build darwin freebsd solaris + +package driver + +import ( + "os" + + "golang.org/x/sys/unix" +) + +// Lchmod changes the mode of a file not following symlinks. +func (d *driver) Lchmod(path string, mode os.FileMode) error { + return unix.Fchmodat(unix.AT_FDCWD, path, uint32(mode), unix.AT_SYMLINK_NOFOLLOW) +} From 6d0b39409f2f8a5deadc67471881658bd6af34fc Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 10 Jul 2018 20:14:02 -0700 Subject: [PATCH 5/5] context.Apply: no need to skip chmod on symlinks With the fix for Lchmod in place (see previous commit), there is no need to skip Lchmod() for symlinks. Signed-off-by: Kir Kolyshkin --- context.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/context.go b/context.go index ec21cf4f..40cb27f8 100644 --- a/context.go +++ b/context.go @@ -473,10 +473,6 @@ func (c *context) Apply(resource Resource) error { } } - // NOTE(stevvooe): Chmod on symlink is not supported on linux. We - // may want to maintain support for other platforms that have it. - chmod = false - case Device: if fi == nil { if err := c.driver.Mknod(fp, resource.Mode(), int(r.Major()), int(r.Minor())); err != nil {