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

@@ -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.,

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;

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.
  *

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(); }