From 6bcc73666a0aa4fda96c8058b5ae7cccdeb9ddb7 Mon Sep 17 00:00:00 2001
From: David Herrmann <dh.herrmann@googlemail.com>
Date: Sun, 25 Mar 2012 18:06:33 +0200
Subject: [PATCH] pty: fix race when starting child

We should do fork() as the last operation when starting the child.
Currently, if our eloop-add() fails, we have a started child but return
failure to the caller. Therefore, the child will stay alive but we do not
use it.

We now perform all startup correctly before fork()'ing so we are always
safe when starting the child.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
 src/pty.c | 52 +++++++++++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/src/pty.c b/src/pty.c
index 0888f38..d177f93 100644
--- a/src/pty.c
+++ b/src/pty.c
@@ -49,6 +49,7 @@ struct kmscon_pty {
 	struct ev_eloop *eloop;
 
 	int fd;
+	pid_t child;
 	struct ev_fd *efd;
 	struct kmscon_ring *msgbuf;
 	char io_buf[KMSCON_NREAD];
@@ -198,50 +199,36 @@ err_out:
  * a little bit more control of the process, and as a bonus avoid linking to
  * the libutil library in glibc.
  */
-static int pty_spawn(struct kmscon_pty *pty, unsigned short width,
-							unsigned short height)
+static int pty_spawn(struct kmscon_pty *pty, int master,
+			unsigned short width, unsigned short height)
 {
 	int ret;
 	pid_t pid;
-	int master;
 	struct winsize ws;
 
-	if (pty->fd >= 0)
-		return -EALREADY;
-
 	memset(&ws, 0, sizeof(ws));
 	ws.ws_col = width;
 	ws.ws_row = height;
 
-	master = posix_openpt(O_RDWR | O_NOCTTY | O_CLOEXEC | O_NONBLOCK);
-	if (master < 0) {
-		log_err("cannot open master: %m");
-		return -errno;
-	}
-
 	log_debug("forking child");
 	pid = fork();
 	switch (pid) {
 	case -1:
 		log_err("cannot fork: %m");
-		ret = -errno;
-		goto err_master;
+		return -errno;
 	case 0:
 		ret = setup_child(master, &ws);
 		if (ret)
-			goto err_master;
+			return ret;
 		exec_child(pty->fd);
 		abort();
 	default:
 		pty->fd = master;
+		pty->child = pid;
 		break;
 	}
 
 	return 0;
-
-err_master:
-	close(master);
-	return ret;
 }
 
 static int send_buf(struct kmscon_pty *pty)
@@ -321,6 +308,7 @@ int kmscon_pty_open(struct kmscon_pty *pty, unsigned short width,
 							unsigned short height)
 {
 	int ret;
+	int master;
 
 	if (!pty)
 		return -EINVAL;
@@ -328,19 +316,29 @@ int kmscon_pty_open(struct kmscon_pty *pty, unsigned short width,
 	if (pty->fd >= 0)
 		return -EALREADY;
 
-	ret = pty_spawn(pty, width, height);
-	if (ret)
-		return ret;
+	master = posix_openpt(O_RDWR | O_NOCTTY | O_CLOEXEC | O_NONBLOCK);
+	if (master < 0) {
+		log_err("cannot open master: %m");
+		return -errno;
+	}
 
 	ret = ev_eloop_new_fd(pty->eloop, &pty->efd, pty->fd,
 					EV_READABLE, pty_input, pty);
-	if (ret) {
-		close(pty->fd);
-		pty->fd = -1;
-		return ret;
-	}
+	if (ret)
+		goto err_master;
+
+	ret = pty_spawn(pty, master, width, height);
+	if (ret)
+		goto err_fd;
 
 	return 0;
+
+err_fd:
+	ev_eloop_rm_fd(pty->efd);
+	pty->efd = NULL;
+err_master:
+	close(master);
+	return ret;
 }
 
 void kmscon_pty_close(struct kmscon_pty *pty)