From ec6d91fd549df85a383c3ef63a8d13cf803b935c Mon Sep 17 00:00:00 2001
From: Matt Whitlock <c-lightning@mattwhitlock.name>
Date: Wed, 9 Feb 2022 04:21:53 -0500
Subject: [PATCH] hsmd: don't leak message buffers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

client_read_next(…) calls io_read_wire(…), passing &c->msg_in as the
address of a pointer that will be set to the address of the buffer that
io_read_wire_(…) will allocate, and passing c (a pointer to the struct
client instance) as the parent for the new allocation. As long as the
struct client instance eventually gets freed, the allocated message
buffer will be freed too, so there is no "leak" in the strict sense of
the term, but the freeing of the buffer may not occur for an arbitrarily
long time after the buffer has become disused, and indeed many millions
of message buffers may be allocated within the lifetime of one struct
client instance.

handle_client(…) ultimately hands off the c->msg_in to one of several
message-type-specific handler functions, and those functions are not
TAKES or STEALS on their message buffer parameters and do not free their
message buffer arguments. Consequently, each successive call to
client_read_next(…) will cause io_read_wire_(…) to overwrite the
c->msg_in pointer with the address of a newly allocated message buffer,
and the old buffer will be left dangling off of the struct client
instance indefinitely.

Fix this by initializing c->msg_in to NULL in new_client(…) and then
having client_read_next(…) do `c->msg_in = tal_free(c->msg_in)` prior to
calling io_read_wire(…). That way, the previous message buffer will be
freed just before beginning to read the next message. The same strategy
is already employed in common/daemon_conn.c, albeit without nulling out
dc->msg_in after freeing it.

Fixes: #5035
Changelog-Fixed: hsmd: Fixed a significant memory leak
---
 hsmd/hsmd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c
index a7342a2e1..a236131f1 100644
--- a/hsmd/hsmd.c
+++ b/hsmd/hsmd.c
@@ -165,6 +165,7 @@ static struct io_plan *bad_req(struct io_conn *conn,
  * and then call handle_client with argument 'c' */
 static struct io_plan *client_read_next(struct io_conn *conn, struct client *c)
 {
+	c->msg_in = tal_free(c->msg_in);
 	return io_read_wire(conn, c, &c->msg_in, handle_client, c);
 }
 
@@ -187,6 +188,8 @@ static struct client *new_client(const tal_t *ctx,
 {
 	struct client *c = tal(ctx, struct client);
 
+	c->msg_in = NULL;
+
 	/*~ All-zero pubkey is used for the initial master connection */
 	if (id) {
 		c->id = *id;