Use Fchmodat from x/sys/unix

[tklauser]

Replace the custom Fchmodat implementation in sysx by using Fchmodat
from golang.org/x/sys/unix now that it supports Fchmodat on Linux,
Darwin, FreeBSD and Solaris (i.e. all platforms containerd/continuity
supports).

Because chmod on symlinks (AT_SYMLINK_NOFOLLOW) is not supported on
Linux, some additional checking is needed. Previously, driver.Lchmod
just followed the symlink and applied the mode to the linked path. See
golang/go#20130 and golang/sys@c23410a for details.

Updates #63

Signed-off-by: Tobias Klauser [email protected]

[dmcgowan]

I agree with removing the sysx entry, but not sure I understand the rationale behind the if blocks. An error is already thrown when the operation is not supported, we should let the operating system be responsible for making this determination rather than hard coding it. Currently no action is taken in that condition and the error is not switchable. I would recommend just leaving that section of the code the way it was, or possibly adding an errors.Wrap if there is not enough error context.

[tklauser]

You're right, the if blocks should no longer be needed as we now catch the flag in x/sys/unix and return ENOTSUPP in case AT_SYMLINK_NOFOLLOW is given in flags on Linux. Will update the PR.

[tklauser]

Ah, now I remember why I added the condition. Because Fchmodat is always called with AT_SYMLINK_NOFOLLOW unconditionally set, on Linux we would always get an ENOTSUPP error (see #100 (comment)). So I think the check for mode&os.ModeSymlink needs to stay (but inverted) and we need to pass 0 flags in case it is not a symlink, in order to get the fchmodat syscall actually called. I made the check Linux specific because the other GOOSes supported by continuity are not affected by this problem.

I'll adjust the PR accordingly to not return an own error but pass on the ENOTSUPP then.

[dmcgowan]

This isn't going to work, normally only a os.ModePerm is passed in

[dmcgowan]

I still don't understand, if Lchmod on symlinks is not supported on Linux, why is it bad to return a not supported error when it is called? I would rather not try and do magic here and attempt to figure out what the caller wants. The caller tried to do something that isn't allowed, it failed. Is this just a condition we should document and let the callers handle?

[tklauser]

So continuity's Lchmod is only called on symlinks then, not regular files?

Otherwise, it would be bad because the error is always returned if AT_SYMLINK_NOFOLLOW is set (which Lchmod currently does unconditionally), even if path is a file and not a symlink. This behaviour is implemented in unix.Fchmodat (see golang/sys@c23410a), not the Linux syscall and was added to avoid people from accidentally changing the mode of a file when they meant to change the symlink (as the syscall will just silently follow the symlink). This is also in line with what fchmodat(2) in glibc implements on Linux.

Given your inline comment it doesn't seem easy to distinguish files from symlinks, so I guess the current implementation with wrapping the syscall in the driver and risking to change the file not the symlink is better than this PR's solution.

[dmcgowan]

So continuity's Lchmod is only called on symlinks then, not regular files?

I think there is a mixture of my own confusion and some misunderstanding of what we are trying to communicate to each other. Let's just break it down first by current behavior and desired behavior, then make sure this PR is accomplishing that.

My understanding of current behavior

• Lchmod("/somesymlink", 0644) -> Incorrectly follows symlinks since the no follow flag is not supported on Linux and it is just ignored (previously not supported error was handled by glibc by calling directly just drops the flag) ...This is really bad!
• Lchmod("/regularfile", 0644) -> works just as expected

What I think the correct behavior is

• Lchmod("/somesymlink", 0644) -> returns unsupported error on Linux, no way to perform operation
• Lchmod("/regularfile", 0644) -> works just as expected

Then the expectation would be the caller uses Lchmod and handles the not supported error with symlinks or does not call this method at all on symlinks. Based on this thinking and the fix you already put into sys, shouldn't return unix.Fchmodat(0, path, uint32(mode), unix.AT_SYMLINK_NOFOLLOW) be enough to get to the correct behavior? If not, please let me know where we are not in agreement or I am wrong.

[dmcgowan]

shouldn't return unix.Fchmodat(0, path, uint32(mode), unix.AT_SYMLINK_NOFOLLOW) be enough to get to the correct behavior?

I see what you are saying where this call will always error since it always ends up throwing the error regardless of whether it is a symlink. My suggestion would then, if we are going to keep Lchmod, it needs to stat on Linux and throw the unsupported error if the file is a symlink. We should be providing a regular Chmod as well that does not need to do this. Looking at containerd and docker, neither attempt to call Lchmod and only do Chmod after checking that it is not a symlink. Long story short, Lchmod probably just shouldn't exist or be used on Linux...

[tklauser]

Sorry for not being clearer (English is not my native language) and thanks for taking the time for breaking it down!

My understanding of current behavior

• Lchmod("/somesymlink", 0644) -> Incorrectly follows symlinks since the no follow flag is not supported on Linux and it is just ignored (previously not supported error was handled by glibc by calling directly just drops the flag) ...This is really bad!
• Lchmod("/regularfile", 0644) -> works just as expected

That's my understanding as well and what I was trying to explain (insufficiently) in the previous comments.

What I think the correct behavior is

• Lchmod("/somesymlink", 0644) -> returns unsupported error on Linux, no way to perform operation
• Lchmod("/regularfile", 0644) -> works just as expected

Same here, I agree that the implementation should behave like this.

I see what you are saying where this call will always error since it always ends up throwing the error regardless of whether it is a symlink.

Yes, that's what the referenced change in x/sys/unix introduced.

My suggestion would then, if we are going to keep Lchmod, it needs to stat on Linux and throw the unsupported error if the file is a symlink. We should be providing a regular Chmod as well that does not need to do this. Looking at containerd and docker, neither attempt to call Lchmod and only do Chmod after checking that it is not a symlink. Long story short, Lchmod probably just shouldn't exist or be used on Linux...

Ok, that makes sense.

So should I rather update the PR to remove Lchmod? Or open a new PR for this?

And then update func (c *context) Apply(resource Resource) error to call os.Chmod. I just noticed that there is already a check for the file type and chmod is disabled in case it is a symlink:

continuity/context.go

Lines 476 to 478 in b2b946a

	// NOTE(stevvooe): Chmod on symlink is not supported on linux. We
	// may want to maintain support for other platforms that have it.
	chmod = false

So I guess it would be safe to call os.Chmod instead of c.driver.Lchmod here:

continuity/context.go

Lines 534 to 538 in b2b946a

	if chmod {
	if err := c.driver.Lchmod(fp, resource.Mode()); err != nil {
	return err
	}
	}

[kolyshkin]

Now when #120 is merged this one can be closed I guess.

[dmcgowan]

Thanks @tklauser for the contribution, go 1.11 forced us to solve this in a slightly different way