lightningd: handle properly if our own request to plugin is freed.

We should really unify the cases of a local request, vs a forwarded
request, but for now, don't steal the request onto the plugin, and
if we return from the plugin and the request is gone, don't get upset.

This uncovered a case where we weren't inside a transaction, in
test_hook_crash, so fix that.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This commit is contained in:
Rusty Russell 2023-10-24 12:11:30 +10:30
parent db7f59eeba
commit 7c1d07a94f
2 changed files with 22 additions and 35 deletions

View file

@ -525,11 +525,7 @@ static const char *plugin_notify_handle(struct plugin *plugin,
json_tok_full(plugin->buffer, idtok),
json_tok_full_len(idtok));
if (!request) {
return tal_fmt(
plugin,
"Received a JSON-RPC notify for non-existent request '%.*s'",
json_tok_full_len(idtok),
json_tok_full(plugin->buffer, idtok));
return NULL;
}
/* Ignore if they don't have a callback */
@ -633,41 +629,27 @@ static bool was_plugin_destroyed(struct plugin_destroyed *pd)
return true;
}
/* Returns the error string, or NULL */
static const char *plugin_response_handle(struct plugin *plugin,
const jsmntok_t *toks,
const jsmntok_t *idtok)
WARN_UNUSED_RESULT;
static const char *plugin_response_handle(struct plugin *plugin,
const jsmntok_t *toks,
const jsmntok_t *idtok)
static void plugin_response_handle(struct plugin *plugin,
const jsmntok_t *toks,
const jsmntok_t *idtok)
{
struct plugin_destroyed *pd;
struct jsonrpc_request *request;
const tal_t *ctx;
request = strmap_getn(&plugin->pending_requests,
json_tok_full(plugin->buffer, idtok),
json_tok_full_len(idtok));
/* Can happen if request was freed before plugin responded */
if (!request) {
return tal_fmt(
plugin,
"Received a JSON-RPC response for non-existent request '%.*s'",
json_tok_full_len(idtok),
json_tok_full(plugin->buffer, idtok));
return;
}
/* We expect the request->cb to copy if needed */
pd = plugin_detect_destruction(plugin);
/* Request callback often frees request: if not, we do. */
ctx = tal(NULL, char);
tal_steal(ctx, request);
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);
return NULL;
tal_free(ctx);
}
/**
@ -778,7 +760,8 @@ static const char *plugin_read_json_one(struct plugin *plugin,
*
* https://www.jsonrpc.org/specification#response_object
*/
err = plugin_response_handle(plugin, plugin->toks, idtok);
plugin_response_handle(plugin, plugin->toks, idtok);
err = NULL;
}
if (want_transaction)
db_commit_transaction(wallet->db);
@ -2380,13 +2363,13 @@ static void destroy_request(struct jsonrpc_request *req,
}
void plugin_request_send(struct plugin *plugin,
struct jsonrpc_request *req TAKES)
struct jsonrpc_request *req)
{
/* Add to map so we can find it later when routing the response */
tal_steal(plugin, req);
strmap_add(&plugin->pending_requests, req->id, req);
/* Add destructor in case plugin dies. */
/* Add destructor in case request is freed. */
tal_add_destructor2(req, destroy_request, plugin);
plugin_send(plugin, req->stream);
/* plugin_send steals the stream, so remove the dangling
* pointer here */

View file

@ -73,7 +73,7 @@ struct plugin {
/* An array of subscribed topics */
char **subscriptions;
/* Currently pending requests by their request ID */
/* Our pending requests by their request ID */
STRMAP(struct jsonrpc_request *) pending_requests;
/* If set, the plugin is so important that if it terminates early,
@ -322,9 +322,13 @@ void plugins_notify(struct plugins *plugins,
/**
* Send a jsonrpc_request to the specified plugin
* @plugin: the plugin to send the request to
* @req: the request.
*
* If @req is freed, any response from the plugin is ignored.
*/
void plugin_request_send(struct plugin *plugin,
struct jsonrpc_request *req TAKES);
struct jsonrpc_request *req);
/**
* Callback called when parsing options. It just stores the value in