From 4a218835538de4d37eb563a81629fc8fcb8f924b Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Wed, 12 Feb 2020 19:44:01 +0100 Subject: [PATCH] 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 | 100 +++++++++++++++++++++++++++++++++++---- tests/test_plugin.py | 1 - 2 files changed, 91 insertions(+), 10 deletions(-) diff --git a/lightningd/plugin_hook.c b/lightningd/plugin_hook.c index 3418b53d5..9a308d13d 100644 --- a/lightningd/plugin_hook.c +++ b/lightningd/plugin_hook.c @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -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; iplugins); 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 diff --git a/tests/test_plugin.py b/tests/test_plugin.py index c77d38f7d..7a3920e95 100644 --- a/tests/test_plugin.py +++ b/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.