diff --git a/lightningd/plugin.c b/lightningd/plugin.c index ba8fa560f..50026f9e5 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -233,6 +233,7 @@ static void plugin_response_handle(struct plugin *plugin, const jsmntok_t *toks, const jsmntok_t *idtok) { + struct plugin_destroyed *pd; struct jsonrpc_request *request; u64 id; /* We only send u64 ids, so if this fails it's a critical error (note @@ -253,9 +254,13 @@ static void plugin_response_handle(struct plugin *plugin, } /* We expect the request->cb to copy if needed */ + pd = plugin_detect_destruction(plugin); request->response_cb(plugin->buffer, toks, idtok, request->response_cb_arg); - tal_free(request); + /* Note that in the case of 'plugin stop' this can free request (since + * plugin is parent), so detect that case */ + if (!was_plugin_destroyed(pd)) + tal_free(request); } /** @@ -264,10 +269,15 @@ static void plugin_response_handle(struct plugin *plugin, * Internally calls the handler if it was able to fully parse a JSON message, * and returns true in that case. */ -static bool plugin_read_json_one(struct plugin *plugin) +static bool plugin_read_json_one(struct plugin *plugin, bool *destroyed) { bool valid; const jsmntok_t *toks, *jrtok, *idtok; + struct plugin_destroyed *pd; + + *destroyed = false; + /* Note that in the case of 'plugin stop' this can free request (since + * plugin is parent), so detect that case */ /* FIXME: This could be done more efficiently by storing the * toks and doing an incremental parse, like lightning-cli @@ -300,6 +310,7 @@ static bool plugin_read_json_one(struct plugin *plugin) return false; } + pd = plugin_detect_destruction(plugin); if (!idtok) { /* A Notification is a Request object without an "id" * member. A Request object that is a Notification @@ -345,15 +356,21 @@ static bool plugin_read_json_one(struct plugin *plugin) plugin_response_handle(plugin, toks, idtok); } - /* Move this object out of the buffer */ - memmove(plugin->buffer, plugin->buffer + toks[0].end, - tal_count(plugin->buffer) - toks[0].end); - plugin->used -= toks[0].end; - tal_free(toks); + /* Corner case: rpc_command hook can destroy plugin for 'plugin + * stop'! */ + if (was_plugin_destroyed(pd)) { + *destroyed = true; + } else { + /* Move this object out of the buffer */ + memmove(plugin->buffer, plugin->buffer + toks[0].end, + tal_count(plugin->buffer) - toks[0].end); + plugin->used -= toks[0].end; + tal_free(toks); + } return true; } -static struct io_plan *plugin_read_json(struct io_conn *conn UNUSED, +static struct io_plan *plugin_read_json(struct io_conn *conn, struct plugin *plugin) { bool success; @@ -367,7 +384,12 @@ static struct io_plan *plugin_read_json(struct io_conn *conn UNUSED, /* Read and process all messages from the connection */ do { - success = plugin_read_json_one(plugin); + bool destroyed; + success = plugin_read_json_one(plugin, &destroyed); + + /* If it's destroyed, conn is already freed! */ + if (destroyed) + return io_close(NULL); /* Processing the message from the plugin might have * resulted in it stopping, so let's check. */ @@ -1203,3 +1225,32 @@ struct log *plugin_get_log(struct plugin *plugin) { return plugin->log; } + +struct plugin_destroyed { + const struct plugin *plugin; +}; + +static void mark_plugin_destroyed(const struct plugin *unused, + struct plugin_destroyed *pd) +{ + pd->plugin = NULL; +} + +struct plugin_destroyed *plugin_detect_destruction(const struct plugin *plugin) +{ + struct plugin_destroyed *pd = tal(NULL, struct plugin_destroyed); + pd->plugin = plugin; + tal_add_destructor2(plugin, mark_plugin_destroyed, pd); + return pd; +} + +bool was_plugin_destroyed(struct plugin_destroyed *pd) +{ + if (pd->plugin) { + tal_del_destructor2(pd->plugin, mark_plugin_destroyed, pd); + tal_free(pd); + return false; + } + tal_free(pd); + return true; +} diff --git a/lightningd/plugin.h b/lightningd/plugin.h index b858b1897..037b4593d 100644 --- a/lightningd/plugin.h +++ b/lightningd/plugin.h @@ -240,4 +240,9 @@ struct io_plan *plugin_stdout_conn_init(struct io_conn *conn, */ struct log *plugin_get_log(struct plugin *plugin); +/* Pair of functions to detect if plugin destroys itself: must always + * call both! */ +struct plugin_destroyed *plugin_detect_destruction(const struct plugin *plugin); +bool was_plugin_destroyed(struct plugin_destroyed *destroyed); + #endif /* LIGHTNING_LIGHTNINGD_PLUGIN_H */ diff --git a/lightningd/plugin_hook.c b/lightningd/plugin_hook.c index bd620db70..e1fd750a3 100644 --- a/lightningd/plugin_hook.c +++ b/lightningd/plugin_hook.c @@ -76,16 +76,21 @@ static void plugin_hook_callback(const char *buffer, const jsmntok_t *toks, struct plugin_hook_request *r) { const jsmntok_t *resulttok = json_get_member(buffer, toks, "result"); + struct db *db = r->db; + struct plugin_destroyed *pd; if (!resulttok) fatal("Plugin for %s returned non-result response %.*s", r->hook->name, toks->end - toks->start, buffer + toks->start); - db_begin_transaction(r->db); + /* If command is "plugin stop", this can free r! */ + pd = plugin_detect_destruction(r->hook->plugin); + db_begin_transaction(db); r->hook->response_cb(r->cb_arg, buffer, resulttok); - db_commit_transaction(r->db); - tal_free(r); + db_commit_transaction(db); + if (!was_plugin_destroyed(pd)) + tal_free(r); } void plugin_hook_call_(struct lightningd *ld, const struct plugin_hook *hook, diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 4da0e4baa..ba0634d64 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -761,7 +761,6 @@ def test_sendpay_notifications(node_factory, bitcoind): assert results['sendpay_failure'][0] == err.value.error -@pytest.mark.xfail(strict=True) def test_rpc_command_hook(node_factory): """Test the `sensitive_command` hook""" plugin = os.path.join(os.getcwd(), "tests/plugins/rpc_command.py")