From 3564263e1255694c19b60b79bfa28e8d9f53cc44 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 28 Sep 2017 13:01:36 +0930 Subject: [PATCH] jsonrpc: use-after-free bug due to unspecified free behavior 1/2 These were fun to hunt down. The jcon and the conn are allocated off of ld, so the free order is unspecified and if conn is freed before conn then the finish_jcon destructor uses conn after free. [ Edit: split commit, modified to use a destructor directly on jcon, which is more robust than relying on it only being freed via conn --RR ] Signed-off-by: Christian Decker --- lightningd/jsonrpc.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c index 6bb3a2428..efd404e7d 100644 --- a/lightningd/jsonrpc.c +++ b/lightningd/jsonrpc.c @@ -25,14 +25,15 @@ struct json_output { const char *json; }; -static void finish_jcon(struct io_conn *conn, struct json_connection *jcon) +static void destroy_jcon(struct json_connection *jcon) { log_debug(jcon->log, "Closing (%s)", strerror(errno)); if (jcon->current) { log_unusual(jcon->log, "Abandoning current command"); jcon->current->jcon = NULL; } - tal_free(jcon); + /* Make sure this happens last! */ + tal_free(jcon->log); } static void json_help(struct command *cmd, @@ -576,17 +577,18 @@ static struct io_plan *jcon_connected(struct io_conn *conn, { struct json_connection *jcon; - jcon = tal(ld, struct json_connection); + jcon = tal(conn, struct json_connection); jcon->ld = ld; jcon->used = 0; jcon->buffer = tal_arr(jcon, char, 64); jcon->stop = false; jcon->current = NULL; - jcon->log = new_log(jcon, ld->log_book, "%sjcon fd %i:", + /* We want to log on destruction, so we free this in destructor. */ + jcon->log = new_log(ld->log_book, ld->log_book, "%sjcon fd %i:", log_prefix(ld->log), io_conn_fd(conn)); list_head_init(&jcon->output); - io_set_finish(conn, finish_jcon, jcon); + tal_add_destructor(jcon, destroy_jcon); return io_duplex(conn, io_read_partial(conn, jcon->buffer,