[lxc-devel] [PATCH v3] fix console stdin,stdout,stderr fds

[Serge Hallyn]

Quoting Dwight Engen (dwight.engen at oracle.com):
> The fds for stdin,stdout,stderr that we were leaving open for /sbin/init
> in the container were those from /dev/tty or lxc.console (if given), which
> wasn't right. Inside the container it should only have access to the pty
> that lxc creates representing the console.
> 
> This was noticed because busybox's init was resetting the termio on its
> stdin which was effecting the actual users terminal instead of the pty.
> This meant it was setting icanon so were were not passing keystrokes
> immediately to the pty, and hence command line history/editing wasn't
> working.
> 
> Fix by dup'ing the console pty to stdin,stdout,stderr just before
> exec()ing /sbin/init. Fix fd leak in error handling that I noticed while
> going through this code.
> 
> Also tested with lxc.console = none, lxc.console = /dev/tty7 and no
> lxc.console specified.
> 
> V2: The first version was getting EBADF sometimes on dup2() because
> lxc_console_set_stdfds() was being called after lxc_check_inherited()
> had already closed the fds for the pty. Fix by calling
> lxc_check_inherited() as late as possible which also extends coverage
> of open fd checked code.
> 
> V3: Don't move lxc_check_inherited() since it needs to be called while
> the tmp proc mount is still mounted. Move call to lxc_console_set_stdfds()
> just before it.
> 
> Signed-off-by: Dwight Engen <dwight.engen at oracle.com>

looks good, thanks!

Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>

> ---
>  src/lxc/console.c     | 18 +++++++++++++++++-
>  src/lxc/console.h     |  1 +
>  src/lxc/lxc_console.c |  1 -
>  src/lxc/lxc_start.c   |  1 -
>  src/lxc/start.c       | 10 +++++++++-
>  5 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/src/lxc/console.c b/src/lxc/console.c
> index 6bfc8a3..67e5d0f 100644
> --- a/src/lxc/console.c
> +++ b/src/lxc/console.c
> @@ -506,7 +506,7 @@ static void lxc_console_peer_default(struct lxc_console *console)
>  	DEBUG("using '%s' as console", path);
>  
>  	if (!isatty(console->peer))
> -		return;
> +		goto err1;
>  
>  	ts = lxc_console_sigwinch_init(console->peer, console->master);
>  	if (!ts)
> @@ -611,7 +611,23 @@ err:
>  	return -1;
>  }
>  
> +int lxc_console_set_stdfds(struct lxc_handler *handler)
> +{
> +	struct lxc_conf *conf = handler->conf;
> +	struct lxc_console *console = &conf->console;
> +
> +	if (console->slave < 0)
> +		return 0;
>  
> +	if (dup2(console->slave, 0) < 0 ||
> +	    dup2(console->slave, 1) < 0 ||
> +	    dup2(console->slave, 2) < 0)
> +	{
> +		SYSERROR("failed to dup console");
> +		return -1;
> +	}
> +	return 0;
> +}
>  
>  static int lxc_console_cb_tty_stdin(int fd, uint32_t events, void *cbdata,
>  				    struct lxc_epoll_descr *descr)
> diff --git a/src/lxc/console.h b/src/lxc/console.h
> index d45260c..eb3894b 100644
> --- a/src/lxc/console.h
> +++ b/src/lxc/console.h
> @@ -36,3 +36,4 @@ extern int  lxc_console(struct lxc_container *c, int ttynum,
>  		        int escape);
>  extern int  lxc_console_getfd(struct lxc_container *c, int *ttynum,
>  			      int *masterfd);
> +extern int  lxc_console_set_stdfds(struct lxc_handler *);
> diff --git a/src/lxc/lxc_console.c b/src/lxc/lxc_console.c
> index bfee6fb..0fd08e8 100644
> --- a/src/lxc/lxc_console.c
> +++ b/src/lxc/lxc_console.c
> @@ -28,7 +28,6 @@
>  #include <errno.h>
>  #include <string.h>
>  #include <fcntl.h>
> -#include <termios.h>
>  #include <unistd.h>
>  #include <signal.h>
>  #include <libgen.h>
> diff --git a/src/lxc/lxc_start.c b/src/lxc/lxc_start.c
> index 05fb161..d9a5694 100644
> --- a/src/lxc/lxc_start.c
> +++ b/src/lxc/lxc_start.c
> @@ -27,7 +27,6 @@
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <string.h>
> -#include <termios.h>
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <signal.h>
> diff --git a/src/lxc/start.c b/src/lxc/start.c
> index 6b57ee1..ec10496 100644
> --- a/src/lxc/start.c
> +++ b/src/lxc/start.c
> @@ -31,7 +31,6 @@
>  #include <unistd.h>
>  #include <signal.h>
>  #include <fcntl.h>
> -#include <termios.h>
>  #include <grp.h>
>  #include <poll.h>
>  #include <sys/param.h>
> @@ -697,6 +696,15 @@ static int do_start(void *data)
>  	if (lsm_process_label_set(lsm_label, 1, 1) < 0)
>  		goto out_warn_father;
>  
> +	/* Some init's such as busybox will set sane tty settings on stdin,
> +	 * stdout, stderr which it thinks is the console. We already set them
> +	 * the way we wanted on the real terminal, and we want init to do its
> +	 * setup on its console ie. the pty allocated in lxc_console_create()
> +	 * so make sure that that pty is stdin,stdout,stderr.
> +	 */
> +	if (lxc_console_set_stdfds(handler) < 0)
> +		goto out_warn_father;
> +
>  	if (lxc_check_inherited(handler->conf, handler->sigfd))
>  		return -1;
>  
> -- 
> 1.8.5.3
>