plugin: handle corner case where rpc_command is to stop the plugin.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

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,8 +254,12 @@ 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);
 
+	/* 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);
 	}
 
+	/* 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;
+}

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

lightningd/plugin_hook.c

@@ -76,15 +76,20 @@ 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);
+	db_commit_transaction(db);
+	if (!was_plugin_destroyed(pd))
 		tal_free(r);
 }
 

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")