From 9e49ec6cd325bf521f63450f3f87525cb82c63a9 Mon Sep 17 00:00:00 2001
From: Nicholas Marriott <nicm@openbsd.org>
Date: Sun, 12 Jul 2009 17:33:18 +0000
Subject: [PATCH] Creating a key binding which replaces itself (such as "bind x
 bind x lsw") frees the command list bound to the key while it is still being
 executed, leading to a use after free. To prevent this, create a dead keys
 list and defer freeing replaced or removed key bindings until the main loop
 when the key binding will have finished executing.

Found by Johan Friis when creating a key binding to reload his configuration
file.
---
 key-bindings.c | 29 +++++++++++++++++++++--------
 server.c       |  3 +++
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/key-bindings.c b/key-bindings.c
index fc232928..438836f3 100644
--- a/key-bindings.c
+++ b/key-bindings.c
@@ -27,6 +27,7 @@
 SPLAY_GENERATE(key_bindings, key_binding, entry, key_bindings_cmp);
 
 struct key_bindings	key_bindings;
+struct key_bindings	dead_key_bindings;
 
 int
 key_bindings_cmp(struct key_binding *bd1, struct key_binding *bd2)
@@ -48,12 +49,12 @@ key_bindings_add(int key, int can_repeat, struct cmd_list *cmdlist)
 {
 	struct key_binding	*bd;
 
-	if ((bd = key_bindings_lookup(key)) == NULL) {
-		bd = xmalloc(sizeof *bd);
-		bd->key = key;
-		SPLAY_INSERT(key_bindings, &key_bindings, bd);
-	} else
-		cmd_list_free(bd->cmdlist);
+	key_bindings_remove(key);
+		    
+	bd = xmalloc(sizeof *bd);
+	bd->key = key;
+	SPLAY_INSERT(key_bindings, &key_bindings, bd);
+	
 	bd->can_repeat = can_repeat;
 	bd->cmdlist = cmdlist;
 }
@@ -66,9 +67,20 @@ key_bindings_remove(int key)
 	if ((bd = key_bindings_lookup(key)) == NULL)
 		return;
 	SPLAY_REMOVE(key_bindings, &key_bindings, bd);
+	SPLAY_INSERT(key_bindings, &dead_key_bindings, bd);
+}
 
-	cmd_list_free(bd->cmdlist);
-	xfree(bd);
+void
+key_bindings_clean(void)
+{
+	struct key_binding	*bd;
+
+	while (!SPLAY_EMPTY(&dead_key_bindings)) {
+		bd = SPLAY_ROOT(&dead_key_bindings);
+		SPLAY_REMOVE(key_bindings, &dead_key_bindings, bd);
+		cmd_list_free(bd->cmdlist);
+		xfree(bd);
+	}
 }
 
 void
@@ -162,6 +174,7 @@ key_bindings_free(void)
 {
 	struct key_binding	*bd;
 
+	key_bindings_clean();
 	while (!SPLAY_EMPTY(&key_bindings)) {
 		bd = SPLAY_ROOT(&key_bindings);
 		SPLAY_REMOVE(key_bindings, &key_bindings, bd);
diff --git a/server.c b/server.c
index e9986a72..71b619ae 100644
--- a/server.c
+++ b/server.c
@@ -346,6 +346,9 @@ server_main(int srv_fd)
 		server_handle_windows(&pfd);
 		server_handle_clients(&pfd);
 
+		/* Collect any unset key bindings. */
+		key_bindings_clean();
+		
 		/*
 		 * If we have no sessions and clients left, let's get out
 		 * of here...