chdir to / in parent #210
chdir to / in parent #210
Conversation
There's no reason the dumb-init process should hold on to a reference to the original cwd (but of course the child must be spawned with that cwd). One example where that can be problematic: # Terminal 1 $ mkdir -p /tmp/d/e $ sudo mount --make-shared -t tmpfs tmpfs /tmp/d/e # Terminal 2 $ docker run -v /tmp/d:/tmp/d:rshared --rm -ti --workdir /tmp/d/e some-image-with-dumb-init bash root@922ef180b49a:/tmp/d/e# cd .. root@922ef180b49a:/tmp/d# ls -l /proc/*/cwd lrwxrwxrwx 1 root root 0 Oct 1 11:25 /proc/1/cwd -> /tmp/d/e lrwxrwxrwx 1 root root 0 Oct 1 11:25 /proc/6/cwd -> /tmp/d lrwxrwxrwx 1 root root 0 Oct 1 11:25 /proc/self/cwd -> /tmp/d lrwxrwxrwx 1 root root 0 Oct 1 11:25 /proc/thread-self/cwd -> /tmp/d # Terminal 1 $ sudo umount /tmp/d/e umount: /tmp/d/e: target is busy. i.e., the application inside the container has already let go of its reference, but the host is not able to unmount /tmp/d/e because the container's pid 1 still has one. As for a test case, the 'docker run --workdir' could be emulated by something like sh -c "cd /tmp/d/e; exec $PWD/dumb-init bash -i" but one would still need root to do the mount and umount.
|
Polite ping. |
|
Hey @Villemoes, thanks for the PR! I haven't run into this issue before, but I think what you describe makes sense, and I can't think of any way this would be a breaking change. Can you think of any cases where this would cause issues for existing use-cases? My thinking is that as long as the Before merging it'd be nice to add a small test. Maybe we can adapt an existing test (e.g. this one) and have the test look at the working directory of the spawned dumb-init process? Looking at |
No, I don't think anything can rely on what cwd of the dumb-init process is. If chdir("/") fails, something is catastrophically wrong on the machine, so things will fall apart soon enough, but yes, I deliberately didn't make it an error.
Yeah, it should be possible to do something like in python and check that stdout is as expected (i.e. both that the dumb-init process chdir'ed, but also that the child process kept the original cwd). |
I added a patch adding such a test case. |
tests/cwd_test.py
Outdated
| # We need absolute path to dumb-init since we pass cwd=/tmp to get | ||
| # predictable output - so we can't rely on dumb-init being found | ||
| # in the "." directory. | ||
| proc = run((os.path.join(os.getcwd(), 'dumb-init'), |
There was a problem hiding this comment.
I think this line fails under tox because dumb-init will be installed into the virtualenv's bin directory. You can see the failure in Travis:
=================================== FAILURES ===================================
________________________ test_working_directories[1-1] _________________________
@pytest.mark.usefixtures('both_debug_modes', 'both_setsid_modes')
def test_working_directories():
"""The child process must start in the working directory in which
dumb-init was invoked, but dumb-init itself should not keep a
reference to that."""
# We need absolute path to dumb-init since we pass cwd=/tmp to get
# predictable output - so we can't rely on dumb-init being found
# in the "." directory.
proc = run((os.path.join(os.getcwd(), 'dumb-init'),
'sh', '-c', 'readlink /proc/$PPID/cwd && readlink /proc/$$/cwd'),
> cwd="/tmp", stdout=PIPE, stderr=PIPE)
tests/cwd_test.py:17:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/usr/lib/python3.7/subprocess.py:472: in run
with Popen(*popenargs, **kwargs) as process:
/usr/lib/python3.7/subprocess.py:775: in __init__
restore_signals, start_new_session)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <subprocess.Popen object at 0x7f25371729e8>
args = ['/test/dumb-init', 'sh', '-c', 'readlink /proc/$PPID/cwd && readlink /proc/$$/cwd']
executable = b'/test/dumb-init', preexec_fn = None, close_fds = True
pass_fds = (), cwd = '/tmp', env = None, startupinfo = None, creationflags = 0
shell = False, p2cread = -1, p2cwrite = -1, c2pread = 11, c2pwrite = 12
errread = 13, errwrite = 14, restore_signals = True, start_new_session = False
def _execute_child(self, args, executable, preexec_fn, close_fds,
pass_fds, cwd, env,
startupinfo, creationflags, shell,
p2cread, p2cwrite,
c2pread, c2pwrite,
errread, errwrite,
restore_signals, start_new_session):
"""Execute program (POSIX version)"""
[...]
> raise child_exception_type(errno_num, err_msg, err_filename)
E FileNotFoundError: [Errno 2] No such file or directory: '/test/dumb-init': '/test/dumb-init'
(from https://travis-ci.org/github/Yelp/dumb-init/jobs/747386344)
Maybe you can use shutil.which to get the path instead? https://docs.python.org/3/library/shutil.html#shutil.which
There was a problem hiding this comment.
Hm, yes, with a grain of os.path.realpath added - shutil.which just returns ./dumb-init when . is in path and dumb-init does exist in .. Let's try this one then.
|
Looks good to me, thanks! I'll release this as v1.2.5 in a few hours once the Travis builds are done. |
There's no reason the dumb-init process should hold on to a reference
to the original cwd (but of course the child must be spawned with that
cwd). One example where that can be problematic:
i.e., the application inside the container has already let go of its
reference, but the host is not able to unmount /tmp/d/e because the
container's pid 1 still has one.
As for a test case, the 'docker run --workdir' could be emulated by
something like
sh -c "cd /tmp/d/e; exec $PWD/dumb-init bash -i"
but one would still need root to do the mount and umount.