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

WIP - ignore * when reading from /dev/console #1434

Conversation

TomasTomecek
Copy link

This is my attempt to initiate fix of moby/moby#27202. This PR started from code posted inside this comment: moby/moby#27202 (comment)

Using the current code I just wanted to make sure that if I ignore sighups (actually all errors), whether I get output from systemd. I did. The next step should be to decide how to fix this issue. Probably the best way would be to use epoll, which would obviously work only on linux: not sure whether if running on linux we should use epoll and other systems should use current upstream solution.

The issue is that systemd closes /dev/console because of kernel SAK -- it's arguable whether this behaviour is a bug/feature in kernel, systemd or runc.

Signed-off-by: Tomas Tomecek [email protected]

@cyphar
Copy link
Member

cyphar commented May 3, 2017

Eugh, I really don't like that we have to rewrite io.Copy. 😞

@cyphar
Copy link
Member

cyphar commented May 3, 2017

As an aside, while the /dev/console behaviour might make sense on the host, it makes literally no sense inside a container -- since /dev/console isn't actually the "console" proper. It's just a symlink we use because some legacy apps want to write to /dev/console for some stupid reason (such as systemd it seems). And I don't know of any container runtime that bindmounts the host's /dev/console -- because it would be a horrific security issue.

So (as much as I hate punting issues like this because the systemd folks tend to get annoyed) why can't systemd only enable this SAK security behaviour if /dev/console is actually a console (it has the major and minor numbers of a console)?

@cyphar
Copy link
Member

cyphar commented May 3, 2017

I've left a comment on the systemd issue.

However, one more thing to note is that changing this here won't actually fix the bug. When you run runc in a detached mode (like containerd does) then runc doesn't stick around -- the file descriptors are handed to the parent process and are managed by it (this is especially true as of #1018 which I don't think Docker has updated to yet). So you would also need to fix this problem there.

@TomasTomecek
Copy link
Author

@cyphar thank you for commenting inside the systemd issue. As I understand this now, both sides resist to fix the issue in their codebases because you and Lennart think it is a hack, is this correct? I'm not sure how to proceed.

This moby/moby#9212 seems to be the same issue as the systemd issue. So Lennart seems to be correct that every init has this behaviour.

@crosbymichael
Copy link
Member

Is there a reason why you even run systemd with a --tty? Just run it without and let it create a /dev/console?

@TomasTomecek
Copy link
Author

I would love to see output from systemd. systemd acts as a true daemon and "closes" its std{in,out,err}:

[root@runc /]# ls -lha /proc/1/fd
total 0
dr-x------ 2 root root  0 May  9 09:29 .
dr-xr-xr-x 9 root root  0 May  9 09:29 ..
lrwx------ 1 root root 64 May  9 09:29 0 -> /dev/null
lrwx------ 1 root root 64 May  9 09:29 1 -> /dev/null
lrwx------ 1 root root 64 May  9 09:29 2 -> /dev/null

Hence /dev/console is likely the only way to get logs from systemd.

@hqhq
Copy link
Contributor

hqhq commented May 12, 2017

TBH we are using the same hack in our internal version but for containerd-shim, I don't quite like it though.

@TomasTomecek
Copy link
Author

closing in favor of #1446

dqminh added a commit to dqminh/containerd that referenced this pull request Jul 30, 2017
this adds a `platform` interface for shim service to manage platform-specific
behaviors such as I/O (which uses epoll in linux to work around bugs with applications
that closes all consoles i.e. opencontainers/runc#1434
and moby/moby#27202)

Its expected that we only have 1 epollfd per containerd_shim to manage all processes.
Since all the work are done outside of the container runtime, upgrading of runc
is not required and should be done separately.

Signed-off-by: Daniel Dao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants