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: #5282 (comment)
Fixes: #5282
Fixes: #5284
Changelog-Fixed: connectd no longer crashes when peers reconnect.

Author: whitslack

connectd/connectd.c

@@ -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
@@ -231,12 +243,6 @@ static struct io_plan *retry_peer_connected(struct io_conn *conn,
 			      true);
 }
 
-/*~ 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,
@@ -253,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). */
@@ -1989,7 +1986,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,
@@ -2136,7 +2132,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());

connectd/connectd.h

@@ -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;