- djm@cvs.openbsd.org 2010/05/14 23:29:23

     [channels.c channels.h mux.c ssh.c]
     Pause the mux channel while waiting for reply from aynch callbacks.
     Prevents misordering of replies if new requests arrive while waiting.

     Extend channel open confirm callback to allow signalling failure
     conditions as well as success. Use this to 1) fix a memory leak, 2)
     start using the above pause mechanism and 3) delay sending a success/
     failure message on mux slave session open until we receive a reply from
     the server.

     motivated by and with feedback from markus@

5 files changed, 70 insertions, 20 deletions

diff --git a/ChangeLog b/ChangeLog
index b90af22b..a485000a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -11,6 +11,18 @@
      [ssh-add.c]
      check that the certificate matches the corresponding private key before
      grafting it on
+   - djm@cvs.openbsd.org 2010/05/14 23:29:23
+     [channels.c channels.h mux.c ssh.c]
+     Pause the mux channel while waiting for reply from aynch callbacks.
+     Prevents misordering of replies if new requests arrive while waiting.
+     
+     Extend channel open confirm callback to allow signalling failure
+     conditions as well as success. Use this to 1) fix a memory leak, 2)
+     start using the above pause mechanism and 3) delay sending a success/
+     failure message on mux slave session open until we receive a reply from
+     the server.
+     
+     motivated by and with feedback from markus@
 
 20100511
  - (dtucker) [Makefile.in] Bug #1770: Link libopenbsd-compat twice to solve
diff --git a/channels.c b/channels.c
index a55d2781..0f750c4d 100644
--- a/channels.c
+++ b/channels.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: channels.c,v 1.303 2010/01/30 21:12:08 djm Exp $ */
+/* $OpenBSD: channels.c,v 1.304 2010/05/14 23:29:23 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -330,6 +330,7 @@ channel_new(char *ctype, int type, int rfd, int wfd, int efd,
 	c->ctl_chan = -1;
 	c->mux_rcb = NULL;
 	c->mux_ctx = NULL;
+	c->mux_pause = 0;
 	c->delayed = 1;		/* prevent call to channel_post handler */
 	TAILQ_INIT(&c->status_confirms);
 	debug("channel %d: new [%s]", found, remote_name);
@@ -703,7 +704,7 @@ channel_register_status_confirm(int id, channel_confirm_cb *cb,
 }
 
 void
-channel_register_open_confirm(int id, channel_callback_fn *fn, void *ctx)
+channel_register_open_confirm(int id, channel_open_fn *fn, void *ctx)
 {
 	Channel *c = channel_lookup(id);
 
@@ -991,7 +992,7 @@ channel_pre_x11_open(Channel *c, fd_set *readset, fd_set *writeset)
 static void
 channel_pre_mux_client(Channel *c, fd_set *readset, fd_set *writeset)
 {
-	if (c->istate == CHAN_INPUT_OPEN &&
+	if (c->istate == CHAN_INPUT_OPEN && !c->mux_pause &&
 	    buffer_check_alloc(&c->input, CHAN_RBUF))
 		FD_SET(c->rfd, readset);
 	if (c->istate == CHAN_INPUT_WAIT_DRAIN) {
@@ -1840,7 +1841,7 @@ channel_post_mux_client(Channel *c, fd_set *readset, fd_set *writeset)
 	if (!compat20)
 		fatal("%s: entered with !compat20", __func__);
 
-	if (c->rfd != -1 && FD_ISSET(c->rfd, readset) &&
+	if (c->rfd != -1 && !c->mux_pause && FD_ISSET(c->rfd, readset) &&
 	    (c->istate == CHAN_INPUT_OPEN ||
 	    c->istate == CHAN_INPUT_WAIT_DRAIN)) {
 		/*
@@ -2463,7 +2464,7 @@ channel_input_open_confirmation(int type, u_int32_t seq, void *ctxt)
 		c->remote_maxpacket = packet_get_int();
 		if (c->open_confirm) {
 			debug2("callback start");
-			c->open_confirm(c->self, c->open_confirm_ctx);
+			c->open_confirm(c->self, 1, c->open_confirm_ctx);
 			debug2("callback done");
 		}
 		debug2("channel %d: open confirm rwindow %u rmax %u", c->self,
@@ -2514,6 +2515,11 @@ channel_input_open_failure(int type, u_int32_t seq, void *ctxt)
 			xfree(msg);
 		if (lang != NULL)
 			xfree(lang);
+		if (c->open_confirm) {
+			debug2("callback start");
+			c->open_confirm(c->self, 0, c->open_confirm_ctx);
+			debug2("callback done");
+		}
 	}
 	packet_check_eom();
 	/* Schedule the channel for cleanup/deletion. */
diff --git a/channels.h b/channels.h
index cc71885f..0680ed00 100644
--- a/channels.h
+++ b/channels.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: channels.h,v 1.103 2010/01/26 01:28:35 djm Exp $ */
+/* $OpenBSD: channels.h,v 1.104 2010/05/14 23:29:23 djm Exp $ */
 
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
@@ -60,6 +60,7 @@
 struct Channel;
 typedef struct Channel Channel;
 
+typedef void channel_open_fn(int, int, void *);
 typedef void channel_callback_fn(int, void *);
 typedef int channel_infilter_fn(struct Channel *, char *, int);
 typedef void channel_filter_cleanup_fn(int, void *);
@@ -130,7 +131,7 @@ struct Channel {
 	char   *ctype;		/* type */
 
 	/* callback */
-	channel_callback_fn	*open_confirm;
+	channel_open_fn		*open_confirm;
 	void			*open_confirm_ctx;
 	channel_callback_fn	*detach_user;
 	int			detach_close;
@@ -151,6 +152,7 @@ struct Channel {
 	/* multiplexing protocol hook, called for each packet received */
 	mux_callback_fn		*mux_rcb;
 	void			*mux_ctx;
+	int			mux_pause;
 };
 
 #define CHAN_EXTENDED_IGNORE		0
@@ -208,7 +210,7 @@ void	 channel_stop_listening(void);
 void	 channel_send_open(int);
 void	 channel_request_start(int, char *, int);
 void	 channel_register_cleanup(int, channel_callback_fn *, int);
-void	 channel_register_open_confirm(int, channel_callback_fn *, void *);
+void	 channel_register_open_confirm(int, channel_open_fn *, void *);
 void	 channel_register_filter(int, channel_infilter_fn *,
     channel_outfilter_fn *, channel_filter_cleanup_fn *, void *);
 void	 channel_register_status_confirm(int, channel_confirm_cb *,
diff --git a/mux.c b/mux.c
index ac477a0e..18dfd99f 100644
--- a/mux.c
+++ b/mux.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: mux.c,v 1.16 2010/04/23 22:27:38 djm Exp $ */
+/* $OpenBSD: mux.c,v 1.17 2010/05/14 23:29:23 djm Exp $ */
 /*
  * Copyright (c) 2002-2008 Damien Miller <djm@openbsd.org>
  *
@@ -106,6 +106,7 @@ struct mux_session_confirm_ctx {
 	char *term;
 	struct termios tio;
 	char **env;
+	u_int rid;
 };
 
 /* fd to control socket */
@@ -149,7 +150,7 @@ struct mux_master_state {
 #define MUX_FWD_REMOTE  2
 #define MUX_FWD_DYNAMIC 3
 
-static void mux_session_confirm(int, void *);
+static void mux_session_confirm(int, int, void *);
 
 static int process_mux_master_hello(u_int, Channel *, Buffer *, Buffer *);
 static int process_mux_new_session(u_int, Channel *, Buffer *, Buffer *);
@@ -301,6 +302,7 @@ process_mux_new_session(u_int rid, Channel *c, Buffer *m, Buffer *r)
 	/* Reply for SSHMUX_COMMAND_OPEN */
 	cctx = xcalloc(1, sizeof(*cctx));
 	cctx->term = NULL;
+	cctx->rid = rid;
 	cmd = reserved = NULL;
 	if ((reserved = buffer_get_string_ret(m, NULL)) == NULL ||
 	    buffer_get_int_ret(&cctx->want_tty, m) != 0 ||
@@ -454,14 +456,10 @@ process_mux_new_session(u_int rid, Channel *c, Buffer *m, Buffer *r)
 
 	channel_send_open(nc->self);
 	channel_register_open_confirm(nc->self, mux_session_confirm, cctx);
+	c->mux_pause = 1; /* stop handling messages until open_confirm done */
 	channel_register_cleanup(nc->self, mux_master_session_cleanup_cb, 1);
 
-	/* prepare reply */
-	/* XXX defer until mux_session_confirm() fires */
-	buffer_put_int(r, MUX_S_SESSION_OPENED);
-	buffer_put_int(r, rid);
-	buffer_put_int(r, nc->self);
-
+	/* reply is deferred, sent by mux_session_confirm */
 	return 0;
 }
 
@@ -1000,17 +998,31 @@ muxserver_listen(void)
 
 /* Callback on open confirmation in mux master for a mux client session. */
 static void
-mux_session_confirm(int id, void *arg)
+mux_session_confirm(int id, int success, void *arg)
 {
 	struct mux_session_confirm_ctx *cctx = arg;
 	const char *display;
-	Channel *c;
+	Channel *c, *cc;
 	int i;
+	Buffer reply;
 
 	if (cctx == NULL)
 		fatal("%s: cctx == NULL", __func__);
 	if ((c = channel_by_id(id)) == NULL)
 		fatal("%s: no channel for id %d", __func__, id);
+	if ((cc = channel_by_id(c->ctl_chan)) == NULL)
+		fatal("%s: channel %d lacks control channel %d", __func__,
+		    id, c->ctl_chan);
+
+	if (!success) {
+		debug3("%s: sending failure reply", __func__);
+		/* prepare reply */
+		buffer_init(&reply);
+		buffer_put_int(&reply, MUX_S_FAILURE);
+		buffer_put_int(&reply, cctx->rid);
+		buffer_put_cstring(&reply, "Session open refused by peer");
+		goto done;
+	}
 
 	display = getenv("DISPLAY");
 	if (cctx->want_x_fwd && options.forward_x11 && display != NULL) {
@@ -1033,6 +1045,21 @@ mux_session_confirm(int id, void *arg)
 	client_session2_setup(id, cctx->want_tty, cctx->want_subsys,
 	    cctx->term, &cctx->tio, c->rfd, &cctx->cmd, cctx->env);
 
+	debug3("%s: sending success reply", __func__);
+	/* prepare reply */
+	buffer_init(&reply);
+	buffer_put_int(&reply, MUX_S_SESSION_OPENED);
+	buffer_put_int(&reply, cctx->rid);
+	buffer_put_int(&reply, c->self);
+
+ done:
+	/* Send reply */
+	buffer_put_string(&cc->output, buffer_ptr(&reply), buffer_len(&reply));
+	buffer_free(&reply);
+
+	if (cc->mux_pause <= 0)
+		fatal("%s: mux_pause %d", __func__, cc->mux_pause);
+	cc->mux_pause = 0; /* start processing messages again */
 	c->open_confirm_ctx = NULL;
 	buffer_free(&cctx->cmd);
 	xfree(cctx->term);
diff --git a/ssh.c b/ssh.c
index 2230edd1..ee224e9f 100644
--- a/ssh.c
+++ b/ssh.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh.c,v 1.336 2010/04/10 00:00:16 djm Exp $ */
+/* $OpenBSD: ssh.c,v 1.337 2010/05/14 23:29:23 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -1175,12 +1175,15 @@ ssh_session(void)
 
 /* request pty/x11/agent/tcpfwd/shell for channel */
 static void
-ssh_session2_setup(int id, void *arg)
+ssh_session2_setup(int id, int success, void *arg)
 {
 	extern char **environ;
 	const char *display;
 	int interactive = tty_flag;
 
+	if (!success)
+		return; /* No need for error message, channels code sens one */
+
 	display = getenv("DISPLAY");
 	if (options.forward_x11 && display != NULL) {
 		char *proto, *data;