From d4690358d9229f7b0b3e77b0fb95d21953c0aa4d Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Fri, 1 Oct 2021 07:59:40 +0800 Subject: [PATCH] plugins/libplugin.c: Allow freeing notification `struct command *`. We always allocate a new `struct command` when we get a full JSON object from stdin: https://github.com/ElementsProject/lightning/blob/b2df01dc73ea7a51ae2a495281bf4d775eafa0a4/plugins/libplugin.c#L1229-L1233 If it happens to be a notification, we pass the `struct command` to the handler, and not free it ourselves: https://github.com/ElementsProject/lightning/blob/b2df01dc73ea7a51ae2a495281bf4d775eafa0a4/plugins/libplugin.c#L1270-L1275 There are only nine points in `plugins/libplugin.c` where we `tal_free` anything, and only one of them frees a `struct command`: https://github.com/ElementsProject/lightning/blob/b2df01dc73ea7a51ae2a495281bf4d775eafa0a4/plugins/libplugin.c#L224-L234 The above function `command_complete` is not appropriate for notification handlers; the above function sends out a response to our stdout, which a notification handler should not do. However, as-is, it does mean that notification handling leaks `struct command` objects, which can be problematic if we ever have future built-in plugins which are significantly more dependent on notifications. This commit changes notification handlers to return `struct command_result *`, because possibly in the future notification handlers may want to perform `send_outreq`, so we might as well use our standard convention for callbacks, and to encourage future developers to check how to properly terminate notification handlers (and free up the `struct command`). We also now provide a `notification_handled` function which a notification handler must eventually call, as well as a `notification_handler_pending` which is just a snowclone of `command_still_pending`. --- plugins/funder.c | 16 ++++++++++------ plugins/libplugin.c | 7 +++++++ plugins/libplugin.h | 15 ++++++++++++--- plugins/spender/openchannel.c | 13 +++++++++---- tests/plugins/test_libplugin.c | 15 ++++++++------- 5 files changed, 46 insertions(+), 20 deletions(-) diff --git a/plugins/funder.c b/plugins/funder.c index 1caf3273e..d0a41d8dd 100644 --- a/plugins/funder.c +++ b/plugins/funder.c @@ -708,9 +708,9 @@ json_rbf_channel_call(struct command *cmd, return send_outreq(cmd->plugin, req); } -static void json_disconnect(struct command *cmd, - const char *buf, - const jsmntok_t *params) +static struct command_result *json_disconnect(struct command *cmd, + const char *buf, + const jsmntok_t *params) { struct node_id id; const char *err; @@ -730,11 +730,13 @@ static void json_disconnect(struct command *cmd, type_to_string(tmpctx, struct node_id, &id)); cleanup_peer_pending_opens(&id); + + return notification_handled(cmd); } -static void json_channel_open_failed(struct command *cmd, - const char *buf, - const jsmntok_t *params) +static struct command_result *json_channel_open_failed(struct command *cmd, + const char *buf, + const jsmntok_t *params) { struct channel_id cid; struct pending_open *open; @@ -758,6 +760,8 @@ static void json_channel_open_failed(struct command *cmd, open = cleanup_channel_pending_open(&cid); if (open) unreserve_psbt(open); + + return notification_handled(cmd); } static void json_add_policy(struct json_stream *stream, diff --git a/plugins/libplugin.c b/plugins/libplugin.c index 71ac8159b..d24c85147 100644 --- a/plugins/libplugin.c +++ b/plugins/libplugin.c @@ -1759,3 +1759,10 @@ command_hook_success(struct command *cmd) json_add_string(response, "result", "continue"); return command_finished(cmd, response); } + +struct command_result *WARN_UNUSED_RESULT +notification_handled(struct command *cmd) +{ + tal_free(cmd); + return &complete; +} diff --git a/plugins/libplugin.h b/plugins/libplugin.h index cf1979c89..aba277032 100644 --- a/plugins/libplugin.h +++ b/plugins/libplugin.h @@ -84,9 +84,11 @@ struct plugin_option { /* Create an array of these, one for each notification you subscribe to. */ struct plugin_notification { const char *name; - void (*handle)(struct command *cmd, - const char *buf, - const jsmntok_t *params); + /* The handler must eventually trigger a `notification_handled` + * call. */ + struct command_result* (*handle)(struct command *cmd, + const char *buf, + const jsmntok_t *params); }; /* Create an array of these, one for each hook you subscribe to. */ @@ -190,6 +192,13 @@ command_success(struct command *cmd, const struct json_out *result); struct command_result *WARN_UNUSED_RESULT command_hook_success(struct command *cmd); +/* End a notification handler. */ +struct command_result *WARN_UNUSED_RESULT +notification_handled(struct command *cmd); + +/* Helper for notification handler that will be finished in a callback. */ +#define notification_handler_pending(cmd) command_still_pending(cmd) + /* Synchronous helper to send command and extract fields from * response; can only be used in init callback. */ void rpc_scan(struct plugin *plugin, diff --git a/plugins/spender/openchannel.c b/plugins/spender/openchannel.c index 34704d450..501f28ca7 100644 --- a/plugins/spender/openchannel.c +++ b/plugins/spender/openchannel.c @@ -524,9 +524,9 @@ check_sigs_ready(struct multifundchannel_command *mfc) return command_still_pending(mfc->cmd); } -static void json_peer_sigs(struct command *cmd, - const char *buf, - const jsmntok_t *params) +static struct command_result *json_peer_sigs(struct command *cmd, + const char *buf, + const jsmntok_t *params) { struct channel_id cid; const struct wally_psbt *psbt; @@ -552,7 +552,7 @@ static void json_peer_sigs(struct command *cmd, "mfc ??: `openchannel_peer_sigs` no " "pending dest found for channel_id %s", type_to_string(tmpctx, struct channel_id, &cid)); - return; + return notification_handled(cmd); } plugin_log(cmd->plugin, LOG_DBG, @@ -587,7 +587,12 @@ static void json_peer_sigs(struct command *cmd, dest->state = MULTIFUNDCHANNEL_SIGNED; } + /* Possibly free up the struct command for the mfc that + * we found. */ check_sigs_ready(dest->mfc); + + /* Free up the struct command for *this* call. */ + return notification_handled(cmd); } /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~* diff --git a/tests/plugins/test_libplugin.c b/tests/plugins/test_libplugin.c index 7320ad5db..49440be82 100644 --- a/tests/plugins/test_libplugin.c +++ b/tests/plugins/test_libplugin.c @@ -48,24 +48,25 @@ json_peer_connected(struct command *cmd, return command_finished(cmd, response); } -static void json_connected(struct command *cmd, - const char *buf, - const jsmntok_t *params) +static struct command_result *json_connected(struct command *cmd, + const char *buf, + const jsmntok_t *params) { const jsmntok_t *idtok = json_get_member(buf, params, "id"); assert(idtok); plugin_log(cmd->plugin, LOG_INFORM, "%s connected", json_strdup(tmpctx, buf, idtok)); + return notification_handled(cmd); } -static void json_shutdown(struct command *cmd, - const char *buf, - const jsmntok_t *params) +static struct command_result *json_shutdown(struct command *cmd, + const char *buf, + const jsmntok_t *params) { plugin_log(cmd->plugin, LOG_DBG, "shutdown called"); if (dont_shutdown) - return; + return notification_handled(cmd); plugin_exit(cmd->plugin, 0); }