Skip to content
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.

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

Clearlinux docker images does not unpack in containerd #1765

Closed
lucidprogrammer opened this issue Feb 20, 2020 · 26 comments
Closed

Clearlinux docker images does not unpack in containerd #1765

lucidprogrammer opened this issue Feb 20, 2020 · 26 comments
Assignees

Comments

@lucidprogrammer
Copy link

Not sure where to file this bug to get attention. Please review clearlinux/dockerfiles#384

@mythi
Copy link

mythi commented Feb 20, 2020

@lucidprogrammer this is the right place for your bug. #1044 suggests a couple of workarounds to try

@lucidprogrammer
Copy link
Author

@mythi tried whatever possible, no luck at all. As you see i am testing with the official clearlinux python image.

@mythi
Copy link

mythi commented Feb 20, 2020

tried whatever possible, no luck at all.

with having Clear Linux as your host OS it works

$ sudo ctr -n k8s.io i pull docker.io/clearlinux/python:3.7

@ahkok ahkok removed the new label Feb 20, 2020
@bryteise
Copy link
Member

Hrm interesting that it works on Clear, @lucidprogrammer what host OS are you running on?

@lucidprogrammer
Copy link
Author

lucidprogrammer commented Feb 25, 2020

please review clearlinux/dockerfiles#384 This issue is in aws fargate. (i run production workloads of every other type of container there, only clearlinux fails )

Kernel Version:             4.14.152-127.182.amzn2.x86_64
 OS Image:                   Amazon Linux 2
 Operating System:           linux
 Architecture:               amd64
 Container Runtime Version:  containerd://1.3.0

@ZhongbaoShi
Copy link

it seems there is a similar issue reported on containerd, containerd/containerd#3974

@mythi
Copy link

mythi commented Feb 25, 2020

Looks different. For me this fails on Fedora 31 (containerd.io 1.2.12 35bd7a5f69c13e1563af8a93431411cd9ecf5021) but works on Clear

@mythi
Copy link

mythi commented Feb 27, 2020

@ZhongbaoShi any findings so far?

FWIW, the problem also triggers with --snapshotter native:

$ sudo -E ctr -n k8s.io i pull --snapshotter native docker.io/clearlinux/python:3.7
docker.io/clearlinux/python:3.7:                                                  resolved       |++++++++++++++++++++++++++++++++++++++|
manifest-sha256:98ea6980c3121ef3fbacc0f39ae2c97fa648cc914a280640134b49acc007c1bc: done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:e646dfc8e3d30e3d56a236643548b65562d3ffc59b711a10505b114ee0f278f4:    done           |++++++++++++++++++++++++++++++++++++++|
config-sha256:324349baa65b82a31001ca959c23495ccf160f5622434857ad77515001f19b38:   done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:a0c4b5f181b4adbb122a68c4c16e572a7cfea7e7cb3aad888c8e3338d265222a:    done           |++++++++++++++++++++++++++++++++++++++|
elapsed: 2.7 s                                                                    total:   0.0 B (0.0 B/s)
unpacking linux/amd64 sha256:98ea6980c3121ef3fbacc0f39ae2c97fa648cc914a280640134b49acc007c1bc...
INFO[0002] apply failure, attempting cleanup             error="failed to extract layer sha256:baa90903a66df167bb3f28a20e040afe5c7cde54de5824ccf74032270d200778: mount callback failed on /var/lib/containerd/tmpmounts/containerd-mount402275418: chmod /var/lib/containerd/tmpmounts/containerd-mount402275418/usr/bin/b2sum: no such file or directory: unknown" key="extract-456472543-9lPq sha256:baa90903a66df167bb3f28a20e040afe5c7cde54de5824ccf74032270d200778"
ctr: failed to extract layer sha256:baa90903a66df167bb3f28a20e040afe5c7cde54de5824ccf74032270d200778: mount callback failed on /var/lib/containerd/tmpmounts/containerd-mount402275418: chmod /var/lib/containerd/tmpmounts/containerd-mount402275418/usr/bin/b2sum: no such file or directory: unknown
$ sudo -E ctr -n k8s.io i pull --snapshotter native docker.io/library/python:3.7-buster
docker.io/library/python:3.7-buster:                                              resolved       |++++++++++++++++++++++++++++++++++++++|
index-sha256:10eacbe8c336fbd0343661c2f831f6a1a6164770dcc95116c1328246673c8947:    done           |++++++++++++++++++++++++++++++++++++++|
manifest-sha256:af8fc40f758a1847b87db6c0239f2a5fb70622adc95a68bf1b736fa57ad332bc: done           |++++++++++++++++++++++++++++++++++++++|
config-sha256:f66befd33669ae085c81bcf63a7d649b1f27b651b5c738861d7e144a9ffb917a:   done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:97fb33d206b17aba883603c67aa3dc75c9f97818de816ed173590b04a1ce379d:    done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:55769680e8277a4ff083d05f0993d1483b3d26b93a8814cf3c6f04fe5975ffa0:    done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:94cdd3612287b9fa01a047c4dd8e7a6a4791488d48af947ea0b422119e5e5c16:    done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:f5e195d50b880fcf8dc81f38d743c8b087777b3bfe0f27cd4ecdda513f5d20b9:    done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:dd8c6d374ea51e3dd671f71b28d025a7794ebea181b00838987d0b4d8a51372f:    done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:b4510960083997ab567642d91bca9a28f84888b48de44c88d7baf4da7fce460d:    done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:e0c7a90c35ead4b5ab084648086c59dbc7e7d2a290f54bfdf0e4f2b66d0c8311:    done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:c85513200d847a64a6e8f2cb714e2169f559b24b7736c586ff7b9aaedf71f410:    done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:50e431f790939a2f924af65084cc9d39c3d3fb9ad2d57d183b7eadf86ea46992:    done           |++++++++++++++++++++++++++++++++++++++|
elapsed: 56.8s                                                                    total:  330.7  (5.8 MiB/s)
unpacking linux/amd64 sha256:10eacbe8c336fbd0343661c2f831f6a1a6164770dcc95116c1328246673c8947...
done

@hongzhanchen
Copy link

This issue can be reproduced with ubuntu 18.04 + containerd 1.3.3 but can not be reproduced on Clear 32400 which is using containerd 1.3.0.
following is error info with ubuntu 18.04 +containerd 1.3.3:

unpacking linux/amd64 sha256:98ea6980c3121ef3fbacc0f39ae2c97fa648cc914a280640134b49acc007c1bc...
INFO[0007] apply failure, attempting cleanup error="failed to extract layer sha256:baa90903a66df167bb3f28a20e040afe5c7cde54de5824ccf74032270d200778: mount callback failed on /var/lib/containerd/tmpmounts/containerd-mount202404474: chmod /var/lib/containerd/tmpmounts/containerd-mount202404474/sbin: no such file or directory: unknown" key="extract-991688858-48Sm sha256:baa90903a66df167bb3f28a20e040afe5c7cde54de5824ccf74032270d200778"
ctr: failed to extract layer sha256:baa90903a66df167bb3f28a20e040afe5c7cde54de5824ccf74032270d200778: mount callback failed on /var/lib/containerd/tmpmounts/containerd-mount202404474: chmod /var/lib/containerd/tmpmounts/containerd-mount202404474/sbin: no such file or directory: unknown

@qzheng527
Copy link

This issue could be reproduce on Ubuntu 18.04 + containerd 1.2.10 as well.
And the issue existed for all the clearlinux container images, including the clearlinux base one.

$ sudo -E ctr -n k8s.io i pull --snapshotter native docker.io/library/clearlinux:latest
docker.io/library/clearlinux:latest: resolved |++++++++++++++++++++++++++++++++++++++|
index-sha256:3bf333df010c9800ee9a3beef94bd79cf62baeb160a8606fd5efda0934e2a70e: exists |++++++++++++++++++++++++++++++++++++++|
manifest-sha256:df277584f95ba674d979b298e185e46b94a8472e3dfa6622e31599df70220798: exists |++++++++++++++++++++++++++++++++++++++|
config-sha256:33687c1309d239059cb70f738eba3c4dc63c580a4648296fb586eca858bc55d7: exists |++++++++++++++++++++++++++++++++++++++|
layer-sha256:211c52c4b03f623579e7c9dcc311e5dd140146014a6e21e3ee888393c3a93dcd: exists |++++++++++++++++++++++++++++++++++++++|
elapsed: 12.3s total: 0.0 B (0.0 B/s)
unpacking linux/amd64 sha256:3bf333df010c9800ee9a3beef94bd79cf62baeb160a8606fd5efda0934e2a70e...
INFO[0012] apply failure, attempting cleanup error="failed to extract layer sha256:66d862308550fa6e37a287b61b6d8b0425ee01b8644574a307f3e3565ec15527: mount callback failed on /var/lib/containerd/tmpmounts/containerd-mount087888735: chmod /var/lib/containerd/tmpmounts/containerd-mount087888735/sbin: no such file or directory: unknown" key="extract-866114543-uDnf sha256:66d862308550fa6e37a287b61b6d8b0425ee01b8644574a307f3e3565ec15527"
ctr: failed to extract layer sha256:66d862308550fa6e37a287b61b6d8b0425ee01b8644574a307f3e3565ec15527: mount callback failed on /var/lib/containerd/tmpmounts/containerd-mount087888735: chmod /var/lib/containerd/tmpmounts/containerd-mount087888735/sbin: no such file or directory: unknown

@mythi
Copy link

mythi commented Mar 5, 2020

@ZhongbaoShi @hongzhanchen @qzheng527 any updates?

@ZhongbaoShi
Copy link

@mythi, Hongzhan is checking carefully what's gap between image pack by Clear and unpack by containerd. Suppose something miss match now. He will update more analysis soon.

@mythi
Copy link

mythi commented Mar 5, 2020

between image pack by Clear and unpack by containerd.

are you checking this on both Clear Linux host and Fedora/Ubuntu host?

@hongzhanchen
Copy link

I found there was similiar issue on containerd containerd/containerd#1785 which was fixed long time ago and it mentioned there was debug patch containerd/cri#463 to check the status of the rootfs after unpack. I am checking if I can use it to debug current issue.

@hongzhanchen
Copy link

I added some debug info in containerd and found that it meet error when handling link sbin but before that it handle link lib64 and bin and lib correctly, it seems that sbin link is broken?

Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: 4096/var/lib/containerd/tmpmounts/containerd-mount374528018
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz archive tar.go Apply
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/autofs
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/autofs
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname =
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=53)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/bin
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/bin
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname = usr/bin
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=50)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/boot
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/boot
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname =
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=53)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/dev
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/dev
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname =
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=53)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/etc
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/etc
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname =
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=53)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/home
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/home
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname =
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=53)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/lib
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/lib
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname = usr/lib
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=50)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/lib64
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/lib64
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname = usr/lib64
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=50)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/media
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/media
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname =
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=53)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/mnt
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/mnt
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname =
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=53)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/proc
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/proc
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname =
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=53)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/root
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/root
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname =
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=53)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/run
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/run
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname =
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=53)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz path = /var/lib/containerd/tmpmounts/containerd-mount374528018/sbin
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod path = /var/lib/containerd/tmpmounts/containerd-mount374528018/sbin
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Linkname = bin
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: handleLChmod hdr.Typeflag = %!s(uint8=49)
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz chmode------ err = chmod /var/lib/containerd/tmpmounts/containerd-mount374528018/sbin: no such file or directory
Mar 09 08:49:28 intel-Z97X-UD5H containerd[17914]: chz archive tar.go Apply err = chmod /var/lib/containerd/tmpmounts/containerd-mount374528018/sbin: no such file or directory

@bryteise
Copy link
Member

bryteise commented Mar 9, 2020

Well the hdr.Typeflag looks suspicious? Is that a broken symlink? Maybe containerd processing these doesn't create the /bin symlink to /usr/bin before the /sbin -> /bin symlink. Seems easy enough to test.

@ZhongbaoShi
Copy link

@qzheng527 did some test this afternoon.

If you try generate the clearlinux base image following https://docs.01.org/clearlinux/latest/guides/maintenance/container-image-new.html#container-image-new, you will have the same issue.

chmod /var/lib/containerd/tmpmounts/containerd-mount769753388/sbin: no such file or directory: unknown

But if you modify the “base/usr/sbin” from relative link “bin” to absolutely link “/usr/bin”, re-generate the image, you may not have the “sbin” error.
Keep going it will have another similar error for “b2sum”.
Do the same, change the relative link to absolutely link, re-generate the image, no “b2sum” error anymore.
Then “base32” error …

Use scripts to modify all the relative link to “coreutils” to absolute link “/usr/bin/coreutils”, re-generate the image, no issue anymore.

@bryteise
Copy link
Member

bryteise commented Mar 9, 2020

That's good to know. The last thing that confuses me then is that in the image (if I run on Clear Linux I can ctr run and validate this) /sbin is a symlink to usr/bin not /bin. /bin and /sbin should be exactly the same (and created in the same way in our tooling and even are the same file according to our manifests) so them being different looking at the debug logs makes me suspicious.

@bryteise
Copy link
Member

bryteise commented Mar 9, 2020

Doing a little test with Clear Linux on containerd 1.3.3 shows it working there as well.

@hongzhanchen
Copy link

hongzhanchen commented Mar 10, 2020

@bryteise, Actually, it was not easy for me to debug it out. Several days ago , I did not know go language and had not reviewed containerd and ctr code before. The log I added and printed is in function of handleLChmod of archive/tar_unix.go. The handleLChmod is called by createTarFile of archive/tar.go. Yes , as you said, sbin should be symlink. Actually , in createTarFile , it will create symlink if it is. But in problematic case , it detected sbin type is not symlink. hdr.Typeflag = 50 means that it is symblic. As you can see in my debug log, lib and bin and lib64 are all 50 but sbin is not.
Following is my debug patch for container 1.3.0 on ubuntu 18.04

diff --git a/archive/tar_unix.go b/archive/tar_unix.go
index d081351f..6feaf378 100644
--- a/archive/tar_unix.go
+++ b/archive/tar_unix.go
@@ -24,6 +24,7 @@ import (
        "strings"
        "sync"
        "syscall"
+       "fmt"

        "github.com/containerd/continuity/fs"
        "github.com/containerd/continuity/sysx"
@@ -123,6 +124,9 @@ func handleTarTypeBlockCharFifo(hdr *tar.Header, path string) error {
 }

 func handleLChmod(hdr *tar.Header, path string, hdrInfo os.FileInfo) error {
+       fmt.Printf("handleLChmod path = %s\n", path)
+       fmt.Printf("handleLChmod hdr.Linkname = %s\n", hdr.Linkname)
+       fmt.Printf("handleLChmod hdr.Typeflag = %s\n", hdr.Typeflag)
        if hdr.Typeflag == tar.TypeLink {

@mythi
Copy link

mythi commented Mar 10, 2020

Here's the fix:

diff --git a/archive/tar_unix.go b/archive/tar_unix.go
index e8721875..5325b672 100644
--- a/archive/tar_unix.go
+++ b/archive/tar_unix.go
@@ -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) {
                        if err := os.Chmod(path, hdrInfo.Mode()); err != nil {
                                return err
                        }

I'm going to submit this to upstream for review. It's not totally clear to me why the previous code worked on some setups and did not on some but the fix itself makes sense regardless:

os.Chmod follows symlinks and changes the mod bits of the file behind the link(s) so it is worth checking the file exists...

in the tar file, sbin is a hardlink to bin which is a symlink to usr/bin but for some reason bin shows up as a regular directory and so os.Chmod is called.

@bryteise
Copy link
Member

@mythi Ah I wasn't thinking about the fact we hardlink symlinks in swupd.

So the current code checks if the tar header says the path is for a symlink, then it stats the linkname (which is supposed to be the target of the symlink), checks there is no error and then checks the fileinfo mode is a symlink and only then chmods the symlink's target (which may not even exist as I don't see any tests for that). I'm a bit confused about why it is chmod'ing symlinks in the first place. As long as the linked file has the right mode, symlinks should be skipped I would think.

Your code checks if the symlink's target exists. But no longer tests the file is a symlink like the tar header says it is. So if the tar header is corrupt you'll chmod (but at least in the same way the else if case below your change does) a non-symlink which fixes the case where the file is actually a hardlink like we run into. This maybe masks over a problem in the tar file but since the code didn't raise an error before I can't really say this is worse. You then chmod the symlink as the previous code was doing.

So while I would say this is a fix, I'm still thinking that doing it at all is weird.

diff --git a/archive/tar_unix.go b/archive/tar_unix.go
index e8721875..26f8c877 100644
--- a/archive/tar_unix.go
+++ b/archive/tar_unix.go
@@ -123,15 +123,9 @@ 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.Chmod(path, hdrInfo.Mode()); err != nil {
-                               return err
-                       }
-               }
-       } else if hdr.Typeflag != tar.TypeSymlink {
-               if err := os.Chmod(path, hdrInfo.Mode()); err != nil {
-                       return err
+       if hdr.Typeflag != tar.TypeSymlink {
+               if err := os.Chmod(path, hdrInfo.Mode()); err != nil {
+                       return err
                }
        }
        return nil

Seems to work just fine in my testing.

@mythi
Copy link

mythi commented Mar 10, 2020

(which is supposed to be the target of the symlink), checks there is no error and then checks the
fileinfo mode is a symlink and only then chmods the symlink's target

AFAICS It checks the target is not a symlink and chmods that.

@bryteise
Copy link
Member

Ah yes, you are correct there sorry. Looking at the history of the file, handleLChmod is from the original implementation of the tar processing based on the OCI spec so not a lot of detail on why that test is reasonable. Looking at the spec I don't see anything really enlightening about why the test is done in that way.

@mythi
Copy link

mythi commented Mar 10, 2020

It was indeed hard to find the reasons for handleLChmod.

My patch is submitted here:
containerd/containerd#4099

Let's wait for the comments.

@bryteise
Copy link
Member

Sounds good to me. Regardless this isn't a Clear Linux bug (nor can Clear Linux fix the issue for other distros) so I'm closing this issue for now and suggest those interested follow the upstream PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants