plugin: Fix hanging hook calls if the plugin dies

Changelog-Fixed: plugin: A crashing plugin will no longer cause a hook call to be delayed indefinitely

lightningd/plugin_hook.c

@@ -1,4 +1,5 @@
 #include <ccan/io/io.h>
+#include <ccan/list/list.h>
 #include <common/configdir.h>
 #include <common/memleak.h>
 #include <lightningd/jsonrpc.h>
@@ -9,6 +10,7 @@
 /* Struct containing all the information needed to deserialize and
  * dispatch an eventual plugin_hook response. */
 struct plugin_hook_request {
+	struct list_head call_chain;
 	struct plugin *plugin;
 	int current_plugin;
 	const struct plugin_hook *hook;
@@ -17,6 +19,18 @@ struct plugin_hook_request {
 	struct db *db;
 };
 
+/* A link in the plugin_hook call chain (there's a joke in there about
+ * computer scientists and naming...). The purpose is to act both as a list
+ * from which elements can be popped off as we progress along the chain as
+ * well as have a way for plugins to notify about their untimely demise during
+ * a hook call.
+ */
+struct plugin_hook_call_link {
+	struct list_node list;
+	struct plugin *plugin;
+	struct plugin_hook_request *req;
+};
+
 static struct plugin_hook *plugin_hook_by_name(const char *name)
 {
 	static struct plugin_hook **hooks = NULL;
@@ -89,6 +103,43 @@ void plugin_hook_unregister_all(struct plugin *plugin)
 
 /* Mutual recursion */
 static void plugin_hook_call_next(struct plugin_hook_request *ph_req);
+static void plugin_hook_callback(const char *buffer, const jsmntok_t *toks,
+				 const jsmntok_t *idtok,
+				 struct plugin_hook_request *r);
+
+/* We get notified if a plugin was killed while it was part of a call
+ * chain. If it was still to be called we just remove it from the list,
+ * otherwise it was the plugin that was currently handling the hook call, and
+ * we need to fail over to the next plugin.
+*/
+static void plugin_hook_killed(struct plugin_hook_call_link *link)
+{
+	struct plugin_hook_call_link *head;
+
+	head = list_top(&link->req->call_chain, struct plugin_hook_call_link,
+			list);
+
+	/* If we are the head of the call chain, then the plugin died while it
+	 * was handling the hook call. Pretend it didn't get the memo by
+	 * calling the next one instead. This is correct since it is
+	 * equivalent to the plugin dying before the hook invokation, assuming
+	 * the plugin has not commmitted any changes internally. This is the
+	 * weakest assumption we can make short of restarting the plugin and
+	 * calling the hook again (potentially crashing the plugin the same
+	 * way again.
+	 */
+	if (link == head) {
+		/* Call next will unlink, so we don't need to. This is treated
+		 * equivalent to the plugin returning a continue-result.
+		 */
+		link->req->current_plugin--;
+		plugin_hook_callback(NULL, NULL, NULL, link->req);
+	} else {
+		/* The plugin is in the list waiting to be called, just remove
+		 * it from the list. */
+		list_del(&link->list);
+	}
+}
 
 /**
  * Callback to be passed to the jsonrpc_request.
@@ -102,22 +153,42 @@ static void plugin_hook_callback(const char *buffer, const jsmntok_t *toks,
 {
 	const jsmntok_t *resulttok, *resrestok;
 	struct db *db = r->db;
-	bool more_plugins = r->current_plugin + 1 < tal_count(r->hook->plugins);
+	bool more_plugins, cont;
+	struct plugin_hook_call_link *last;
 
-	resulttok = json_get_member(buffer, toks, "result");
+	/* Pop the head off the call chain and continue with the next */
+	last = list_pop(&r->call_chain, struct plugin_hook_call_link, list);
+	assert(last != NULL);
+	tal_del_destructor(last, plugin_hook_killed);
+	tal_free(last);
 
-	if (!resulttok)
-		fatal("Plugin for %s returned non-result response %.*s",
-		      r->hook->name, toks->end - toks->start,
-		      buffer + toks->start);
+	if (buffer) {
+		resulttok = json_get_member(buffer, toks, "result");
 
-	resrestok = json_get_member(buffer, resulttok, "result");
+		if (!resulttok)
+			fatal("Plugin for %s returned non-result response %.*s",
+			      r->hook->name, toks->end - toks->start,
+			      buffer + toks->start);
+
+		resrestok = json_get_member(buffer, resulttok, "result");
+	} else {
+		/* Buffer and / or resulttok could be used by the reponse_cb
+		 * to identify no-result responses. So make sure both are
+		 * set */
+		resulttok = NULL;
+		/* cppcheck isn't smart enough to notice that `resrestok`
+		 * doesn't need to be initialized in the expression
+		 * initializing `cont`, so set it to NULL to shut it up. */
+		resrestok = NULL;
+	}
+
+	more_plugins = r->current_plugin + 1 < tal_count(r->hook->plugins);
+	cont = buffer == NULL || (resrestok && json_tok_streq(buffer, resrestok, "continue"));
 
 	/* If this is a hook response containing a `continue` and we have more
 	 * plugins queue the next call. In that case we discard the remainder
 	 * of the result, and let the next plugin decide. */
-	if (resrestok && json_tok_streq(buffer, resrestok, "continue") &&
-	    more_plugins) {
+	if (cont && more_plugins) {
 		plugin_hook_call_next(r);
 	} else {
 		db_begin_transaction(db);
@@ -148,6 +219,7 @@ void plugin_hook_call_(struct lightningd *ld, const struct plugin_hook *hook,
 		       void *payload, void *cb_arg)
 {
 	struct plugin_hook_request *ph_req;
+	struct plugin_hook_call_link *link;
 	if (tal_count(hook->plugins)) {
 		/* If we have a plugin that has registered for this
 		 * hook, serialize and call it */
@@ -160,6 +232,16 @@ void plugin_hook_call_(struct lightningd *ld, const struct plugin_hook *hook,
 		ph_req->db = ld->wallet->db;
 		ph_req->payload = tal_steal(ph_req, payload);
 		ph_req->current_plugin = -1;
+
+		list_head_init(&ph_req->call_chain);
+		for (size_t i=0; i<tal_count(hook->plugins); i++) {
+			/* We allocate this off of the plugin so we get notified if the plugin dies. */
+			link = tal(hook->plugins[i], struct plugin_hook_call_link);
+			link->plugin = hook->plugins[i];
+			link->req = ph_req;
+			tal_add_destructor(link, plugin_hook_killed);
+			list_add_tail(&ph_req->call_chain, &link->list);
+		}
 		plugin_hook_call_next(ph_req);
 	} else {
 		/* If no plugin has registered for this hook, just

tests/test_plugin.py

@@ -1014,7 +1014,6 @@ def test_bcli(node_factory, bitcoind, chainparams):
     assert not resp["success"] and "decode failed" in resp["errmsg"]
 
 
-@pytest.mark.xfail(strict=True)
 def test_hook_crash(node_factory, executor, bitcoind):
     """Verify that we fail over if a plugin crashes while handling a hook.