diff options
author | Damien Miller <djm@mindrot.org> | 2010-05-21 14:57:10 +1000 |
---|---|---|
committer | Damien Miller <djm@mindrot.org> | 2010-05-21 14:57:10 +1000 |
commit | d530f5f471491b6be9edb58a063f2590e4dce48d (patch) | |
tree | e536e2b4031a4dd70026ff49c39b02b72af06f30 | |
parent | c6afb5f2c095a6a4380cc13a6480abb7614d949f (diff) |
- 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@
-rw-r--r-- | ChangeLog | 12 | ||||
-rw-r--r-- | channels.c | 16 | ||||
-rw-r--r-- | channels.h | 8 | ||||
-rw-r--r-- | mux.c | 47 | ||||
-rw-r--r-- | ssh.c | 7 |
5 files changed, 70 insertions, 20 deletions
@@ -11,6 +11,18 @@ | |||
11 | [ssh-add.c] | 11 | [ssh-add.c] |
12 | check that the certificate matches the corresponding private key before | 12 | check that the certificate matches the corresponding private key before |
13 | grafting it on | 13 | grafting it on |
14 | - djm@cvs.openbsd.org 2010/05/14 23:29:23 | ||
15 | [channels.c channels.h mux.c ssh.c] | ||
16 | Pause the mux channel while waiting for reply from aynch callbacks. | ||
17 | Prevents misordering of replies if new requests arrive while waiting. | ||
18 | |||
19 | Extend channel open confirm callback to allow signalling failure | ||
20 | conditions as well as success. Use this to 1) fix a memory leak, 2) | ||
21 | start using the above pause mechanism and 3) delay sending a success/ | ||
22 | failure message on mux slave session open until we receive a reply from | ||
23 | the server. | ||
24 | |||
25 | motivated by and with feedback from markus@ | ||
14 | 26 | ||
15 | 20100511 | 27 | 20100511 |
16 | - (dtucker) [Makefile.in] Bug #1770: Link libopenbsd-compat twice to solve | 28 | - (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 @@ | |||
1 | /* $OpenBSD: channels.c,v 1.303 2010/01/30 21:12:08 djm Exp $ */ | 1 | /* $OpenBSD: channels.c,v 1.304 2010/05/14 23:29:23 djm Exp $ */ |
2 | /* | 2 | /* |
3 | * Author: Tatu Ylonen <ylo@cs.hut.fi> | 3 | * Author: Tatu Ylonen <ylo@cs.hut.fi> |
4 | * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland | 4 | * 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, | |||
330 | c->ctl_chan = -1; | 330 | c->ctl_chan = -1; |
331 | c->mux_rcb = NULL; | 331 | c->mux_rcb = NULL; |
332 | c->mux_ctx = NULL; | 332 | c->mux_ctx = NULL; |
333 | c->mux_pause = 0; | ||
333 | c->delayed = 1; /* prevent call to channel_post handler */ | 334 | c->delayed = 1; /* prevent call to channel_post handler */ |
334 | TAILQ_INIT(&c->status_confirms); | 335 | TAILQ_INIT(&c->status_confirms); |
335 | debug("channel %d: new [%s]", found, remote_name); | 336 | debug("channel %d: new [%s]", found, remote_name); |
@@ -703,7 +704,7 @@ channel_register_status_confirm(int id, channel_confirm_cb *cb, | |||
703 | } | 704 | } |
704 | 705 | ||
705 | void | 706 | void |
706 | channel_register_open_confirm(int id, channel_callback_fn *fn, void *ctx) | 707 | channel_register_open_confirm(int id, channel_open_fn *fn, void *ctx) |
707 | { | 708 | { |
708 | Channel *c = channel_lookup(id); | 709 | Channel *c = channel_lookup(id); |
709 | 710 | ||
@@ -991,7 +992,7 @@ channel_pre_x11_open(Channel *c, fd_set *readset, fd_set *writeset) | |||
991 | static void | 992 | static void |
992 | channel_pre_mux_client(Channel *c, fd_set *readset, fd_set *writeset) | 993 | channel_pre_mux_client(Channel *c, fd_set *readset, fd_set *writeset) |
993 | { | 994 | { |
994 | if (c->istate == CHAN_INPUT_OPEN && | 995 | if (c->istate == CHAN_INPUT_OPEN && !c->mux_pause && |
995 | buffer_check_alloc(&c->input, CHAN_RBUF)) | 996 | buffer_check_alloc(&c->input, CHAN_RBUF)) |
996 | FD_SET(c->rfd, readset); | 997 | FD_SET(c->rfd, readset); |
997 | if (c->istate == CHAN_INPUT_WAIT_DRAIN) { | 998 | if (c->istate == CHAN_INPUT_WAIT_DRAIN) { |
@@ -1840,7 +1841,7 @@ channel_post_mux_client(Channel *c, fd_set *readset, fd_set *writeset) | |||
1840 | if (!compat20) | 1841 | if (!compat20) |
1841 | fatal("%s: entered with !compat20", __func__); | 1842 | fatal("%s: entered with !compat20", __func__); |
1842 | 1843 | ||
1843 | if (c->rfd != -1 && FD_ISSET(c->rfd, readset) && | 1844 | if (c->rfd != -1 && !c->mux_pause && FD_ISSET(c->rfd, readset) && |
1844 | (c->istate == CHAN_INPUT_OPEN || | 1845 | (c->istate == CHAN_INPUT_OPEN || |
1845 | c->istate == CHAN_INPUT_WAIT_DRAIN)) { | 1846 | c->istate == CHAN_INPUT_WAIT_DRAIN)) { |
1846 | /* | 1847 | /* |
@@ -2463,7 +2464,7 @@ channel_input_open_confirmation(int type, u_int32_t seq, void *ctxt) | |||
2463 | c->remote_maxpacket = packet_get_int(); | 2464 | c->remote_maxpacket = packet_get_int(); |
2464 | if (c->open_confirm) { | 2465 | if (c->open_confirm) { |
2465 | debug2("callback start"); | 2466 | debug2("callback start"); |
2466 | c->open_confirm(c->self, c->open_confirm_ctx); | 2467 | c->open_confirm(c->self, 1, c->open_confirm_ctx); |
2467 | debug2("callback done"); | 2468 | debug2("callback done"); |
2468 | } | 2469 | } |
2469 | debug2("channel %d: open confirm rwindow %u rmax %u", c->self, | 2470 | 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) | |||
2514 | xfree(msg); | 2515 | xfree(msg); |
2515 | if (lang != NULL) | 2516 | if (lang != NULL) |
2516 | xfree(lang); | 2517 | xfree(lang); |
2518 | if (c->open_confirm) { | ||
2519 | debug2("callback start"); | ||
2520 | c->open_confirm(c->self, 0, c->open_confirm_ctx); | ||
2521 | debug2("callback done"); | ||
2522 | } | ||
2517 | } | 2523 | } |
2518 | packet_check_eom(); | 2524 | packet_check_eom(); |
2519 | /* Schedule the channel for cleanup/deletion. */ | 2525 | /* 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 @@ | |||
1 | /* $OpenBSD: channels.h,v 1.103 2010/01/26 01:28:35 djm Exp $ */ | 1 | /* $OpenBSD: channels.h,v 1.104 2010/05/14 23:29:23 djm Exp $ */ |
2 | 2 | ||
3 | /* | 3 | /* |
4 | * Author: Tatu Ylonen <ylo@cs.hut.fi> | 4 | * Author: Tatu Ylonen <ylo@cs.hut.fi> |
@@ -60,6 +60,7 @@ | |||
60 | struct Channel; | 60 | struct Channel; |
61 | typedef struct Channel Channel; | 61 | typedef struct Channel Channel; |
62 | 62 | ||
63 | typedef void channel_open_fn(int, int, void *); | ||
63 | typedef void channel_callback_fn(int, void *); | 64 | typedef void channel_callback_fn(int, void *); |
64 | typedef int channel_infilter_fn(struct Channel *, char *, int); | 65 | typedef int channel_infilter_fn(struct Channel *, char *, int); |
65 | typedef void channel_filter_cleanup_fn(int, void *); | 66 | typedef void channel_filter_cleanup_fn(int, void *); |
@@ -130,7 +131,7 @@ struct Channel { | |||
130 | char *ctype; /* type */ | 131 | char *ctype; /* type */ |
131 | 132 | ||
132 | /* callback */ | 133 | /* callback */ |
133 | channel_callback_fn *open_confirm; | 134 | channel_open_fn *open_confirm; |
134 | void *open_confirm_ctx; | 135 | void *open_confirm_ctx; |
135 | channel_callback_fn *detach_user; | 136 | channel_callback_fn *detach_user; |
136 | int detach_close; | 137 | int detach_close; |
@@ -151,6 +152,7 @@ struct Channel { | |||
151 | /* multiplexing protocol hook, called for each packet received */ | 152 | /* multiplexing protocol hook, called for each packet received */ |
152 | mux_callback_fn *mux_rcb; | 153 | mux_callback_fn *mux_rcb; |
153 | void *mux_ctx; | 154 | void *mux_ctx; |
155 | int mux_pause; | ||
154 | }; | 156 | }; |
155 | 157 | ||
156 | #define CHAN_EXTENDED_IGNORE 0 | 158 | #define CHAN_EXTENDED_IGNORE 0 |
@@ -208,7 +210,7 @@ void channel_stop_listening(void); | |||
208 | void channel_send_open(int); | 210 | void channel_send_open(int); |
209 | void channel_request_start(int, char *, int); | 211 | void channel_request_start(int, char *, int); |
210 | void channel_register_cleanup(int, channel_callback_fn *, int); | 212 | void channel_register_cleanup(int, channel_callback_fn *, int); |
211 | void channel_register_open_confirm(int, channel_callback_fn *, void *); | 213 | void channel_register_open_confirm(int, channel_open_fn *, void *); |
212 | void channel_register_filter(int, channel_infilter_fn *, | 214 | void channel_register_filter(int, channel_infilter_fn *, |
213 | channel_outfilter_fn *, channel_filter_cleanup_fn *, void *); | 215 | channel_outfilter_fn *, channel_filter_cleanup_fn *, void *); |
214 | void channel_register_status_confirm(int, channel_confirm_cb *, | 216 | void channel_register_status_confirm(int, channel_confirm_cb *, |
@@ -1,4 +1,4 @@ | |||
1 | /* $OpenBSD: mux.c,v 1.16 2010/04/23 22:27:38 djm Exp $ */ | 1 | /* $OpenBSD: mux.c,v 1.17 2010/05/14 23:29:23 djm Exp $ */ |
2 | /* | 2 | /* |
3 | * Copyright (c) 2002-2008 Damien Miller <djm@openbsd.org> | 3 | * Copyright (c) 2002-2008 Damien Miller <djm@openbsd.org> |
4 | * | 4 | * |
@@ -106,6 +106,7 @@ struct mux_session_confirm_ctx { | |||
106 | char *term; | 106 | char *term; |
107 | struct termios tio; | 107 | struct termios tio; |
108 | char **env; | 108 | char **env; |
109 | u_int rid; | ||
109 | }; | 110 | }; |
110 | 111 | ||
111 | /* fd to control socket */ | 112 | /* fd to control socket */ |
@@ -149,7 +150,7 @@ struct mux_master_state { | |||
149 | #define MUX_FWD_REMOTE 2 | 150 | #define MUX_FWD_REMOTE 2 |
150 | #define MUX_FWD_DYNAMIC 3 | 151 | #define MUX_FWD_DYNAMIC 3 |
151 | 152 | ||
152 | static void mux_session_confirm(int, void *); | 153 | static void mux_session_confirm(int, int, void *); |
153 | 154 | ||
154 | static int process_mux_master_hello(u_int, Channel *, Buffer *, Buffer *); | 155 | static int process_mux_master_hello(u_int, Channel *, Buffer *, Buffer *); |
155 | static int process_mux_new_session(u_int, Channel *, Buffer *, Buffer *); | 156 | 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) | |||
301 | /* Reply for SSHMUX_COMMAND_OPEN */ | 302 | /* Reply for SSHMUX_COMMAND_OPEN */ |
302 | cctx = xcalloc(1, sizeof(*cctx)); | 303 | cctx = xcalloc(1, sizeof(*cctx)); |
303 | cctx->term = NULL; | 304 | cctx->term = NULL; |
305 | cctx->rid = rid; | ||
304 | cmd = reserved = NULL; | 306 | cmd = reserved = NULL; |
305 | if ((reserved = buffer_get_string_ret(m, NULL)) == NULL || | 307 | if ((reserved = buffer_get_string_ret(m, NULL)) == NULL || |
306 | buffer_get_int_ret(&cctx->want_tty, m) != 0 || | 308 | 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) | |||
454 | 456 | ||
455 | channel_send_open(nc->self); | 457 | channel_send_open(nc->self); |
456 | channel_register_open_confirm(nc->self, mux_session_confirm, cctx); | 458 | channel_register_open_confirm(nc->self, mux_session_confirm, cctx); |
459 | c->mux_pause = 1; /* stop handling messages until open_confirm done */ | ||
457 | channel_register_cleanup(nc->self, mux_master_session_cleanup_cb, 1); | 460 | channel_register_cleanup(nc->self, mux_master_session_cleanup_cb, 1); |
458 | 461 | ||
459 | /* prepare reply */ | 462 | /* reply is deferred, sent by mux_session_confirm */ |
460 | /* XXX defer until mux_session_confirm() fires */ | ||
461 | buffer_put_int(r, MUX_S_SESSION_OPENED); | ||
462 | buffer_put_int(r, rid); | ||
463 | buffer_put_int(r, nc->self); | ||
464 | |||
465 | return 0; | 463 | return 0; |
466 | } | 464 | } |
467 | 465 | ||
@@ -1000,17 +998,31 @@ muxserver_listen(void) | |||
1000 | 998 | ||
1001 | /* Callback on open confirmation in mux master for a mux client session. */ | 999 | /* Callback on open confirmation in mux master for a mux client session. */ |
1002 | static void | 1000 | static void |
1003 | mux_session_confirm(int id, void *arg) | 1001 | mux_session_confirm(int id, int success, void *arg) |
1004 | { | 1002 | { |
1005 | struct mux_session_confirm_ctx *cctx = arg; | 1003 | struct mux_session_confirm_ctx *cctx = arg; |
1006 | const char *display; | 1004 | const char *display; |
1007 | Channel *c; | 1005 | Channel *c, *cc; |
1008 | int i; | 1006 | int i; |
1007 | Buffer reply; | ||
1009 | 1008 | ||
1010 | if (cctx == NULL) | 1009 | if (cctx == NULL) |
1011 | fatal("%s: cctx == NULL", __func__); | 1010 | fatal("%s: cctx == NULL", __func__); |
1012 | if ((c = channel_by_id(id)) == NULL) | 1011 | if ((c = channel_by_id(id)) == NULL) |
1013 | fatal("%s: no channel for id %d", __func__, id); | 1012 | fatal("%s: no channel for id %d", __func__, id); |
1013 | if ((cc = channel_by_id(c->ctl_chan)) == NULL) | ||
1014 | fatal("%s: channel %d lacks control channel %d", __func__, | ||
1015 | id, c->ctl_chan); | ||
1016 | |||
1017 | if (!success) { | ||
1018 | debug3("%s: sending failure reply", __func__); | ||
1019 | /* prepare reply */ | ||
1020 | buffer_init(&reply); | ||
1021 | buffer_put_int(&reply, MUX_S_FAILURE); | ||
1022 | buffer_put_int(&reply, cctx->rid); | ||
1023 | buffer_put_cstring(&reply, "Session open refused by peer"); | ||
1024 | goto done; | ||
1025 | } | ||
1014 | 1026 | ||
1015 | display = getenv("DISPLAY"); | 1027 | display = getenv("DISPLAY"); |
1016 | if (cctx->want_x_fwd && options.forward_x11 && display != NULL) { | 1028 | if (cctx->want_x_fwd && options.forward_x11 && display != NULL) { |
@@ -1033,6 +1045,21 @@ mux_session_confirm(int id, void *arg) | |||
1033 | client_session2_setup(id, cctx->want_tty, cctx->want_subsys, | 1045 | client_session2_setup(id, cctx->want_tty, cctx->want_subsys, |
1034 | cctx->term, &cctx->tio, c->rfd, &cctx->cmd, cctx->env); | 1046 | cctx->term, &cctx->tio, c->rfd, &cctx->cmd, cctx->env); |
1035 | 1047 | ||
1048 | debug3("%s: sending success reply", __func__); | ||
1049 | /* prepare reply */ | ||
1050 | buffer_init(&reply); | ||
1051 | buffer_put_int(&reply, MUX_S_SESSION_OPENED); | ||
1052 | buffer_put_int(&reply, cctx->rid); | ||
1053 | buffer_put_int(&reply, c->self); | ||
1054 | |||
1055 | done: | ||
1056 | /* Send reply */ | ||
1057 | buffer_put_string(&cc->output, buffer_ptr(&reply), buffer_len(&reply)); | ||
1058 | buffer_free(&reply); | ||
1059 | |||
1060 | if (cc->mux_pause <= 0) | ||
1061 | fatal("%s: mux_pause %d", __func__, cc->mux_pause); | ||
1062 | cc->mux_pause = 0; /* start processing messages again */ | ||
1036 | c->open_confirm_ctx = NULL; | 1063 | c->open_confirm_ctx = NULL; |
1037 | buffer_free(&cctx->cmd); | 1064 | buffer_free(&cctx->cmd); |
1038 | xfree(cctx->term); | 1065 | xfree(cctx->term); |
@@ -1,4 +1,4 @@ | |||
1 | /* $OpenBSD: ssh.c,v 1.336 2010/04/10 00:00:16 djm Exp $ */ | 1 | /* $OpenBSD: ssh.c,v 1.337 2010/05/14 23:29:23 djm Exp $ */ |
2 | /* | 2 | /* |
3 | * Author: Tatu Ylonen <ylo@cs.hut.fi> | 3 | * Author: Tatu Ylonen <ylo@cs.hut.fi> |
4 | * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland | 4 | * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland |
@@ -1175,12 +1175,15 @@ ssh_session(void) | |||
1175 | 1175 | ||
1176 | /* request pty/x11/agent/tcpfwd/shell for channel */ | 1176 | /* request pty/x11/agent/tcpfwd/shell for channel */ |
1177 | static void | 1177 | static void |
1178 | ssh_session2_setup(int id, void *arg) | 1178 | ssh_session2_setup(int id, int success, void *arg) |
1179 | { | 1179 | { |
1180 | extern char **environ; | 1180 | extern char **environ; |
1181 | const char *display; | 1181 | const char *display; |
1182 | int interactive = tty_flag; | 1182 | int interactive = tty_flag; |
1183 | 1183 | ||
1184 | if (!success) | ||
1185 | return; /* No need for error message, channels code sens one */ | ||
1186 | |||
1184 | display = getenv("DISPLAY"); | 1187 | display = getenv("DISPLAY"); |
1185 | if (options.forward_x11 && display != NULL) { | 1188 | if (options.forward_x11 && display != NULL) { |
1186 | char *proto, *data; | 1189 | char *proto, *data; |