Re: [Xen-devel] [PATCH] Fix xenconsole's "Could not read tty from store"

[Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>]

Any conclusion on this one, before I do a second release candidate for
3.2.0?

 -- Keir


On 18/12/07 18:52, "John Levon" <levon@xxxxxxxxxxxxxxxxx> wrote:

> On Tue, Dec 18, 2007 at 06:18:16PM +0000, Samuel Thibault wrote:
> 
>> To my understanding, from the server side tcsetattr should only be
>> performed on the master side (with effect on the slave side too).
> 
> Here's another try, what does this do on Linux? My test program works
> without needing the tcsetattr, as does xenconsoled
> 
> thanks
> john
> 
> 
> # HG changeset patch
> # User john.levon@xxxxxxx
> # Date 1198003657 28800
> # Node ID 6f03f4ec458d4780314c015da214795b7b1cf195
> # Parent  539cbabd97b5ff3d335de151636040bb2f4cd629
> Fix master/slave handling in xenconsoled
> 
> Fix a number of problems with the pty handling:
> 
> - make openpty() implementation work on Solaris
> - set raw on the slave fd, not the master, as the master doesn't
>   have a line discipline pushed on Solaris
> - make sure we don't leak the slave fd returned from openpty()
> - don't use the 'name' argument of openpty() as it's a security risk
> - note behaviour of a zero read of the master on Solaris
> - remove pointless tcget/setattr
> 
> Signed-off-by: John Levon <john.levon@xxxxxxx>
> Signed-off-by: Samuel Thibault <samuel.thibault@xxxxxxxxxx>
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -36,10 +36,14 @@
>  #include <stdarg.h>
>  #include <sys/mman.h>
>  #include <sys/time.h>
> +#include <assert.h>
>  #if defined(__NetBSD__) || defined(__OpenBSD__)
>  #include <util.h>
>  #elif defined(__linux__) || defined(__Linux__)
>  #include <pty.h>
> +#endif
> +#if defined(__sun__)
> +#include <stropts.h>
>  #endif
>  
>  #define MAX(a, b) (((a) > (b)) ? (a) : (b))
> @@ -75,7 +79,8 @@ struct domain
>  struct domain
>  {
> int domid;
> - int tty_fd;
> + int master_fd;
> + int slave_fd;
> int log_fd;
> bool is_dead;
> struct buffer buffer;
> @@ -227,77 +232,90 @@ static int create_domain_log(struct doma
> return fd;
>  }
>  
> +static void domain_close_tty(struct domain *dom)
> +{
> + if (dom->master_fd != -1) {
> +  close(dom->master_fd);
> +  dom->master_fd = -1;
> + }
> +
> + if (dom->slave_fd != -1) {
> +  close(dom->slave_fd);
> +  dom->slave_fd = -1;
> + }
> +}
> +
>  #ifdef __sun__
>  /* Once Solaris has openpty(), this is going to be removed. */
> -int openpty(int *amaster, int *aslave, char *name,
> -     struct termios *termp, struct winsize *winp)
> +static int openpty(int *amaster, int *aslave, char *name,
> +                   struct termios *termp, struct winsize *winp)
>  {
> - int mfd, sfd;
> + const char *slave;
> + int mfd = -1, sfd = -1;
>  
> *amaster = *aslave = -1;
> - mfd = sfd = -1;
>  
> mfd = open("/dev/ptmx", O_RDWR | O_NOCTTY);
> if (mfd < 0)
> -  goto err0;
> +  goto err;
>  
> if (grantpt(mfd) == -1 || unlockpt(mfd) == -1)
> -  goto err1;
> +  goto err;
>  
> - /* This does not match openpty specification,
> -  * but as long as this does not hurt, this is acceptable.
> -  */
> - mfd = sfd;
> + if ((slave = ptsname(mfd)) == NULL)
> +  goto err;
>  
> - if (termp != NULL && tcgetattr(sfd, termp) < 0)
> -  goto err1; 
> + if ((sfd = open(slave, O_RDONLY | O_NOCTTY)) == -1)
> +  goto err;
> +
> + if (ioctl(sfd, I_PUSH, "ptem") == -1)
> +  goto err;
>  
> if (amaster)
> *amaster = mfd;
> if (aslave)
> *aslave = sfd;
> - if (name)
> -  strlcpy(name, ptsname(mfd), sizeof(slave));
> if (winp)
> ioctl(sfd, TIOCSWINSZ, winp);
>  
> + assert(name == NULL);
> + assert(termp == NULL);
> +
> return 0;
>  
> -err1:
> +err:
> + if (sfd != -1)
> +  close(sfd);
> close(mfd);
> -err0:
> return -1;
>  }
>  #endif
>  
> -
>  static int domain_create_tty(struct domain *dom)
>  {
> - char slave[80];
> - struct termios term;
> + const char *slave;
> char *path;
> - int master, slavefd;
> int err;
> bool success;
> char *data;
> unsigned int len;
>  
> - if (openpty(&master, &slavefd, slave, &term, NULL) < 0) {
> -  master = -1;
> + assert(dom->slave_fd == -1);
> + assert(dom->master_fd == -1);
> +
> + if (openpty(&dom->master_fd, &dom->slave_fd, NULL, NULL, NULL) < 0) {
> err = errno;
> dolog(LOG_ERR, "Failed to create tty for domain-%d (errno = %i, %s)",
>      dom->domid, err, strerror(err));
> -  return master;
> +  return 0;
> }
>  
> - cfmakeraw(&term);
> - if (tcsetattr(master, TCSAFLUSH, &term) < 0) {
> + if ((slave = ptsname(dom->master_fd)) == NULL) {
> err = errno;
> -  dolog(LOG_ERR, "Failed to set tty attribute  for domain-%d (errno = %i,
> %s)",
> +  dolog(LOG_ERR, "Failed to get slave name for domain-%d (errno = %i, %s)",
>      dom->domid, err, strerror(err));
> goto out;
> }
> -
>  
> if (dom->use_consolepath) {
> success = asprintf(&path, "%s/limit", dom->conspath) !=
> @@ -340,15 +358,15 @@ static int domain_create_tty(struct doma
> goto out;
> }
>  
> - if (fcntl(master, F_SETFL, O_NONBLOCK) == -1)
> + if (fcntl(dom->master_fd, F_SETFL, O_NONBLOCK) == -1)
> goto out;
>  
> - return master;
> - out:
> - close(master);
> - return -1;
> + return 1;
> +out:
> + domain_close_tty(dom);
> + return 0;
>  }
> -
> + 
>  /* Takes tuples of names, scanf-style args, and void **, NULL terminated. */
>  int xs_gather(struct xs_handle *xs, const char *dir, ...)
>  {
> @@ -454,10 +472,8 @@ static int domain_create_ring(struct dom
> dom->local_port = rc;
> dom->remote_port = remote_port;
>  
> - if (dom->tty_fd == -1) {
> -  dom->tty_fd = domain_create_tty(dom);
> -
> -  if (dom->tty_fd == -1) {
> + if (dom->master_fd == -1) {
> +  if (!domain_create_tty(dom)) {
> err = errno;
> xc_evtchn_close(dom->xce_handle);
> dom->xce_handle = -1;
> @@ -535,7 +551,8 @@ static struct domain *create_domain(int
> dom->conspath = s;
> strcat(dom->conspath, "/console");
>  
> - dom->tty_fd = -1;
> + dom->master_fd = -1;
> + dom->slave_fd = -1;
> dom->log_fd = -1;
>  
> dom->is_dead = false;
> @@ -597,14 +614,7 @@ static void remove_domain(struct domain
>  
>  static void cleanup_domain(struct domain *d)
>  {
> - if (d->tty_fd != -1) {
> -  close(d->tty_fd);
> -  d->tty_fd = -1;
> - }
> - if (d->log_fd != -1) {
> -  close(d->log_fd);
> -  d->log_fd = -1;
> - }
> + domain_close_tty(d);
>  
> free(d->buffer.data);
> d->buffer.data = NULL;
> @@ -683,13 +693,17 @@ static void handle_tty_read(struct domai
> if (len > sizeof(msg))
> len = sizeof(msg);
>  
> - len = read(dom->tty_fd, msg, len);
> - if (len < 1) {
> -  close(dom->tty_fd);
> -  dom->tty_fd = -1;
> + len = read(dom->master_fd, msg, len);
> + /*
> +  * Note: on Solaris, len == 0 means the slave closed, and this
> +  * is no problem, but Linux can't handle this usefully, so we
> +  * keep the slave open for the duration.
> +  */
> + if (len < 0) {
> +  domain_close_tty(dom);
>  
> if (domain_is_valid(dom->domid)) {
> -   dom->tty_fd = domain_create_tty(dom);
> +   domain_create_tty(dom);
> } else {
> shutdown_domain(dom);
> }
> @@ -703,8 +717,7 @@ static void handle_tty_read(struct domai
> intf->in_prod = prod;
> xc_evtchn_notify(dom->xce_handle, dom->local_port);
> } else {
> -  close(dom->tty_fd);
> -  dom->tty_fd = -1;
> +  domain_close_tty(dom);
> shutdown_domain(dom);
> }
>  }
> @@ -716,17 +729,16 @@ static void handle_tty_write(struct doma
> if (dom->is_dead)
> return;
>  
> - len = write(dom->tty_fd, dom->buffer.data + dom->buffer.consumed,
> + len = write(dom->master_fd, dom->buffer.data + dom->buffer.consumed,
>    dom->buffer.size - dom->buffer.consumed);
> if (len < 1) {
> dolog(LOG_DEBUG, "Write failed on domain %d: %zd, %d\n",
>      dom->domid, len, errno);
>  
> -  close(dom->tty_fd);
> -  dom->tty_fd = -1;
> +  domain_close_tty(dom);
>  
> if (domain_is_valid(dom->domid)) {
> -   dom->tty_fd = domain_create_tty(dom);
> +   domain_create_tty(dom);
> } else {
> shutdown_domain(dom);
> }
> @@ -895,13 +907,13 @@ void handle_io(void)
> max_fd = MAX(evtchn_fd, max_fd);
> }
>  
> -   if (d->tty_fd != -1) {
> +   if (d->master_fd != -1) {
> if (!d->is_dead && ring_free_bytes(d))
> -     FD_SET(d->tty_fd, &readfds);
> +     FD_SET(d->master_fd, &readfds);
>  
> if (!buffer_empty(&d->buffer))
> -     FD_SET(d->tty_fd, &writefds);
> -    max_fd = MAX(d->tty_fd, max_fd);
> +     FD_SET(d->master_fd, &writefds);
> +    max_fd = MAX(d->master_fd, max_fd);
> }
> }
>  
> @@ -951,10 +963,10 @@ void handle_io(void)
> handle_ring_read(d);
> }
>  
> -   if (d->tty_fd != -1 && FD_ISSET(d->tty_fd, &readfds))
> +   if (d->master_fd != -1 && FD_ISSET(d->master_fd, &readfds))
> handle_tty_read(d);
>  
> -   if (d->tty_fd != -1 && FD_ISSET(d->tty_fd, &writefds))
> +   if (d->master_fd != -1 && FD_ISSET(d->master_fd, &writefds))
> handle_tty_write(d);
>  
> if (d->is_dead)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel