Skip to content

Fix Lchmod() for Linux+Go1.11 #120

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.

Sign up for GitHub

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

Merged
crosbymichael merged 5 commits into from
Jul 12, 2018
Merged

Fix Lchmod() for Linux+Go1.11 #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d2ce1bc
sysx/xattr_darwin.go: rm duplicate Fchmodat def
kolyshkin Jul 11, 2018
2b69c16
sysx.Fchmodat(): remove
kolyshkin Jul 11, 2018
9ab0ec6
Lchmod(): simplify and optimize
kolyshkin Jul 11, 2018
94af800
Lchmod(): fix for Linux/Go 1.11
kolyshkin Jul 11, 2018
6d0b394
context.Apply: no need to skip chmod on symlinks
kolyshkin Jul 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
13 changes: 0 additions & 13 deletions driver/driver_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"errors"
"fmt"
"os"
"path/filepath"
"sort"

"github.com/containerd/continuity/devices"
Expand All @@ -26,18 +25,6 @@ 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 sysx.Fchmodat(0, path, uint32(mode), sysx.AtSymlinkNofollow)
}

// 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)
Expand Down
19 changes: 19 additions & 0 deletions driver/lchmod_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Perhaps overthinking it, but leaving this here because I learned to never "assume" things 😅)

From https://go-review.googlesource.com/c/go/+/118658

Currently Linux' fchmodat(2) syscall implementation doesn't support the
flags parameter (though it might in future versions [1]). Fchmodat in
the syscall package takes the parameter and (wrongly) passes it on to the
syscall which will ignore it.

According to the POSIX.1-2008 manual page [2], AT_SYMLINK_NOFOLLOW is
the only valid value for the flags parameter and EOPNOTSUPP should be
returned in case changing the mode of a symbolic link is not supported
by the underlying system. EINVAL should be returned for any other value
of the flags parameter.

  [1] https://patchwork.kernel.org/patch/9596301/
  [2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/chmod.html

That patch mentions the possibility that in future, other options may actually
become active.

Instead of skipping sysx.Fchmodat() for symlinks, should we always run it, but check if the returned error was an EOPNOTSUPP, then return nil if the path was a symlink?

Copy link
Copy Markdown
Contributor Author

@kolyshkin kolyshkin Jul 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. syscall.Fchmodat() from golang < 1.11 does not return the error (same for unix.Fchmodat() from golang.org/x/sys/unix before golang/sys@c23410a which only landed about a year ago), instead changing the mode for the file that a symlink points to (bad bad bad!). So we can't rely on it returning the proper error, and from the correctness/security standpoint it makes sense to avoid the call.

  2. Linux won't support mode for symlinks, ever (I mean, I can't speak for Linus, but it looks like it). The patch being referred to above (https://patchwork.kernel.org/patch/9596301/) only adds flags argument to fchmodat(), for the sake of returning an error in case flags are specified. Even that one didn't made it to the kernel, and it seems to be the 3rd attempt, previous two happened in 2015 (https://groups.google.com/d/msg/linux.kernel/Z2z3_MR2aM8/mFz04xQhZEIJ) and 2014 (https://groups.google.com/d/msg/linux.kernel/8lnkJ-naNRI/QvkfwNih6rcJ).

Copy link
Copy Markdown
Contributor Author

@kolyshkin kolyshkin Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ignore the previous comment; while correct it misses the biggest issue, that is, EOPNOTSUPP is returned always, not just for the symlinks. In other words, calling Fchmodat() with flags != 0 is useless, it does nothing but returns an error, and it happens for all files, not just symlinks.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing the digging; looks like the current approach is best then!

return nil
}
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else if err != nil ? I assume if there is an error in that case, we don't want to rely on Fchmodat returning the appropriate error

Copy link
Copy Markdown
Contributor Author

@kolyshkin kolyshkin Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was my original approach, but then I realized I am changing the behavior that way (as previously the error (like ENOENT or EACCESS) was returned from Fchmodat()).

This code assumes that if path can't be stat() it is definitely not a symlink.

Copy link
Copy Markdown
Member

@dmcgowan dmcgowan Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh, because the os package will wrap the error?

Copy link
Copy Markdown
Contributor Author

@kolyshkin kolyshkin Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For one thing, yes. I didn't want to inadvertently open a can of worms.

In general it's a problem that is out of scope for this PR. The problem is, some errors returned by continuity come from high-level Go functions (like the ones from os) and are of PathError type, some come from low-level (like x/sys/unix or syscall) and are not wrapped. The most user-visible difference is high-level ones contain file names and in general provide more clue about what is going on.

Based on the above, it would be good to review and unify errors returned (I am in favor of os.PathError{}). But again it's out of scope for this PR.


return unix.Fchmodat(unix.AT_FDCWD, path, uint32(mode), 0)
}
14 changes: 14 additions & 0 deletions driver/lchmod_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
}
18 changes: 0 additions & 18 deletions sysx/chmod_darwin.go
View file
Open in desktop

This file was deleted.

25 changes: 0 additions & 25 deletions sysx/chmod_darwin_386.go
View file
Open in desktop

This file was deleted.

25 changes: 0 additions & 25 deletions sysx/chmod_darwin_amd64.go
View file
Open in desktop

This file was deleted.

17 changes: 0 additions & 17 deletions sysx/chmod_freebsd.go
View file
Open in desktop

This file was deleted.

25 changes: 0 additions & 25 deletions sysx/chmod_freebsd_amd64.go
View file
Open in desktop

This file was deleted.

12 changes: 0 additions & 12 deletions sysx/chmod_linux.go
View file
Open in desktop

This file was deleted.

11 changes: 0 additions & 11 deletions sysx/chmod_solaris.go
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion sysx/xattr_darwin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down