hsmd: don't leak message buffers

[whitslack]

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

[cdecker]

ACK 0a30efe

[vincenzopalazzo]

ack 0a30efe

[rustyrussell]

This is great, but why didn't our memleak detection catch it?

... ah, because I broke it in 6c9b752!

Fortunately that's since last release...

[whitslack]

This is great, but why didn't our memleak detection catch it?

@rustyrussell: It's not strictly a memory leak, per se. The buffers have valid ancestry chains up to a notleak root. But that doesn't preclude them from piling up in memory.

I would refer you to this sentence of my commit message:

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…

[rustyrussell]

This is great, but why didn't our memleak detection catch it?

@rustyrussell: It's not strictly a memory leak, per se. The buffers have valid ancestry chains up to a notleak root. But that doesn't preclude them from piling up in memory.

I would refer you to this sentence of my commit message:

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…

Still should be detected by our leak detector! And, once I fixed it, it is...