Skip to content

Commit

Permalink
plugins/libplugin.c: Allow freeing notification struct command *.
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
ZmnSCPxj authored and rustyrussell committed Oct 8, 2021
1 parent e393791 commit d469035
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 20 deletions.
16 changes: 10 additions & 6 deletions plugins/funder.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions plugins/libplugin.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
15 changes: 12 additions & 3 deletions plugins/libplugin.h
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -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,
Expand Down
13 changes: 9 additions & 4 deletions plugins/spender/openchannel.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -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);
}

/*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*
Expand Down
15 changes: 8 additions & 7 deletions tests/plugins/test_libplugin.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down

0 comments on commit d469035

Please sign in to comment.