Bug 70290 - [systemd-nspawn] Doesn't correctly handle std{in,out} not being the same file, and not being a TTY
Summary: [systemd-nspawn] Doesn't correctly handle std{in,out} not being the same file...
Alias: None
Product: systemd
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: systemd-bugs
QA Contact: systemd-bugs
Depends on:
Reported: 2013-10-08 17:49 UTC by Luke Shumaker
Modified: 2013-12-13 04:10 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:

A patch fixing bugs 1-2; stdout-redirection. (5.65 KB, patch)
2013-10-08 19:18 UTC, Luke Shumaker
Details | Splinter Review
Fix for stdout redirection (bugs 1-2) (6.38 KB, patch)
2013-10-09 18:20 UTC, Luke Shumaker
Details | Splinter Review

Description Luke Shumaker 2013-10-08 17:49:17 UTC
This is actually a large, compound bug that results in "it doesn't do the right thing if you redirect stdio".

Bugs 1-2 mean redirecting stdout doesn't work correctly.
Bug 3 means that redirecting/closing stdin doesn't work correctly.
(possible) Bug 4 means that even if bug 3 is fixed, redirecting stdin doesn't work as expected.
(possible) Bug 5 means that even if bugs 1-2 are fixed, redirecting stdout doesn't work as expected.

A quick note on nomenclature: systemd-nspawn creates/acquires a PTY that is used for /dev/console in the chroot. I will refer to this as the "proxy PTY".

Bug 1: It bases the dimensions of the proxy PTY off of the dimensions of stdin, not stdout.

This is a 1-line change, to change `ioctl(STDIN_FILENO, TIOCGWINSZ, &ws)` to use `STDOUT_FILENO`.

Bug 2: It sets the output properties of stdin (if it is a TTY).

This is wrong if stdin and stdout are not the same file.  It gets stdin's terminal attributes (tcgetattr), runs the through cfmakeraw, and saves them back (tcsetattr).  The problem is that cfmakeraw sets both input and output attributes.  The simplest work-around is to set `raw_attr.c_oflag = saved_attr.c_oflag` after running `cfmakeraw(&raw_attr)`.

(PS: it also unset the ECHO local flag on raw_attr after running it through cfmakeraw, which already did that.  That line was superfluous.)

But then, the output properties of stdout are not set--we must then do a similar process for it, but resetting attr.c_iflag, attr.c_lflag, and attr.c_cc.

We must also, set one before starting to set the other (set stdin before reading the current values for stdout), or the second write will reset the first one.

Bug 3: If stdin is not a TTY, it has no form of signal handling.

(the following applies to all signals that a TTY may generate, but I will only speak of SIGINT)

The "signal handling" currently employed is "hey, stdin, if you see C-c, don't send me SIGINT, just let me read C-c as input" (through the call to cfmakeraw mentioned above).  It then passes C-c to the proxy PTY, which translates it into a SIGINT to be sent to the child process.

This means two things: 1) If stdin isn't a TTY, it's "signal handling" won't work. 2) If it receives a "signal" from anything other than a terminal, it won't respond correctly.

So, what happens if the signal handling isn't working.  When it receives SIGINT, it dies, it stops proxying data between the proxy PTY and stdin/stdout.  This causes either the child process to starve on stdin, or fill the proxy PTY's buffer and block.  Either way, it causes an I/O block, and the child process(es) will need to be kill(1)ed.

The simplest work-around would be to open /dev/tty, and set *that* to not interpret C-c as SIGINT; that way the TTY is set even if it isn't stdin.  However, that only fixes consequence 1 above, not consequence 2.

(possible) Bug 4: If stdin is not a TTY, it does not read stdin.
(possible) Bug 5: If stdout is not a TTY, it represents stdout as a TTY to the child process.

In my opinion, the correct behaviour here would be to set up the proxy PTY as /dev/console in the chroot, but not set it to stdin/out/err.  Instead, open a pipe that is used to proxy between the child process' I/O, and systemd-nspawn's I/O.

Whether or not you agree with that, if stdin is open, I don't think it's reasonable to ignore it.
Comment 1 Luke Shumaker 2013-10-08 18:18:18 UTC
Actually, I don't think it is necessary to reset attr.c_cc.
Comment 2 Luke Shumaker 2013-10-08 19:18:06 UTC
Created attachment 87300 [details] [review]
A patch fixing bugs 1-2; stdout-redirection.

This is a patch of two git commits that fix bugs 1 and 2, respectively.  This means that stdout redirection works.
Comment 3 Luke Shumaker 2013-10-08 19:28:38 UTC
Playing with the patch I just uploaded, I noticed a slight issue it created:

Information written to stderr is now subject to raw terminal interpretation.

This can be fixed by appending '\r' to the log messages (ugly), or by resetting stdin right after the current settings for stdout are read, then setting them both later (where it was set before the patch) (less ugly).
Comment 4 Luke Shumaker 2013-10-09 18:20:41 UTC
Created attachment 87351 [details] [review]
Fix for stdout redirection (bugs 1-2)

This fixes the bug in the last patch described in my comment; it uses the less ugly method of resetting stdin after stdout has been detected.
Comment 5 Zbigniew Jedrzejewski-Szmek 2013-12-13 04:10:10 UTC
Patches applied as eaf73b0616 and 90d14d2015.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.