archive: skip chmod IsNotExist error #4099
Conversation
|
Build succeeded.
|
archive/tar_unix.go
Outdated
| @@ -124,7 +124,7 @@ func handleTarTypeBlockCharFifo(hdr *tar.Header, path string) error { | |||
|
|
|||
| func handleLChmod(hdr *tar.Header, path string, hdrInfo os.FileInfo) error { | |||
| if hdr.Typeflag == tar.TypeLink { | |||
| if fi, err := os.Lstat(hdr.Linkname); err == nil && (fi.Mode()&os.ModeSymlink == 0) { | |||
| if _, err := os.Stat(path); os.IsExist(err) { | |||
There was a problem hiding this comment.
Can you clarify os.IsExists(err) part? When file exists, os.Stat returns nil error, right?
There was a problem hiding this comment.
If os.Chmod() changes the mode of the target, wouldn't this be the same as skipping the whole if hdr.Typeflag ..., and unconditionally doing;
if err := os.Chmod(path, hdrInfo.Mode()); err != nil && !os.IsNotExist(err) {
return err
}Was the intent here to set the mode of the link itself (and not the target?)
There was a problem hiding this comment.
Was the intent here to set the mode of the link itself (and not the target?)
What the original code tried to do is not clear (the commit message that added it does not give any reasoning). The original code checks that if tar headers say the file is hardlink and the target is not a symlink then os.Chmod().
My patch follows that but adds a check that the file exists before os.Chmod() for it. AFAICS,
if err := os.Chmod(path, hdrInfo.Mode()); err != nil && !os.IsNotExist(err) {
return err
}
would also work.
There was a problem hiding this comment.
Did some quick digging to find out what this code was intended to do. Haven't looked deeply yet, but looking for the history of it:
The code looks to be copied from https://github.com/moby/moby, where this function was extracted to a function (accounting for windows) in moby/moby@8228ee4.
The original code for this was added in moby/moby@ab181ce, which was this PR: moby/moby#11099, and was created to fix moby/moby#10937
There was a problem hiding this comment.
OK, so IIUC, the code was for situations where the hardlink is pointing to a symlink, which links to something that was not yet extracted from the tar archive. (which (again, IIUC) can cause issues in a situation where the hardlink/symlink is received before the final target is received from the tar stream?
I'm not too familiar with this stuff, but looks like we need to tread with care, and be sure this doesn't cause a regression.
At least looks like;
- we need to add proper comments to this code to explain what it's doing
- if fixes are needed, please also open a pull request in the https://github.com/moby/moby repository to have the same fix there 🤗
There was a problem hiding this comment.
@thaJeztah btw, can you confirm the failure on your side?
sudo ctr i pull docker.io/library/clearlinux:base
There was a problem hiding this comment.
How come the change from os.Lstat() to os.Stat()? If you are concerned with chmod'ing things, if it is a symlink (how this could even be the case, i'm not sure) then you'll need the information of the link itself. Stat will just give the information on the underlying inode which would be the hardlink you're targeting. Either may need to be chmod'ed.
I think the behaviour you're seeking would not be to edit this case, but add another that checks for the presence of the underlying file.
There was a problem hiding this comment.
I think the behaviour you're seeking would not be to edit this case, but add another that checks for the presence of the underlying file.
Yes, so as @thaJeztah also suggested,
if err := os.Chmod(path, hdrInfo.Mode()); err != nil && !os.IsNotExist(err) {
return err
}
There was a problem hiding this comment.
but let me comment still on:
How come the change from os.Lstat() to os.Stat()
os.Lstat() does not follow symlinks which is what os.Chmod() does so currently there's a race that a missing file can get unnoticed... that is why I suggested to move to os.Stat().
anyway, I'll submit an update to have it as suggested above.
handleLChmod() does not properly check that files behind the handlinks exist before calling os.Chmod(). We've seen base images where this results in "no such file or directory" error from os.Chmod() when unpacking the image. To keep the existing logic but fix the problem, this commit simply skips IsNotExist error. Signed-off-by: Mikko Ylinen <mikko.ylinen@intel.com>
mythi
changed the title
|
v2: updated the commit to simply just skip chmod IsNotExist error. |
|
Build succeeded.
|
Codecov Report
@@ Coverage Diff @@
## master #4099 +/- ##
==========================================
+ Coverage 42.43% 45.76% +3.33%
==========================================
Files 129 116 -13
Lines 14875 11935 -2940
==========================================
- Hits 6312 5462 -850
+ Misses 7644 5555 -2089
+ Partials 919 918 -1
Continue to review full report at Codecov.
|
|
LGTM |
1 similar comment
|
LGTM |
handleLChmod() does not properly check that files behind the handlinks exist
before calling os.Chmod(). We've seen base images where this results in
"no such file or directory" error from os.Chmod() when unpacking the image.
os.Stat() follows symlinks and tells us if the destination file exists.
Use that as the pre-check before os.Chmod() to avoid errors.
The failing case for me was:
sudo ctr i pull docker.io/library/clearlinux:base