Attachment 964 Details for Bug 1086 – Probable fix

[patch] Probable fix

session-x11-detach.diff (text/plain), 13.28 KB, created by Damien Miller on 2005-09-24 11:47:31 AEST

(hide)

Description:

Filename:

MIME Type:

Creator: Damien Miller

Created: 2005-09-24 11:47:31 AEST

Size: 13.28 KB

patch

obsolete

>Index: Makefile.inc
>===================================================================
>RCS file: /cvs/src/usr.bin/ssh/Makefile.inc,v
>retrieving revision 1.30
>diff -u -p -r1.30 Makefile.inc
>--- Makefile.inc	17 Jun 2005 02:45:19 -0000	1.30
>+++ Makefile.inc	24 Sep 2005 01:44:10 -0000
>@@ -3,7 +3,7 @@
> CFLAGS+=	-I${.CURDIR}/..
> 
> CDIAGFLAGS=	-Wall
>-#CDIAGFLAGS+=	-Werror
>+CDIAGFLAGS+=	-Werror
> CDIAGFLAGS+=	-Wpointer-arith
> CDIAGFLAGS+=	-Wno-uninitialized
> CDIAGFLAGS+=	-Wstrict-prototypes
>Index: channels.c
>===================================================================
>RCS file: /cvs/src/usr.bin/ssh/channels.c,v
>retrieving revision 1.224
>diff -u -p -r1.224 channels.c
>--- channels.c	7 Sep 2005 08:53:53 -0000	1.224
>+++ channels.c	24 Sep 2005 01:44:11 -0000
>@@ -268,6 +268,7 @@ channel_new(char *ctype, int type, int r
> 	c->force_drain = 0;
> 	c->single_connection = 0;
> 	c->detach_user = NULL;
>+	c->detach_close = NULL;
> 	c->confirm = NULL;
> 	c->confirm_ctx = NULL;
> 	c->input_filter = NULL;
>@@ -627,7 +628,7 @@ channel_register_confirm(int id, channel
> 	c->confirm_ctx = ctx;
> }
> void
>-channel_register_cleanup(int id, channel_callback_fn *fn)
>+channel_register_cleanup(int id, channel_callback_fn *fn, int do_close)
> {
> 	Channel *c = channel_lookup(id);
> 
>@@ -636,6 +637,7 @@ channel_register_cleanup(int id, channel
> 		return;
> 	}
> 	c->detach_user = fn;
>+	c->detach_close = do_close;
> }
> void
> channel_cancel_cleanup(int id)
>@@ -647,6 +649,7 @@ channel_cancel_cleanup(int id)
> 		return;
> 	}
> 	c->detach_user = NULL;
>+	c->detach_close = 0;
> }
> void
> channel_register_filter(int id, channel_filter_fn *fn)
>@@ -1660,7 +1663,7 @@ channel_garbage_collect(Channel *c)
> 	if (c == NULL)
> 		return;
> 	if (c->detach_user != NULL) {
>-		if (!chan_is_dead(c, 0))
>+		if (!chan_is_dead(c, c->detach_close))
> 			return;
> 		debug2("channel %d: gc: notify user", c->self);
> 		c->detach_user(c->self, NULL);
>Index: channels.h
>===================================================================
>RCS file: /cvs/src/usr.bin/ssh/channels.h,v
>retrieving revision 1.79
>diff -u -p -r1.79 channels.h
>--- channels.h	17 Jul 2005 06:49:04 -0000	1.79
>+++ channels.h	24 Sep 2005 01:44:11 -0000
>@@ -105,8 +105,9 @@ struct Channel {
> 
> 	/* callback */
> 	channel_callback_fn	*confirm;
>-	channel_callback_fn	*detach_user;
> 	void			*confirm_ctx;
>+	channel_callback_fn	*detach_user;
>+	int			detach_close;
> 
> 	/* filter */
> 	channel_filter_fn	*input_filter;
>@@ -162,7 +163,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 *);
>+void	 channel_register_cleanup(int, channel_callback_fn *, int);
> void	 channel_register_confirm(int, channel_callback_fn *, void *);
> void	 channel_register_filter(int, channel_filter_fn *);
> void	 channel_cancel_cleanup(int);
>Index: clientloop.c
>===================================================================
>RCS file: /cvs/src/usr.bin/ssh/clientloop.c,v
>retrieving revision 1.142
>diff -u -p -r1.142 clientloop.c
>--- clientloop.c	9 Sep 2005 19:18:05 -0000	1.142
>+++ clientloop.c	24 Sep 2005 01:44:12 -0000
>@@ -1379,7 +1379,7 @@ client_loop(int have_pty, int escape_cha
> 			    simple_escape_filter);
> 		if (session_ident != -1)
> 			channel_register_cleanup(session_ident,
>-			    client_channel_closed);
>+			    client_channel_closed, 0);
> 	} else {
> 		/* Check if we should immediately send eof on stdin. */
> 		client_check_initial_eof_on_stdin();
>Index: serverloop.c
>===================================================================
>RCS file: /cvs/src/usr.bin/ssh/serverloop.c,v
>retrieving revision 1.118
>diff -u -p -r1.118 serverloop.c
>--- serverloop.c	17 Jul 2005 07:17:55 -0000	1.118
>+++ serverloop.c	24 Sep 2005 01:44:12 -0000
>@@ -898,7 +898,7 @@ server_request_session(void)
> 		channel_free(c);
> 		return NULL;
> 	}
>-	channel_register_cleanup(c->self, session_close_by_channel);
>+	channel_register_cleanup(c->self, session_close_by_channel, 0);
> 	return c;
> }
> 
>Index: session.c
>===================================================================
>RCS file: /cvs/src/usr.bin/ssh/session.c,v
>retrieving revision 1.186
>diff -u -p -r1.186 session.c
>--- session.c	25 Jul 2005 11:59:40 -0000	1.186
>+++ session.c	24 Sep 2005 01:44:12 -0000
>@@ -100,6 +100,19 @@ extern Buffer loginmsg;
> /* original command from peer. */
> const char *original_command = NULL;
> 
>+/*
>+ * This list tracks X11 channels, which must have a lifetime dependent on a 
>+ * session's _channel_, which may outlive the session itself.
>+ */
>+struct x11_channels {
>+	TAILQ_ENTRY(x11_channels) next;
>+	Session	*session;
>+	int	channel_owner;
>+	int	*x11_chanids;
>+};
>+TAILQ_HEAD(x11_chans, x11_channels);
>+static struct x11_chans x11_chans = TAILQ_HEAD_INITIALIZER(x11_chans);
>+
> /* data */
> #define MAX_SESSIONS 10
> Session	sessions[MAX_SESSIONS];
>@@ -1271,14 +1284,14 @@ session_new(void)
> 	}
> 	for (i = 0; i < MAX_SESSIONS; i++) {
> 		Session *s = &sessions[i];
>-		if (! s->used) {
>+		if (!s->used) {
> 			memset(s, 0, sizeof(*s));
> 			s->chanid = -1;
> 			s->ptyfd = -1;
> 			s->ttyfd = -1;
> 			s->used = 1;
> 			s->self = i;
>-			s->x11_chanids = NULL;
>+			s->x11 = NULL;
> 			debug("session_new: session %d", i);
> 			return s;
> 		}
>@@ -1290,6 +1303,8 @@ static void
> session_dump(void)
> {
> 	int i;
>+	struct x11_channels *x11c;
>+
> 	for (i = 0; i < MAX_SESSIONS; i++) {
> 		Session *s = &sessions[i];
> 		debug("dump: used %d session %d %p channel %d pid %ld",
>@@ -1299,6 +1314,11 @@ session_dump(void)
> 		    s->chanid,
> 		    (long)s->pid);
> 	}
>+	TAILQ_FOREACH(x11c, &x11_chans, next) {
>+		debug("x11 dump: channel owner %d session %d",
>+		    x11c->channel_owner, 
>+		    x11c->session == NULL ? -1 : x11c->session->self);
>+	}
> }
> 
> int
>@@ -1352,29 +1372,6 @@ session_by_channel(int id)
> }
> 
> static Session *
>-session_by_x11_channel(int id)
>-{
>-	int i, j;
>-
>-	for (i = 0; i < MAX_SESSIONS; i++) {
>-		Session *s = &sessions[i];
>-
>-		if (s->x11_chanids == NULL || !s->used)
>-			continue;
>-		for (j = 0; s->x11_chanids[j] != -1; j++) {
>-			if (s->x11_chanids[j] == id) {
>-				debug("session_by_x11_channel: session %d "
>-				    "channel %d", s->self, id);
>-				return s;
>-			}
>-		}
>-	}
>-	debug("session_by_x11_channel: unknown channel %d", id);
>-	session_dump();
>-	return NULL;
>-}
>-
>-static Session *
> session_by_pid(pid_t pid)
> {
> 	int i;
>@@ -1389,6 +1386,75 @@ session_by_pid(pid_t pid)
> 	return NULL;
> }
> 
>+/* Allocate a X11 channel tracking entry and attach it to a session */
>+static void
>+session_x11_chantrack_attach(Session *s, int *x11_channels)
>+{
>+	struct x11_channels *x11c;
>+
>+	if (s == NULL || s->chanid == -1 || x11_channels == NULL)
>+		return;
>+
>+	x11c = xmalloc(sizeof(*x11c));
>+	x11c->session = s;
>+	x11c->channel_owner = s->chanid;
>+	x11c->x11_chanids = x11_channels;
>+	
>+	TAILQ_INSERT_TAIL(&x11_chans, x11c, next);
>+	s->x11 = x11c;
>+}
>+
>+/* Deallocate a channel tracker */
>+static void
>+session_x11_chantrack_free(struct x11_channels *x11c)
>+{
>+	debug("session_x11_chantrack_free: channel_owner %d",
>+	    x11c->channel_owner);
>+	TAILQ_REMOVE(&x11_chans, x11c, next);
>+	xfree(x11c->x11_chanids);
>+	xfree(x11c);
>+}
>+
>+/* Look up an X11 channel tracker by its channel owner */
>+static struct x11_channels *
>+session_x11_chantrack_by_cid(int id)
>+{
>+	struct x11_channels *x11c;
>+
>+	TAILQ_FOREACH(x11c, &x11_chans, next) {
>+		if (x11c->channel_owner == id) {
>+			debug("session_x11_chantrack_by_cid: channel %d", id);
>+			return (x11c);
>+		}
>+	}
>+
>+	debug("session_x11_chantrack_by_cid: unknown channel %d", id);
>+	session_dump();
>+	return (NULL);
>+}
>+
>+/* Look up an X11 channel tracker by one of its X11 channels */
>+static struct x11_channels *
>+session_x11_chantrack_by_xcid(int id)
>+{
>+	struct x11_channels *x11c;
>+	u_int i;
>+
>+	TAILQ_FOREACH(x11c, &x11_chans, next) {
>+		for (i = 0; x11c->x11_chanids[i] != -1; i++) {
>+			if (x11c->x11_chanids[i] == id) {
>+				debug("session_x11_chantrack_by_xcid: "
>+				    "channel %d", id);
>+				return (x11c);
>+			}
>+		}
>+	}
>+
>+	debug("session_x11_chantrack_by_xcid: unknown channel %d", id);
>+	session_dump();
>+	return (NULL);
>+}
>+
> static int
> session_window_change_req(Session *s)
> {
>@@ -1748,51 +1814,77 @@ session_close_x11(int id)
> 	}
> }
> 
>+/* Channel cleanup callback to destroy X11 listener siblings when one closes */
> static void
> session_close_single_x11(int id, void *arg)
> {
>-	Session *s;
>+	struct x11_channels *x11c;
> 	u_int i;
> 
> 	debug3("session_close_single_x11: channel %d", id);
> 	channel_cancel_cleanup(id);
>-	if ((s  = session_by_x11_channel(id)) == NULL)
>+	if ((x11c  = session_x11_chantrack_by_xcid(id)) == NULL)
> 		fatal("session_close_single_x11: no x11 channel %d", id);
>-	for (i = 0; s->x11_chanids[i] != -1; i++) {
>+	for (i = 0; x11c->x11_chanids[i] != -1; i++) {
> 		debug("session_close_single_x11: session %d: "
>-		    "closing channel %d", s->self, s->x11_chanids[i]);
>+		    "closing channel %d", x11c->session->self,
>+		    x11c->x11_chanids[i]);
> 		/*
> 		 * The channel "id" is already closing, but make sure we
> 		 * close all of its siblings.
> 		 */
>-		if (s->x11_chanids[i] != id)
>-			session_close_x11(s->x11_chanids[i]);
>-	}
>-	xfree(s->x11_chanids);
>-	s->x11_chanids = NULL;
>-	if (s->display) {
>-		xfree(s->display);
>-		s->display = NULL;
>+		if (x11c->x11_chanids[i] != id)
>+			session_close_x11(x11c->x11_chanids[i]);
> 	}
>-	if (s->auth_proto) {
>-		xfree(s->auth_proto);
>-		s->auth_proto = NULL;
>+	if (x11c->session != NULL) {
>+		if (x11c->session->display != NULL) {
>+			xfree(x11c->session->display);
>+			x11c->session->display = NULL;
>+		}
>+		if (x11c->session->auth_proto != NULL) {
>+			xfree(x11c->session->auth_proto);
>+			x11c->session->auth_proto = NULL;
>+		}
>+		if (x11c->session->auth_data != NULL) {
>+			xfree(x11c->session->auth_data);
>+			x11c->session->auth_data = NULL;
>+		}
>+		if (x11c->session->auth_display != NULL) {
>+			xfree(x11c->session->auth_display);
>+			x11c->session->auth_display = NULL;
>+		}
>+		x11c->session->x11 = NULL;
> 	}
>-	if (s->auth_data) {
>-		xfree(s->auth_data);
>-		s->auth_data = NULL;
>+	session_x11_chantrack_free(x11c);
>+}
>+
>+/* Channel callback to destroy dependant X11 listeners when owner channel closes */
>+static void
>+session_cleanup_x11_listeners(int id, void *arg)
>+{
>+	struct x11_channels *x11c;
>+	u_int i;
>+
>+	debug3("session_cleanup_x11_listeners: channel %d", id);
>+	channel_cancel_cleanup(id);
>+	if ((x11c  = session_x11_chantrack_by_cid(id)) == NULL) {
>+		error("session_close_single_x11: no x11 channel %d", id);
>+		return;
> 	}
>-	if (s->auth_display) {
>-		xfree(s->auth_display);
>-		s->auth_display = NULL;
>+
>+	for (i = 0; x11c->x11_chanids[i] != -1; i++) {
>+		debug("session_cleanup_x11_listeners: session channel %d: "
>+		    "closing associated X11 channel %d", id,
>+		    x11c->x11_chanids[i]);
>+		session_close_x11(x11c->x11_chanids[i]);
> 	}
>+	session_x11_chantrack_free(x11c);
> }
> 
> static void
> session_exit_message(Session *s, int status)
> {
> 	Channel *c;
>-	u_int i;
> 
> 	if ((c = channel_lookup(s->chanid)) == NULL)
> 		fatal("session_exit_message: session %d: no channel %d",
>@@ -1827,15 +1919,15 @@ session_exit_message(Session *s, int sta
> 	 */
> 	if (c->ostate != CHAN_OUTPUT_CLOSED)
> 		chan_write_failed(c);
>-	s->chanid = -1;
> 
>-	/* Close any X11 listeners associated with this session */
>-	if (s->x11_chanids != NULL) {
>-		for (i = 0; s->x11_chanids[i] != -1; i++) {
>-			session_close_x11(s->x11_chanids[i]);
>-			s->x11_chanids[i] = -1;
>-		}
>-	}
>+	/*
>+	 * Attach a new cleanup function to the closing channel, so it
>+	 * can take its associated X11 listeners with it when it finally 
>+	 * goes away.
>+	 */
>+	channel_register_cleanup(s->chanid, session_cleanup_x11_listeners, 1);
>+
>+	s->chanid = -1;
> }
> 
> void
>@@ -1848,10 +1940,10 @@ session_close(Session *s)
> 		session_pty_cleanup(s);
> 	if (s->term)
> 		xfree(s->term);
>+	if (s->x11)
>+		s->x11->session = NULL; /* detach */
> 	if (s->display)
> 		xfree(s->display);
>-	if (s->x11_chanids)
>-		xfree(s->x11_chanids);
> 	if (s->auth_display)
> 		xfree(s->auth_display);
> 	if (s->auth_data)
>@@ -1880,6 +1972,7 @@ session_close_by_pid(pid_t pid, int stat
> 	if (s->chanid != -1)
> 		session_exit_message(s, status);
> 	session_close(s);
>+	session_dump();
> }
> 
> /*
>@@ -1963,6 +2056,7 @@ session_setup_x11fwd(Session *s)
> 	char display[512], auth_display[512];
> 	char hostname[MAXHOSTNAMELEN];
> 	u_int i;
>+	int *x11_channels;
> 
> 	if (no_x11_forwarding_flag) {
> 		packet_send_debug("X11 forwarding disabled in user configuration file.");
>@@ -1988,13 +2082,16 @@ session_setup_x11fwd(Session *s)
> 	}
> 	if (x11_create_display_inet(options.x11_display_offset,
> 	    options.x11_use_localhost, s->single_connection,
>-	    &s->display_number, &s->x11_chanids) == -1) {
>+	    &s->display_number, &x11_channels) == -1) {
> 		debug("x11_create_display_inet failed.");
> 		return 0;
> 	}
>-	for (i = 0; s->x11_chanids[i] != -1; i++) {
>-		channel_register_cleanup(s->x11_chanids[i],
>-		    session_close_single_x11);
>+
>+	session_x11_chantrack_attach(s, x11_channels);
>+
>+	for (i = 0; x11_channels[i] != -1; i++) {
>+		channel_register_cleanup(x11_channels[i],
>+		    session_close_single_x11, 0);
> 	}
> 
> 	/* Set up a suitable value for the DISPLAY variable. */
>Index: session.h
>===================================================================
>RCS file: /cvs/src/usr.bin/ssh/session.h,v
>retrieving revision 1.25
>diff -u -p -r1.25 session.h
>--- session.h	17 Jul 2005 06:49:04 -0000	1.25
>+++ session.h	24 Sep 2005 01:44:12 -0000
>@@ -49,7 +49,7 @@ struct Session {
> 	int	single_connection;
> 	/* proto 2 */
> 	int	chanid;
>-	int	*x11_chanids;
>+	struct x11_channels *x11;
> 	int	is_subsystem;
> 	u_int	num_env;
> 	struct {

Actions: View | Diff

Attachments on bug 1086: 964 | 965 | 967