From 4520f73c96909956d605370fa4c9728c771c6b57 Mon Sep 17 00:00:00 2001
From: Christian Decker <decker.christian@gmail.com>
Date: Sun, 12 Apr 2020 15:42:41 +0200
Subject: [PATCH] plugins: Fix undefined deallocation order in `struct plugins`

We use the new function `plugins_free` to define the correct deallocation
order on shutdown, since under normal operation the allocation tree is
organized to allow plugins to terminate and automatically free all dependent
resources. During shutdown the deallocation order is under-defined since
siblings may get freed in any order, but we implicitly rely on them staying
around.
---
 lightningd/lightningd.c               |  2 +-
 lightningd/plugin.c                   | 13 +++++++++++++
 lightningd/plugin.h                   | 20 ++++++++++++++++++++
 lightningd/test/run-find_my_abspath.c |  3 +++
 4 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c
index e97ea13d6..bbe66324c 100644
--- a/lightningd/lightningd.c
+++ b/lightningd/lightningd.c
@@ -997,7 +997,7 @@ int main(int argc, char *argv[])
 	shutdown_subdaemons(ld);
 
 	/* Remove plugins. */
-	ld->plugins = tal_free(ld->plugins);
+	plugins_free(ld->plugins);
 
 	/* Clean up the JSON-RPC. This needs to happen in a DB transaction since
 	 * it might actually be touching the DB in some destructors, e.g.,
diff --git a/lightningd/plugin.c b/lightningd/plugin.c
index ddd37b214..97bc6c184 100644
--- a/lightningd/plugin.c
+++ b/lightningd/plugin.c
@@ -56,6 +56,19 @@ struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book,
 	return p;
 }
 
+void plugins_free(struct plugins *plugins)
+{
+	struct plugin *p;
+	/* Plugins are usually the unit of allocation, and they are internally
+	 * consistent, so let's free each plugin first. */
+	while (!list_empty(&plugins->plugins)) {
+		p = list_pop(&plugins->plugins, struct plugin, list);
+		tal_free(p);
+	}
+
+	tal_free(plugins);
+}
+
 static void destroy_plugin(struct plugin *p)
 {
 	struct plugin_rpccall *call;
diff --git a/lightningd/plugin.h b/lightningd/plugin.h
index a95eeb5c1..1a47232dd 100644
--- a/lightningd/plugin.h
+++ b/lightningd/plugin.h
@@ -137,6 +137,26 @@ void plugins_add_default_dir(struct plugins *plugins);
  */
 void plugins_init(struct plugins *plugins, const char *dev_plugin_debug);
 
+/**
+ * Free all resources that are held by plugins in the correct order.
+ *
+ * This function ensures that the resources dangling off of the plugins struct
+ * are freed in the correct order. This is necessary since the struct manages
+ * two orthogonal sets of resources:
+ *
+ *  - Plugins
+ *  - Hook calls and notifications
+ *
+ * The tal hierarchy is organized in a plugin centric way, i.e., the plugins
+ * may exit in an arbitrary order and they'll unregister pointers in the other
+ * resources. However this will fail if `tal_free` decides to free one of the
+ * non-plugin resources (technically a sibling in the allocation tree) before
+ * the plugins we will get a use-after-free. This function fixes this by
+ * freeing in the correct order without adding additional child-relationships
+ * in the allocation structure and without adding destructors.
+ */
+void plugins_free(struct plugins *plugins);
+
 /**
  * Register a plugin for initialization and execution.
  *
diff --git a/lightningd/test/run-find_my_abspath.c b/lightningd/test/run-find_my_abspath.c
index efe6fd415..78382bb54 100644
--- a/lightningd/test/run-find_my_abspath.c
+++ b/lightningd/test/run-find_my_abspath.c
@@ -190,6 +190,9 @@ void per_peer_state_set_fds_arr(struct per_peer_state *pps UNNEEDED, const int *
 /* Generated stub for plugins_config */
 void plugins_config(struct plugins *plugins UNNEEDED)
 { fprintf(stderr, "plugins_config called!\n"); abort(); }
+/* Generated stub for plugins_free */
+void plugins_free(struct plugins *plugins UNNEEDED)
+{ fprintf(stderr, "plugins_free called!\n"); abort(); }
 /* Generated stub for plugins_init */
 void plugins_init(struct plugins *plugins UNNEEDED, const char *dev_plugin_debug UNNEEDED)
 { fprintf(stderr, "plugins_init called!\n"); abort(); }