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

[Xen-devel] pty fixes: please test

To: xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: [Xen-devel] pty fixes: please test
From: John Levon <levon@xxxxxxxxxxxxxxxxx>
Date: Wed, 12 Dec 2007 22:15:02 +0000
Delivery-date: Wed, 12 Dec 2007 14:15:42 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.9i
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

<Prev in Thread] Current Thread [Next in Thread>
  • [Xen-devel] pty fixes: please test, John Levon <=