[Xen-devel] pty fixes: please test

[John Levon <levon@xxxxxxxxxxxxxxxxx>]

I haven't tested this patch on Linux or BSD so I'd appreciate some
testing (Dan? Christoph?). This makes xenconsoled in unstable work
again on Solaris

thanks
john

# HG changeset patch
# User john.levon@xxxxxxx
# Date 1197495828 28800
# Node ID 3430f8e764191214e0761100e50caf69f167345f
# Parent  3a972b4de3bc3ef90c641a6a036274edb7ca92dc
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
- a read of zero from the master means that a slave closed, and
  is not an error

Signed-off-by: John Levon <john.levon@xxxxxxx>

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
@@ -40,6 +40,9 @@
 #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))
@@ -230,50 +233,64 @@ static int create_domain_log(struct doma
 #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)
+            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 ||
+           (termp != NULL && tcgetattr(sfd, termp) < 0))
+               goto err;
 
        if (amaster)
                *amaster = mfd;
        if (aslave)
                *aslave = sfd;
-       if (name)
-               strlcpy(name, ptsname(mfd), sizeof(slave));
        if (winp)
                ioctl(sfd, TIOCSWINSZ, winp);
 
        return 0;
 
-err1:
+err:
+       if (sfd != -1)
+               close(sfd);
        close(mfd);
-err0:
        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
-
 
 static int domain_create_tty(struct domain *dom)
 {
-       char slave[80];
+       const char *slave;
        struct termios term;
        char *path;
        int master, slavefd;
@@ -282,7 +299,7 @@ static int domain_create_tty(struct doma
        char *data;
        unsigned int len;
 
-       if (openpty(&master, &slavefd, slave, &term, NULL) < 0) {
+       if (openpty(&master, &slavefd, NULL, &term, NULL) < 0) {
                master = -1;
                err = errno;
                dolog(LOG_ERR, "Failed to create tty for domain-%d (errno = %i, 
%s)",
@@ -290,14 +307,23 @@ static int domain_create_tty(struct doma
                return master;
        }
 
-       cfmakeraw(&term);
-       if (tcsetattr(master, TCSAFLUSH, &term) < 0) {
+       if ((slave = ptsname(master)) == 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;
        }
 
+       cfmakeraw(&term);
+
+       if (tcsetattr(slavefd, TCSAFLUSH, &term) < 0) {
+               err = errno;
+               dolog(LOG_ERR, "Failed to set tty attribute for domain-%d 
(errno = %i, %s)",
+                     dom->domid, err, strerror(err));
+               goto out;
+       }
+
+       close(slavefd);
 
        if (dom->use_consolepath) {
                success = asprintf(&path, "%s/limit", dom->conspath) !=
@@ -684,7 +710,9 @@ static void handle_tty_read(struct domai
                len = sizeof(msg);
 
        len = read(dom->tty_fd, msg, len);
-       if (len < 1) {
+       if (len == 0) {
+               /* slave did a close: that's fine. */
+       } else if (len < 0) {
                close(dom->tty_fd);
                dom->tty_fd = -1;
 
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