Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connectd: avoid use-after-free upon multiple reconnections by a peer #5300

Merged
merged 3 commits into from
Jun 28, 2022
Merged

connectd: avoid use-after-free upon multiple reconnections by a peer #5300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
15a1d8b
connectd: avoid use-after-free upon multiple reconnections by a peer
whitslack May 30, 2022
41d67d5
lightningd: fix crash on rapid reconnect.
rustyrussell Jun 19, 2022
debd45c
connectd: don't keep around more than one old connection.
rustyrussell Jun 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 27 additions & 21 deletions connectd/connectd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -227,13 +239,8 @@ static struct io_plan *retry_peer_connected(struct io_conn *conn,
/*~ Usually the pattern is to return this directly. */
return peer_connected(conn, pr->daemon, &pr->id, &pr->addr,
pr->remote_addr,
&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);
&pr->cs, take(pr->their_features), pr->incoming,
true);
}

/*~ If we already know about this peer, we tell lightningd and it disconnects
Expand All @@ -252,27 +259,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). */
Expand Down Expand Up @@ -337,7 +335,8 @@ struct io_plan *peer_connected(struct io_conn *conn,
const struct wireaddr *remote_addr,
struct crypto_state *cs,
const u8 *their_features TAKES,
bool incoming)
bool incoming,
bool retrying)
{
u8 *msg;
struct peer *peer;
Expand All @@ -347,9 +346,18 @@ struct io_plan *peer_connected(struct io_conn *conn,
bool option_gossip_queries;

peer = peer_htable_get(&daemon->peers, id);
if (peer)
if (peer) {
/* If we were already retrying, we only get one chance: there
* can be multiple reconnections, and we must not keep around
* stale ones */
if (retrying) {
if (taken(their_features))
tal_free(their_features);
return io_close(conn);
}
return peer_reconnected(conn, daemon, id, addr, remote_addr, cs,
their_features, incoming);
}

/* We promised we'd take it by marking it TAKEN above; prepare to free it. */
if (taken(their_features))
Expand Down Expand Up @@ -1976,7 +1984,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,
Expand Down Expand Up @@ -2123,7 +2130,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());
Expand Down
37 changes: 2 additions & 35 deletions connectd/connectd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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? */
Expand All @@ -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;

Expand Down Expand Up @@ -246,7 +212,8 @@ struct io_plan *peer_connected(struct io_conn *conn,
const struct wireaddr *remote_addr,
struct crypto_state *cs,
const u8 *their_features TAKES,
bool incoming);
bool incoming,
bool retrying);

/* Called when peer->peer_conn is finally freed */
void peer_conn_closed(struct peer *peer);
Expand Down
3 changes: 2 additions & 1 deletion connectd/peer_exchange_initmsg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ static struct io_plan *peer_init_received(struct io_conn *conn,
remote_addr,
&peer->cs,
take(features),
peer->incoming);
peer->incoming,
false);
}

static struct io_plan *peer_init_hdr_received(struct io_conn *conn,
Expand Down
3 changes: 2 additions & 1 deletion lightningd/peer_control.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1249,7 +1249,8 @@ void peer_active(struct lightningd *ld, const u8 *msg, int fd)
case CHANNELD_NORMAL:
case CHANNELD_SHUTTING_DOWN:
case CLOSINGD_SIGEXCHANGE:
assert(!channel->owner);
/* Maybe old owner was too slow exiting? */
tal_free(channel->owner);
peer_start_channeld(channel,
peer_fd,
NULL, true,
Expand Down