mirror of
https://github.com/ElementsProject/lightning.git
synced 2025-03-15 20:09:18 +01:00
connectd: avoid use-after-free upon multiple reconnections by a peer
`peer_reconnected` was freeing a `struct peer_reconnected` instance while a pointer to that instance was registered to be passed as an argument to the `retry_peer_connected` callback function. This caused a use-after-free crash when `retry_peer_connected` attempted to reparent the instance to the temporary context. Instead, never have `peer_reconnected` free a `struct peer_reconnected` instance, and only ever allow such an instance to be freed after the `retry_peer_connected` callback has finished with it. To ensure that the instance is freed even if the connection is closed before the callback can be invoked, parent the instance to the connection rather than to the daemon. Absent the need to free `struct peer_reconnected` instances outside of the `retry_peer_connected` callback, there is no use for the `reconnected` hashtable, so remove it as well. See: https://github.com/ElementsProject/lightning/issues/5282#issuecomment-1141454255 Fixes: #5282 Fixes: #5284 Changelog-Fixed: connectd no longer crashes when peers reconnect.
This commit is contained in:
parent
4ee55acc71
commit
83c825945c
2 changed files with 13 additions and 52 deletions
|
@ -93,6 +93,18 @@ struct connecting {
|
|||
u32 seconds_waited;
|
||||
};
|
||||
|
||||
/*~ This is an ad-hoc marshalling structure where we store arguments so we
|
||||
* can call peer_connected again. */
|
||||
struct peer_reconnected {
|
||||
struct daemon *daemon;
|
||||
struct node_id id;
|
||||
struct wireaddr_internal addr;
|
||||
const struct wireaddr *remote_addr;
|
||||
struct crypto_state cs;
|
||||
const u8 *their_features;
|
||||
bool incoming;
|
||||
};
|
||||
|
||||
/*~ C programs should generally be written bottom-to-top, with the root
|
||||
* function at the bottom, and functions it calls above it. That avoids
|
||||
* us having to pre-declare functions; but in the case of mutual recursion
|
||||
|
@ -230,12 +242,6 @@ static struct io_plan *retry_peer_connected(struct io_conn *conn,
|
|||
&pr->cs, take(pr->their_features), pr->incoming);
|
||||
}
|
||||
|
||||
/*~ A common use for destructors is to remove themselves from a data structure */
|
||||
static void destroy_peer_reconnected(struct peer_reconnected *pr)
|
||||
{
|
||||
peer_reconnected_htable_del(&pr->daemon->reconnected, pr);
|
||||
}
|
||||
|
||||
/*~ If we already know about this peer, we tell lightningd and it disconnects
|
||||
* the old one. We wait until it tells us that's happened. */
|
||||
static struct io_plan *peer_reconnected(struct io_conn *conn,
|
||||
|
@ -252,27 +258,18 @@ static struct io_plan *peer_reconnected(struct io_conn *conn,
|
|||
|
||||
status_peer_debug(id, "reconnect");
|
||||
|
||||
/* If we have a previous reconnection, we replace it. */
|
||||
pr = peer_reconnected_htable_get(&daemon->reconnected, id);
|
||||
if (pr) {
|
||||
peer_reconnected_htable_del(&daemon->reconnected, pr);
|
||||
tal_free(pr);
|
||||
}
|
||||
|
||||
/* Tell master to kill it: will send peer_disconnect */
|
||||
msg = towire_connectd_reconnected(NULL, id);
|
||||
daemon_conn_send(daemon->master, take(msg));
|
||||
|
||||
/* Save arguments for next time. */
|
||||
pr = tal(daemon, struct peer_reconnected);
|
||||
pr = tal(conn, struct peer_reconnected);
|
||||
pr->daemon = daemon;
|
||||
pr->id = *id;
|
||||
pr->cs = *cs;
|
||||
pr->addr = *addr;
|
||||
pr->remote_addr = tal_dup_or_null(pr, struct wireaddr, remote_addr);
|
||||
pr->incoming = incoming;
|
||||
peer_reconnected_htable_add(&daemon->reconnected, pr);
|
||||
tal_add_destructor(pr, destroy_peer_reconnected);
|
||||
|
||||
/*~ Note that tal_dup_talarr() will do handle the take() of features
|
||||
* (turning it into a simply tal_steal() in those cases). */
|
||||
|
@ -2000,7 +1997,6 @@ static void dev_connect_memleak(struct daemon *daemon, const u8 *msg)
|
|||
/* Now delete daemon and those which it has pointers to. */
|
||||
memleak_remove_region(memtable, daemon, sizeof(daemon));
|
||||
memleak_remove_htable(memtable, &daemon->peers.raw);
|
||||
memleak_remove_htable(memtable, &daemon->reconnected.raw);
|
||||
|
||||
found_leak = dump_memleak(memtable, memleak_status_broken);
|
||||
daemon_conn_send(daemon->master,
|
||||
|
@ -2147,7 +2143,6 @@ int main(int argc, char *argv[])
|
|||
/* Allocate and set up our simple top-level structure. */
|
||||
daemon = tal(NULL, struct daemon);
|
||||
peer_htable_init(&daemon->peers);
|
||||
peer_reconnected_htable_init(&daemon->reconnected);
|
||||
memleak_add_helper(daemon, memleak_daemon_cb);
|
||||
list_head_init(&daemon->connecting);
|
||||
timers_init(&daemon->timers, time_mono());
|
||||
|
|
|
@ -126,37 +126,6 @@ HTABLE_DEFINE_TYPE(struct peer,
|
|||
peer_eq_node_id,
|
||||
peer_htable);
|
||||
|
||||
/*~ This is an ad-hoc marshalling structure where we store arguments so we
|
||||
* can call peer_connected again. */
|
||||
struct peer_reconnected {
|
||||
struct daemon *daemon;
|
||||
struct node_id id;
|
||||
struct wireaddr_internal addr;
|
||||
const struct wireaddr *remote_addr;
|
||||
struct crypto_state cs;
|
||||
const u8 *their_features;
|
||||
bool incoming;
|
||||
};
|
||||
|
||||
static const struct node_id *
|
||||
peer_reconnected_keyof(const struct peer_reconnected *pr)
|
||||
{
|
||||
return &pr->id;
|
||||
}
|
||||
|
||||
static bool peer_reconnected_eq_node_id(const struct peer_reconnected *pr,
|
||||
const struct node_id *id)
|
||||
{
|
||||
return node_id_eq(&pr->id, id);
|
||||
}
|
||||
|
||||
/*~ This defines 'struct peer_reconnected_htable'. */
|
||||
HTABLE_DEFINE_TYPE(struct peer_reconnected,
|
||||
peer_reconnected_keyof,
|
||||
node_id_hash,
|
||||
peer_reconnected_eq_node_id,
|
||||
peer_reconnected_htable);
|
||||
|
||||
/*~ This is the global state, like `struct lightningd *ld` in lightningd. */
|
||||
struct daemon {
|
||||
/* Who am I? */
|
||||
|
@ -173,9 +142,6 @@ struct daemon {
|
|||
* have disconnected. */
|
||||
struct peer_htable peers;
|
||||
|
||||
/* Peers which have reconnected, waiting for us to kill existing conns */
|
||||
struct peer_reconnected_htable reconnected;
|
||||
|
||||
/* Peers we are trying to reach */
|
||||
struct list_head connecting;
|
||||
|
||||
|
|
Loading…
Add table
Reference in a new issue