4 files changed, 90 insertions, 53 deletions

diff --git a/ChangeLog b/ChangeLog
index 5ea4afcac..bd6b640b1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -92,6 +92,18 @@
      sending channel errors instead of than exiting with fatal().
      bz#1090; MaxSessions config bits and manpage from junyer AT gmail.com
      ok markus@
+   - djm@cvs.openbsd.org 2008/05/08 13:06:11
+     [clientloop.c clientloop.h ssh.c]
+     Use new channel status confirmation callback system to properly deal
+     with "important" channel requests that fail, in particular command exec,
+     shell and subsystem requests. Previously we would optimistically assume
+     that the requests would always succeed, which could cause hangs if they
+     did not (e.g. when the server runs out of fds) or were unimplemented by
+     the server (bz #1384)
+     Also, properly report failing multiplex channel requests via the mux
+     client stderr (subject to LogLevel in the mux master) - better than
+     silently failing.
+     most bits ok markus@ (as part of a larger diff)
 
 20080403
  - (djm) [openbsd-compat/bsd-poll.c] Include stdlib.h to avoid compile-
@@ -3952,4 +3964,4 @@
    OpenServer 6 and add osr5bigcrypt support so when someone migrates
    passwords between UnixWare and OpenServer they will still work. OK dtucker@
 
-$Id: ChangeLog,v 1.4923 2008/05/19 05:34:50 djm Exp $
+$Id: ChangeLog,v 1.4924 2008/05/19 05:36:08 djm Exp $
diff --git a/clientloop.c b/clientloop.c
index edd801440..c40f2c303 100644
--- a/clientloop.c
+++ b/clientloop.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: clientloop.c,v 1.189 2008/05/08 12:02:23 djm Exp $ */
+/* $OpenBSD: clientloop.c,v 1.190 2008/05/08 13:06:10 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -173,6 +173,11 @@ struct confirm_ctx {
         char **env;
 };
 
+struct channel_reply_ctx {
+        const char *request_type;
+        int id, do_close;
+};
+
 /*XXX*/
 extern Kex *xxx_kex;
 
@@ -645,25 +650,60 @@ client_process_net_input(fd_set *readset)
 }
 
 static void
-client_subsystem_reply(int type, u_int32_t seq, void *ctxt)
+client_status_confirm(int type, Channel *c, void *ctx)
 {
-        int id;
-        Channel *c;
+        struct channel_reply_ctx *cr = (struct channel_reply_ctx *)ctx;
+        char errmsg[256];
+        int tochan;
+
+        /* XXX supress on mux _client_ quietmode */
+        tochan = options.log_level >= SYSLOG_LEVEL_ERROR &&
+            c->ctl_fd != -1 && c->extended_usage == CHAN_EXTENDED_WRITE;
+
+        if (type == SSH2_MSG_CHANNEL_SUCCESS) {
+                debug2("%s request accepted on channel %d",
+                    cr->request_type, c->self);
+        } else if (type == SSH2_MSG_CHANNEL_FAILURE) {
+                if (tochan) {
+                        snprintf(errmsg, sizeof(errmsg),
+                            "%s request failed\r\n", cr->request_type);
+                } else {
+                        snprintf(errmsg, sizeof(errmsg),
+                            "%s request failed on channel %d",
+                            cr->request_type, c->self);
+                }
+                /* If error occurred on primary session channel, then exit */
+                if (cr->do_close && c->self == session_ident)
+                        fatal("%s", errmsg);
+                /* If error occurred on mux client, append to their stderr */
+                if (tochan)
+                        buffer_append(&c->extended, errmsg, strlen(errmsg));
+                else
+                        error("%s", errmsg);
+                if (cr->do_close) {
+                        chan_read_failed(c);
+                        chan_write_failed(c);
+                }
+        }
+        xfree(cr);
+}
 
-        id = packet_get_int();
-        packet_check_eom();
+static void
+client_abandon_status_confirm(Channel *c, void *ctx)
+{
+        xfree(ctx);
+}
 
-        if ((c = channel_lookup(id)) == NULL) {
-                error("%s: no channel for id %d", __func__, id);
-                return;
-        }
+static void
+client_expect_confirm(int id, const char *request, int do_close)
+{
+        struct channel_reply_ctx *cr = xmalloc(sizeof(*cr));
 
-        if (type == SSH2_MSG_CHANNEL_SUCCESS)
-                debug2("Request suceeded on channel %d", id);
-        else if (type == SSH2_MSG_CHANNEL_FAILURE) {
-                error("Request failed on channel %d", id);
-                channel_free(c);
-        }
+        cr->request_type = request;
+        cr->do_close = do_close;
+
+        channel_register_status_confirm(id, client_status_confirm,
+            client_abandon_status_confirm, cr);
 }
 
 static void
@@ -698,8 +738,7 @@ client_extra_session2_setup(int id, void *arg)
         }
 
         client_session2_setup(id, cctx->want_tty, cctx->want_subsys,
-            cctx->term, &cctx->tio, c->rfd, &cctx->cmd, cctx->env,
-            client_subsystem_reply);
+            cctx->term, &cctx->tio, c->rfd, &cctx->cmd, cctx->env);
 
         c->open_confirm_ctx = NULL;
         buffer_free(&cctx->cmd);
@@ -1366,7 +1405,9 @@ client_process_buffered_input_packets(void)
 static int
 simple_escape_filter(Channel *c, char *buf, int len)
 {
-        /* XXX we assume c->extended is writeable */
+        if (c->extended_usage != CHAN_EXTENDED_WRITE)
+                return 0;
+
         return process_escapes(&c->input, &c->output, &c->extended, buf, len);
 }
 
@@ -1962,8 +2003,7 @@ client_input_global_request(int type, u_int32_t seq, void *ctxt)
 
 void
 client_session2_setup(int id, int want_tty, int want_subsystem,
-    const char *term, struct termios *tiop, int in_fd, Buffer *cmd, char **env,
-    dispatch_fn *subsys_repl)
+    const char *term, struct termios *tiop, int in_fd, Buffer *cmd, char **env)
 {
         int len;
         Channel *c = NULL;
@@ -1981,7 +2021,8 @@ client_session2_setup(int id, int want_tty, int want_subsystem,
                 if (ioctl(in_fd, TIOCGWINSZ, &ws) < 0)
                         memset(&ws, 0, sizeof(ws));
 
-                channel_request_start(id, "pty-req", 0);
+                channel_request_start(id, "pty-req", 1);
+                client_expect_confirm(id, "PTY allocation", 0);
                 packet_put_cstring(term != NULL ? term : "");
                 packet_put_int((u_int)ws.ws_col);
                 packet_put_int((u_int)ws.ws_row);
@@ -2036,22 +2077,21 @@ client_session2_setup(int id, int want_tty, int want_subsystem,
                 if (len > 900)
                         len = 900;
                 if (want_subsystem) {
-                        debug("Sending subsystem: %.*s", len, (u_char*)buffer_ptr(cmd));
-                        channel_request_start(id, "subsystem", subsys_repl != NULL);
-                        if (subsys_repl != NULL) {
-                                /* register callback for reply */
-                                /* XXX we assume that client_loop has already been called */
-                                dispatch_set(SSH2_MSG_CHANNEL_FAILURE, subsys_repl);
-                                dispatch_set(SSH2_MSG_CHANNEL_SUCCESS, subsys_repl);
-                        }
+                        debug("Sending subsystem: %.*s",
+                            len, (u_char*)buffer_ptr(cmd));
+                        channel_request_start(id, "subsystem", 1);
+                        client_expect_confirm(id, "subsystem", 1);
                 } else {
-                        debug("Sending command: %.*s", len, (u_char*)buffer_ptr(cmd));
-                        channel_request_start(id, "exec", 0);
+                        debug("Sending command: %.*s",
+                            len, (u_char*)buffer_ptr(cmd));
+                        channel_request_start(id, "exec", 1);
+                        client_expect_confirm(id, "exec", 1);
                 }
                 packet_put_string(buffer_ptr(cmd), buffer_len(cmd));
                 packet_send();
         } else {
-                channel_request_start(id, "shell", 0);
+                channel_request_start(id, "shell", 1);
+                client_expect_confirm(id, "shell", 1);
                 packet_send();
         }
 }
diff --git a/clientloop.h b/clientloop.h
index c7d2233d0..cb2d7c089 100644
--- a/clientloop.h
+++ b/clientloop.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: clientloop.h,v 1.17 2007/08/07 07:32:53 djm Exp $ */
+/* $OpenBSD: clientloop.h,v 1.18 2008/05/08 13:06:11 djm Exp $ */
 
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
@@ -43,7 +43,7 @@ void	 client_x11_get_proto(const char *, const char *, u_int,
             char **, char **);
 void     client_global_request_reply_fwd(int, u_int32_t, void *);
 void     client_session2_setup(int, int, int, const char *, struct termios *,
-            int, Buffer *, char **, dispatch_fn *);
+            int, Buffer *, char **);
 int      client_request_tun_fwd(int, int, int);
 
 /* Multiplexing protocol version */
diff --git a/ssh.c b/ssh.c
index b144a7130..cfefadef9 100644
--- a/ssh.c
+++ b/ssh.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh.c,v 1.310 2008/05/08 12:02:23 djm Exp $ */
+/* $OpenBSD: ssh.c,v 1.311 2008/05/08 13:06:11 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -1039,21 +1039,6 @@ ssh_session(void)
             options.escape_char : SSH_ESCAPECHAR_NONE, 0);
 }
 
-static void
-ssh_subsystem_reply(int type, u_int32_t seq, void *ctxt)
-{
-        int id, len;
-
-        id = packet_get_int();
-        len = buffer_len(&command);
-        if (len > 900)
-                len = 900;
-        packet_check_eom();
-        if (type == SSH2_MSG_CHANNEL_FAILURE)
-                fatal("Request for subsystem '%.*s' failed on channel %d",
-                    len, (u_char *)buffer_ptr(&command), id);
-}
-
 void
 client_global_request_reply_fwd(int type, u_int32_t seq, void *ctxt)
 {
@@ -1150,7 +1135,7 @@ ssh_session2_setup(int id, void *arg)
         }
 
         client_session2_setup(id, tty_flag, subsystem_flag, getenv("TERM"),
-            NULL, fileno(stdin), &command, environ, &ssh_subsystem_reply);
+            NULL, fileno(stdin), &command, environ);
 
         packet_set_interactive(interactive);
 }