lightningd: don't access peer after free if it disconnects during pee…

…r_connected hook.

We keep the node_id, not a pointer to the peer.

This also means that it might have reconnected while we were in the hook, so make
sure we ignore the result if it's in state PEER_CONNECTED.

And remove the `tal_steal(peer, hook_payload)` which doesn't do anything: the
plugin_hook call steals hook_payload anyway!

Fixes: ElementsProject#5944

lightningd/peer_control.c

@@ -1071,17 +1071,17 @@ struct peer_connected_hook_payload {
 	struct wireaddr_internal addr;
 	struct wireaddr *remote_addr;
 	bool incoming;
-	struct peer *peer;
+	/* We don't keep a pointer to peer: it might be freed! */
+	struct node_id peer_id;
 	u8 *error;
 };
 
 static void
 peer_connected_serialize(struct peer_connected_hook_payload *payload,
 			 struct json_stream *stream, struct plugin *plugin)
 {
-	const struct peer *p = payload->peer;
 	json_object_start(stream, "peer");
-	json_add_node_id(stream, "id", &p->id);
+	json_add_node_id(stream, "id", &payload->peer_id);
 	json_add_string(stream, "direction", payload->incoming ? "in" : "out");
 	json_add_string(
 	    stream, "addr",
@@ -1090,7 +1090,10 @@ peer_connected_serialize(struct peer_connected_hook_payload *payload,
 		json_add_string(
 		    stream, "remote_addr",
 		    type_to_string(stream, struct wireaddr, payload->remote_addr));
-	json_add_hex_talarr(stream, "features", p->their_features);
+	/* Since this is start of hook, peer is always in table! */
+	json_add_hex_talarr(stream, "features",
+			    peer_by_id(payload->ld, &payload->peer_id)
+			    ->their_features);
 	json_object_end(stream); /* .peer */
 }
 
@@ -1186,17 +1189,24 @@ static void peer_connected_hook_final(struct peer_connected_hook_payload *payloa
 	struct lightningd *ld = payload->ld;
 	struct channel *channel;
 	struct wireaddr_internal addr = payload->addr;
-	struct peer *peer = payload->peer;
+	struct peer *peer;
 	u8 *error;
 
 	/* Whatever happens, we free payload (it's currently a child
 	 * of the peer, which may be freed if we fail to start
 	 * subd). */
 	tal_steal(tmpctx, payload);
 
+	/* Peer might have gone away while we were waiting for plugin! */
+	peer = peer_by_id(ld, &payload->peer_id);
+	if (!peer)
+		return;
+
 	/* If we disconnected in the meantime, forget about it.
-	 * (disconnect will have failed any connect commands). */
-	if (peer->connected == PEER_DISCONNECTED)
+	 * (disconnect will have failed any connect commands).
+	 * And if it has reconnected, and we're the second time the
+	 * hook has been called, it'll be PEER_CONNECTED. */
+	if (peer->connected != PEER_CONNECTING)
 		return;
 
 	/* Check for specific errors of a hook */
@@ -1424,10 +1434,8 @@ void peer_connected(struct lightningd *ld, const u8 *msg)
 	if (peer->remote_addr)
 		tal_free(peer->remote_addr);
 	peer->remote_addr = NULL;
-	peer_update_features(peer, take(their_features));
-
-	tal_steal(peer, hook_payload);
-	hook_payload->peer = peer;
+	peer_update_features(peer, their_features);
+	hook_payload->peer_id = id;
 
 	/* If there's a connect command, use its id as basis for hook id */
 	cmd_id = connect_any_cmd_id(tmpctx, ld, peer);