Fix run/exec with terminal: true in case stdin is not a terminal #2535
Conversation
tty.go
Outdated
| } | ||
| if err := stdin.SetRaw(); err != nil { | ||
| if hostConsole == nil { | ||
| return errors.New("unable to find console (are all streams redirected?)") |
There was a problem hiding this comment.
I think a nicer solution might be to just not do interrupt or resize handling if there isn't a console, but still allow the container to run. I don't really have an opinion on whether we should check stdout or stderr ... it feels a little wrong because Ctrl-C feels like an "input" thing but 🤷.
There was a problem hiding this comment.
We need a terminal to set the raw mode etc -- and any terminal we have will do. Tools like clear and tput do the same thing -- they try stderr, stdout, stdin, and if all three fails, open("/dev/tty") should do the same thing.
I didn't do open("/dev/tty") as I am not sure where to close it, but I think I'll implement that later, to make cases likerunc run $ID top </dev/null >/dev/null 2>&1work.
There was a problem hiding this comment.
not do interrupt or resize handling if there isn't a console, but still allow the container to run
I disagree. If we say in config we want a console, and there's no [host] console we should bail out. This is what my current code does (see #2535 (comment))
kolyshkin
force-pushed
the
no-term-on-stdin
branch
from
a99bf55 to
ea7b1ff
Compare
|
Reworked to open("/dev/tty") as a last resort. Without it, In case we don't have a terminal at all, runc should fail gracefully and early. Tested it without a terminal (which is kinda hard -- the only way I know is to use [root@kir-rhat runc]# cat /root/runme
cd /home/kir/go/src/github.com/opencontainers/runc
./runc run -b tst xxx-$$
echo $?
[root@kir-rhat runc]# ssh -T root@localhost ./runme
time="2020-07-31T18:51:55-07:00" level=error msg="open /dev/tty: no such device or address"
1
[root@kir-rhat runc]# (update: fixed test case output) |
kolyshkin
changed the title
|
@cyphar @crosbymichael PTAL |
|
@AkihiroSuda @mrunalp @cyphar PTAL |
|
LGTM |
This fixes the following failure: > sudo runc run -b bundle ctr </dev/null > WARN[0000] exit status 2 > ERRO[0000] container_linux.go:367: starting container process caused: process_linux.go:459: container init caused: The "exit status 2" with no error message is caused by SIGHUP which is sent to init by the kernel when we are losing the controlling terminal. If we choose to ignore that, we'll get panic in console.Current(), which is addressed by [1]. Otherwise, the issue here is simple: the code assumes stdin is opened to a terminal, and fails to work otherwise. Some standard Linux tools (e.g. stty, top) do the same (modulo panic), while some others (reset, tput) use the trick of trying all the three std streams (starting with stderr as it is least likely to be redirected), and if all three fails, open /dev/tty. This commit does a similar thing (see initHostConsole). It also replaces the call to console.Current(), which may panic (see [1]), by reusing the t.hostConsole. Finally, a simple test case is added. Fixes: opencontainers#2485 [1] containerd/console#37 Signed-off-by: Kir Kolyshkin <[email protected]>
Note that stdout/stderr are already redirected by bats' `run` command, so the only way to get a controlling terminal is to open /dev/tty (which fails if there isn't one). Here's how I tested the failure to open /dev/tty: > [root@kir-rhat ~]# ssh -T root@localhost cat ./runme > cd /home/kir/go/src/github.com/opencontainers/runc > ./runc run -b tst xxx-$$ > echo $? > > [root@kir-rhat ~]# ssh -T root@localhost ./runme > time="2020-07-31T16:35:47-07:00" level=error msg="chdir tst: no such file or directory" > 1 If anyone knows how to obtain an tty-less environment without using ssh -T, please raise your hand. Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
no-term-on-stdin
branch
from
d1a3550 to
42d9a6b
Compare
@crosbymichael we no longer use pullapprove for this repo, so you have to use github-native LGTM (i.e. go to 'commits' or 'file changes', click the 'review changes', select the 'approve' radio button...) |
This fixes the following failure (and the likes):
The "exit status 2" with no error message is caused by SIGHUP
which is sent to init by the kernel when we are losing the
controlling terminal. If we choose to ignore that, we get
the same result, this time caused by panic in console.Current(),
which is addressed by [1].
Otherwise, the issue here is simple: the code assumes stdin
is opened to a terminal, and fails to work otherwise. Some
standard Linux tools (e.g. stty, top) do the same (modulo panic),
while some others (reset, tput) use the trick of trying
all the three std streams (starting with stderr as it is least likely
to be redirected), and if all three fails, open /dev/tty.
This commit does a similar thing (see
initHostConsole).It also replaces the call to console.Current(), which may panic
(see [1]), by reusing the t.hostConsole.
Finally, a couple of simple test cases are added.
Alas, I'm not sure how to automate the test case when runc is
run without a controlling terminal. I have reproduced it manually:
(which is much better than mysterious "container init caused" error, right?)
[1] containerd/console#37
Fixes: #2485