launcher: fix socket message race condition

fixes issue #484 (race condition with message to/from weston launch)

The race condition occurs after weston sends the WESTON_LAUNCHER_OPEN
message to weston-launch.  The race is between when weston-launch replies
with the fd handle versus weston-launch sending an activation message.  If
weston-launch sends an activation message before sending the fd handle,
then weston will be in an invalid state.

To fix this, I modified the fd handle reply that weston-launch sends to
include a message id at the beginning, which I called
WESTON_LAUNCHER_OPEN_REPLY.  Along with this, weston now inspects the
first part of any reply to determine whether it is an activation message
or a reply to the OPEN message.  In the newly handled case that it's an
activation message, it tracks whether the latest result is a deactivate
message and stores it in a flag to be handled once the open function has
completed.

Signed-off-by: Jonathan Marler <johnnymarler@gmail.com>

libweston/launcher-weston-launch.c

@@ -93,6 +93,7 @@ struct launcher_weston_launch {
 	struct wl_event_source *source;
 
 	int kb_mode, tty, drm_fd;
+	int deferred_deactivate;
 };
 
 static ssize_t
@@ -107,12 +108,36 @@ launcher_weston_launch_send(int sockfd, void *buf, size_t buflen)
 	return len;
 }
 
+static void
+handle_deactivate(struct launcher_weston_launch *launcher)
+{
+	int reply;
+
+	launcher->compositor->session_active = false;
+	wl_signal_emit(&launcher->compositor->session_signal,
+		       launcher->compositor);
+
+	reply = WESTON_LAUNCHER_DEACTIVATE_DONE;
+	launcher_weston_launch_send(launcher->fd, &reply, sizeof reply);
+}
+
+static void
+idle_deactivate(void *data)
+{
+	struct launcher_weston_launch *launcher = data;
+
+	if (launcher->deferred_deactivate) {
+		launcher->deferred_deactivate = 0;
+		handle_deactivate((struct launcher_weston_launch*)data);
+	}
+}
+
 static int
 launcher_weston_launch_open(struct weston_launcher *launcher_base,
 		     const char *path, int flags)
 {
 	struct launcher_weston_launch *launcher = wl_container_of(launcher_base, launcher, base);
-	int n, ret;
+	int n;
 	struct msghdr msg;
 	struct cmsghdr *cmsg;
 	struct iovec iov;
@@ -120,6 +145,7 @@ launcher_weston_launch_open(struct weston_launcher *launcher_base,
 	char control[CMSG_SPACE(sizeof data->fd)];
 	ssize_t len;
 	struct weston_launcher_open *message;
+	struct { int id; int ret; } event;
 
 	n = sizeof(*message) + strlen(path) + 1;
 	message = malloc(n);
@@ -134,19 +160,33 @@ launcher_weston_launch_open(struct weston_launcher *launcher_base,
 	free(message);
 
 	memset(&msg, 0, sizeof msg);
-	iov.iov_base = &ret;
+	iov.iov_base = &event;
-	iov.iov_len = sizeof ret;
+	iov.iov_len = sizeof event;
 	msg.msg_iov = &iov;
 	msg.msg_iovlen = 1;
 	msg.msg_control = control;
-	msg.msg_controllen = sizeof control;
 
-	do {
+	while (1) {
-		len = recvmsg(launcher->fd, &msg, MSG_CMSG_CLOEXEC);
+		msg.msg_controllen = sizeof control;
-	} while (len < 0 && errno == EINTR);
 
-	if (len != sizeof ret ||
+		do {
-	    ret < 0)
+			len = recvmsg(launcher->fd, &msg, MSG_CMSG_CLOEXEC);
+		} while (len < 0 && errno == EINTR);
+
+		// Only OPEN_REPLY and up to one DEACTIVATE message should be possible here
+		if ((len == sizeof event) && (event.id == WESTON_LAUNCHER_OPEN_REPLY))
+			break;
+
+		if ((len == sizeof event.id) && (event.id == WESTON_LAUNCHER_DEACTIVATE) && (launcher->deferred_deactivate == 0)) {
+			wl_event_loop_add_idle(wl_display_get_event_loop(launcher->compositor->wl_display), idle_deactivate, launcher);
+			launcher->deferred_deactivate = 1;
+		} else {
+			weston_log("unexpected event %d (len=%zd) from weston-launch\n", event.id, len);
+			return -1;
+		}
+	}
+
+	if (event.ret < 0)
 		return -1;
 
 	cmsg = CMSG_FIRSTHDR(&msg);
@@ -201,7 +241,7 @@ static int
 launcher_weston_launch_data(int fd, uint32_t mask, void *data)
 {
 	struct launcher_weston_launch *launcher = data;
-	int len, ret, reply;
+	int len, ret;
 
 	if (mask & (WL_EVENT_HANGUP | WL_EVENT_ERROR)) {
 		weston_log("launcher socket closed, exiting\n");
@@ -212,6 +252,12 @@ launcher_weston_launch_data(int fd, uint32_t mask, void *data)
 		exit(-1);
 	}
 
+	if (launcher->deferred_deactivate) {
+		launcher->deferred_deactivate = 0;
+		handle_deactivate(launcher);
+		return 1;
+	}
+
 	do {
 		len = recv(launcher->fd, &ret, sizeof ret, 0);
 	} while (len < 0 && errno == EINTR);
@@ -223,13 +269,7 @@ launcher_weston_launch_data(int fd, uint32_t mask, void *data)
 			       launcher->compositor);
 		break;
 	case WESTON_LAUNCHER_DEACTIVATE:
-		launcher->compositor->session_active = false;
+		handle_deactivate(launcher);
-		wl_signal_emit(&launcher->compositor->session_signal,
-			       launcher->compositor);
-
-		reply = WESTON_LAUNCHER_DEACTIVATE_DONE;
-		launcher_weston_launch_send(launcher->fd, &reply, sizeof reply);
-
 		break;
 	default:
 		weston_log("unexpected event from weston-launch\n");
@@ -287,6 +327,7 @@ launcher_weston_launch_connect(struct weston_launcher **out, struct weston_compo
 	* (struct launcher_weston_launch **) out = launcher;
 	launcher->compositor = compositor;
 	launcher->drm_fd = -1;
+	launcher->deferred_deactivate = 0;
 	launcher->fd = launcher_weston_environment_get_fd("WESTON_LAUNCHER_SOCK");
 	if (launcher->fd != -1) {
 		launcher->tty = launcher_weston_environment_get_fd("WESTON_TTY_FD");

libweston/weston-launch.c

@@ -377,8 +377,9 @@ err0:
 		nmsg.msg_controllen = cmsg->cmsg_len;
 		ret = 0;
 	}
-	iov.iov_base = &ret;
+	struct { int reply_id; int ret; } reply_iov_data = { WESTON_LAUNCHER_OPEN_REPLY, ret };
-	iov.iov_len = sizeof ret;
+	iov.iov_base = &reply_iov_data;
+	iov.iov_len = sizeof reply_iov_data;
 
 	if (wl->verbose)
 		fprintf(stderr, "weston-launch: opened %s: ret: %d, fd: %d\n",

libweston/weston-launch.h

@@ -35,6 +35,8 @@ enum weston_launcher_event {
 	WESTON_LAUNCHER_ACTIVATE,
 	WESTON_LAUNCHER_DEACTIVATE,
 	WESTON_LAUNCHER_DEACTIVATE_DONE,
+	// This event is followed by an fd handle
+	WESTON_LAUNCHER_OPEN_REPLY,
 };
 
 struct weston_launcher_message {