search

containerd / continuity

A transport-agnostic, filesystem metadata manifest system
https://containerd.io
Apache License 2.0
139 stars 67 forks source link

Use Fchmodat from x/sys/unix #100

Closed tklauser closed 6 years ago

tklauser commented 6 years ago

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 tklauser@distanz.ch

dmcgowan commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 https://github.com/containerd/continuity/pull/100#issuecomment-352671984). 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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

What I think the correct behavior is

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 commented 6 years ago

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 commented 6 years ago

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: https://github.com/containerd/continuity/blob/b2b946a77f5973f420514090d6f6dd58b08303f0/context.go#L476-L478 So I guess it would be safe to call os.Chmod instead of c.driver.Lchmod here: https://github.com/containerd/continuity/blob/b2b946a77f5973f420514090d6f6dd58b08303f0/context.go#L534-L538

kolyshkin commented 6 years ago

Now when https://github.com/containerd/continuity/pull/120 is merged this one can be closed I guess.

dmcgowan commented 6 years ago

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