WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

On Wed, Dec 19, 2007 at 12:00:25PM +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?
> 
> That one seems to work fine.

Excellent. Keir, can you apply?

cheers
john

# HG changeset patch
# User john.levon@xxxxxxx
# Date 1198003657 28800
# Node ID 6f03f4ec458d4780314c015da214795b7b1cf195
# Parent  539cbabd97b5ff3d335de151636040bb2f4cd629
Fix master/slave handling in xenconsoled and qemu

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)
diff --git a/tools/ioemu/vl.c b/tools/ioemu/vl.c
--- a/tools/ioemu/vl.c
+++ b/tools/ioemu/vl.c
@@ -65,6 +65,9 @@
 #include <linux/rtc.h>
 #include <linux/ppdev.h>
 #endif
+#endif
+#if defined(__sun__)
+#include <stropts.h>
 #endif
 #endif
 
@@ -1801,7 +1804,65 @@ static int store_dev_info(char *devName,
     return 0;
 }
 
-#if defined(__linux__) || defined(__NetBSD__) || defined(__OpenBSD__)
+#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)
+{
+       const char *slave;
+       int mfd = -1, sfd = -1;
+
+       *amaster = *aslave = -1;
+
+       mfd = open("/dev/ptmx", O_RDWR | O_NOCTTY);
+       if (mfd < 0)
+               goto err;
+
+       if (grantpt(mfd) == -1 || unlockpt(mfd) == -1)
+               goto err;
+
+       if ((slave = ptsname(mfd)) == NULL)
+               goto err;
+
+       if ((sfd = open(slave, O_RDONLY | O_NOCTTY)) == -1)
+               goto err;
+
+       if (ioctl(sfd, I_PUSH, "ptem") == -1 ||
+           (termp != NULL && tcgetattr(sfd, termp) < 0))
+               goto err;
+
+       if (amaster)
+               *amaster = mfd;
+       if (aslave)
+               *aslave = sfd;
+       if (winp)
+               ioctl(sfd, TIOCSWINSZ, winp);
+
+       return 0;
+
+err:
+       if (sfd != -1)
+               close(sfd);
+       close(mfd);
+       return -1;
+}
+
+void cfmakeraw (struct termios *termios_p)
+{
+       termios_p->c_iflag &=
+               ~(IGNBRK|BRKINT|PARMRK|ISTRIP|INLCR|IGNCR|ICRNL|IXON);
+       termios_p->c_oflag &= ~OPOST;
+       termios_p->c_lflag &= ~(ECHO|ECHONL|ICANON|ISIG|IEXTEN);
+       termios_p->c_cflag &= ~(CSIZE|PARENB);
+       termios_p->c_cflag |= CS8;
+
+       termios_p->c_cc[VMIN] = 0;
+       termios_p->c_cc[VTIME] = 0;
+}
+
+#endif
+
+#if defined(__linux__) || defined(__NetBSD__) || defined(__OpenBSD__) || 
defined(__sun__)
 static CharDriverState *qemu_chr_open_pty(void)
 {
     struct termios tty;
@@ -1816,6 +1877,8 @@ static CharDriverState *qemu_chr_open_pt
     cfmakeraw(&tty);
     tcsetattr(slave_fd, TCSAFLUSH, &tty);
     
+    close(slave_fd);
+
     fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
 
     return qemu_chr_open_fd(master_fd, master_fd);
@@ -2038,7 +2101,7 @@ static CharDriverState *qemu_chr_open_pt
 {
     return NULL;
 }
-#endif /* __linux__ || __NetBSD__ || __OpenBSD__ */
+#endif /* __linux__ || __NetBSD__ || __OpenBSD__ || __sun__ */
 
 #endif /* !defined(_WIN32) */
 

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