- 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)

diff --git a/ChangeLog b/ChangeLog
index bf075cb9f3a67fac50e3e5133fde9c9250270dfe..ae003e8eaad9bb022e2c297ae5172acc9098957b 100644 (file)
--- 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-

diff --git a/clientloop.c b/clientloop.c
index edd80144007c989a99b0dccdc9d64bc0ab21ddc3..c40f2c3032a3c6cb2cf69f6735f1b8b174e5c20c 100644 (file)
--- 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 c7d2233d03a82106848d0f98575d6a011011b723..cb2d7c08949f1fdb0742ba43b7f248591dbdd34b 100644 (file)
--- 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 b144a7130bdf30be01251f868655860a08876986..cfefadef95f5bfe5a468f4d30a6fe22f2a181aba 100644 (file)
--- 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);
 }