5 files changed, 70 insertions, 20 deletions

diff --git a/ChangeLog b/ChangeLog
index b90af22b0..a485000a8 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 a55d27817..0f750c4d4 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 cc71885f4..0680ed00e 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 ac477a0e7..18dfd99f3 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 2230edd16..ee224e9ff 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;