fixuid does not handle errors from filepath.Walk

[tomeon]

Hi! First, thanks for creating this eminently useful utility -- container bind mounts can be such a pain, and anything that makes dealing with mounts easier is most, most welcome.

I am hitting a fatal error when using fixuid as the entrypoint for a container that's based on the ruby:2.4 image. Here's an overview of my environment:

$ docker version
Client:
 Version:       17.12.0-ce
 API version:   1.35
 Go version:    go1.9.2
 Git commit:    c97c6d6
 Built: Wed Dec 27 20:10:14 2017
 OS/Arch:       linux/amd64

Server:
 Engine:
  Version:      17.12.0-ce
  API version:  1.35 (minimum version 1.12)
  Go version:   go1.9.2
  Git commit:   c97c6d6
  Built:        Wed Dec 27 20:12:46 2017
  OS/Arch:      linux/amd64
  Experimental: false
$ uname -a
Linux mydomain.com 3.10.0-693.11.1.el7.x86_64 #1 SMP Mon Dec 4 23:52:40 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
$ cat /etc/os-release
NAME="CentOS Linux"
VERSION="7 (Core)"
ID="centos"
ID_LIKE="rhel fedora"
VERSION_ID="7"
PRETTY_NAME="CentOS Linux 7 (Core)"
ANSI_COLOR="0;31"
CPE_NAME="cpe:/o:centos:centos:7"
HOME_URL="https://www.centos.org/"
BUG_REPORT_URL="https://bugs.centos.org/"

CENTOS_MANTISBT_PROJECT="CentOS-7"
CENTOS_MANTISBT_PROJECT_VERSION="7"
REDHAT_SUPPORT_PRODUCT="centos"
REDHAT_SUPPORT_PRODUCT_VERSION="7"

And here are the two Dockerfiles involved in the build:

FROM debian:jessie                                                                                                                         
                                                                                                                                           
ARG FIXUID_VERSION                                                                                                                         
ENV FIXUID_VERSION 0.2                                                                                                                     
                                                                                                                                           
ADD "https://github.com/boxboat/fixuid/releases/download/v${FIXUID_VERSION}/fixuid-${FIXUID_VERSION}-linux-amd64.tar.gz" /tmp/fixuid.tar.gz
                                                                                                                                           
RUN     tar -C /usr/local/bin -xvf /tmp/fixuid.tar.gz \                                                                                    
    &&  rm -f /tmp/fixuid.tar.gz \                                                                                                         
    &&  chmod u+sx /usr/local/bin/fixuid                                                                                                   
                                                                                                                                           
ENTRYPOINT ["/usr/local/bin/fixuid"]                                                                                                       

FROM custom/fixuid:custom AS fixuid                           
                                                              
FROM ruby:2.4                                                 
                                                                  
COPY --from=fixuid /usr/local/bin/fixuid /usr/local/bin/fixuid

# More stuff below

When I try to run a container based off of an image built from the second Dockerfile, I get a stack trace:

$ docker run --rm -it -u 1002:100 custom/ruby:2.4 bash
fixuid: fixuid should only ever be used on development systems. DO NOT USE IN PRODUCT
fixuid: updating user 'auser' to UID '1002                                         
fixuid: runtime GID '100' matches existing group 'agroup'; not changing GID           
panic: runtime error: invalid memory address or nil pointer dereference              
[signal SIGSEGV: segmentation violation code=0x1 addr=0x48 pc=0x522b30]              
                                                                                     
goroutine 1 [running]:                                                               
main.main.func1(0xc4201074c0, 0x18, 0x0, 0x0, 0x613600, 0xc420080ba0, 0x0, 0x0)      
        /home/caleb/go/src/github.com/boxboat/fixuid/fixuid.go:185 +0x60             
path/filepath.walk(0xc420102515, 0x9, 0x614a80, 0xc42011c750, 0xc420016ba0, 0x0, 0x0)
        /usr/local/go/src/path/filepath/path.go:377 +0x2eb                           
path/filepath.walk(0xc420015850, 0x4, 0x614a80, 0xc4200f2750, 0xc420016ba0, 0x0, 0x0)
        /usr/local/go/src/path/filepath/path.go:381 +0x39a                           
path/filepath.walk(0x569e59, 0x1, 0x614a80, 0xc4200828f0, 0xc420016ba0, 0x0, 0x30)   
        /usr/local/go/src/path/filepath/path.go:381 +0x39a                           
path/filepath.Walk(0x569e59, 0x1, 0xc420016ba0, 0xc420082820, 0x0)                   
        /usr/local/go/src/path/filepath/path.go:403 +0x11d                           
main.main()                                                                          
        /home/caleb/go/src/github.com/boxboat/fixuid/fixuid.go:215 +0x1349           

This seems to result from the fact that fixuid does not (explicitly) handle errors passed to the filepath.WalkFunc by filepath.Walk. I compiled a patched version of fixuid with the following change:

// this function is called for every file visited                      
visit := func(filePath string, fileInfo os.FileInfo, err error) error {
    // bail out on walk error
    if err != nil {                                                    
        logger.Fatalln(err)                                            
    }

When I rebuild the two Docker images using this patched fixuid, I instead get the following error:

docker run --rm -it -u 1002:100 custom/ruby:2.4 bash
fixuid: fixuid should only ever be used on development systems. DO NOT USE IN PRODUCTION
fixuid: updating user 'auser' to UID '1002
fixuid: updating group 'agroup' to GID '1010
fixuid: lstat /etc/dpkg/shlibs.default: no such file or directory

And, indeed:

docker run --entrypoint '' --rm -it -u 1002:100 custom/ruby:2.4 ls -l /etc/dpkg
ls: cannot access /etc/dpkg/shlibs.default: No such file or directory
ls: cannot access /etc/dpkg/shlibs.override: No such file or directory
total 4
-rw-r--r-- 1 root root 446 Nov 28  2014 dpkg.cfg
drwxr-xr-x 2 root root  31 Dec 10 00:00 dpkg.cfg.d
drwxr-xr-x 2 root root  33 Dec 10 00:00 origins
?????????? ? ?    ?      ?            ? shlibs.default
?????????? ? ?    ?      ?            ? shlibs.override

I'm not quite sure what the deal is with the ruby:2.4 image (shlibs.default and shlibs.override have the same weird ls output in both custom/ruby:2.4 and the upstream ruby:2.4), but in any case it would be great if fixuid could handle errors like missing files that nonetheless "show up" (?) when walking the filesystem hierarchy. I'm not a Gopher, but I'd be happy to submit a PR if I can get an idea of what fixuid should do (log and bail out? blithely ignore?) in scenarios like the one I've described.

Thanks very much!

[caleblloyd]

Thanks for reporting the issue and finding the root cause!

Reviewing the filepath.walk in path.go, it appears that the error passed to walkFn can be triggered from an error in the lstat syscall or an error in filepath.readDirNames. Looks like you hit the former.

I think that logging the error and continuing seems appropriate. Something like:

// this function is called for every file visited                      
visit := func(filePath string, fileInfo os.FileInfo, err error) error {
    if err != nil {
        logger.Println("error when visiting " + filePath)
        logger.Println(err)
        return nil
    }
...

In the lstat case, filepath.walk doesn't even know if the path is a directory, so it will continue upon receiving a nil response from the walkFn. In the filepath.readDirNames case, filepath.walk knows it's in the directory but skips recursion upon receiving a nil response from the walkFn

I can make the change and get a new package published within the next day or so. I'm not sure how to get an inode into this state to add a unit test though

[tomeon]

Thanks for your prompt attention to this, @caleblloyd! FWIW, it looks like the strange ls output (and other weird behavior not described my original comment) traces to the fact that I had been using the overlay2 storage driver on an XFS filesystem that doesn't support d_type, in contravention of the overlay2 driver docs. I'm now using a correctly-configured XFS filesystem, and the issue has gone away.