From 4ea0fa645750813c4e672a4bdc0c546ca090a4ab Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 25 Jul 2021 20:06:00 +0930 Subject: [PATCH] dualopend: remove runtime memleak in favor of at-termination. dualopend doesn't always listen to lightningd messages, so it would sometimes hang at the end of tests. Signed-off-by: Rusty Russell --- lightningd/dual_open_control.c | 2 -- lightningd/opening_common.c | 15 ++++------- openingd/dualopend.c | 35 +++++++++++-------------- openingd/dualopend_wire.csv | 6 ----- openingd/dualopend_wiregen.c | 47 +--------------------------------- openingd/dualopend_wiregen.h | 14 +--------- 6 files changed, 22 insertions(+), 97 deletions(-) diff --git a/lightningd/dual_open_control.c b/lightningd/dual_open_control.c index 20c861604..45d14c0a1 100644 --- a/lightningd/dual_open_control.c +++ b/lightningd/dual_open_control.c @@ -3000,7 +3000,6 @@ static unsigned int dual_opend_msg(struct subd *dualopend, case WIRE_DUALOPEND_FAIL_FALLEN_BEHIND: channel_fail_fallen_behind(dualopend, msg); return 0; - case WIRE_DUALOPEND_DEV_MEMLEAK_REPLY: /* Messages we send */ case WIRE_DUALOPEND_INIT: @@ -3016,7 +3015,6 @@ static unsigned int dual_opend_msg(struct subd *dualopend, case WIRE_DUALOPEND_SEND_TX_SIGS: case WIRE_DUALOPEND_SEND_SHUTDOWN: case WIRE_DUALOPEND_DEPTH_REACHED: - case WIRE_DUALOPEND_DEV_MEMLEAK: break; } diff --git a/lightningd/opening_common.c b/lightningd/opening_common.c index 1882cdd06..449efd4b5 100644 --- a/lightningd/opening_common.c +++ b/lightningd/opening_common.c @@ -213,15 +213,10 @@ static void opening_memleak_req_done(struct subd *open_daemon, bool found_leak; struct peer *p; - if (streq(open_daemon->name, "dualopend")) - p = ((struct channel *)open_daemon->channel)->peer; - else - p = ((struct uncommitted_channel *)open_daemon->channel)->peer; + p = ((struct uncommitted_channel *)open_daemon->channel)->peer; tal_del_destructor2(open_daemon, opening_died_forget_memleak, cmd); - if (!fromwire_openingd_dev_memleak_reply(msg, &found_leak) && - !fromwire_dualopend_dev_memleak_reply(msg, - &found_leak)) { + if (!fromwire_openingd_dev_memleak_reply(msg, &found_leak)) { was_pending(command_fail(cmd, LIGHTNINGD, "Bad opening_dev_memleak")); return; @@ -263,11 +258,11 @@ static void opening_memleak_req_next(struct command *cmd, struct peer *prev) if (!open_daemon) continue; + /* FIXME: dualopend doesn't support memleak when we ask */ if (streq(open_daemon->name, "dualopend")) - msg = towire_dualopend_dev_memleak(NULL); - else - msg = towire_openingd_dev_memleak(NULL); + continue; + msg = towire_openingd_dev_memleak(NULL); subd_req(p, open_daemon, take(msg), -1, 0, opening_memleak_req_done, cmd); /* Just in case it dies before replying! */ diff --git a/openingd/dualopend.c b/openingd/dualopend.c index 24995a7b1..32c96ae8b 100644 --- a/openingd/dualopend.c +++ b/openingd/dualopend.c @@ -881,25 +881,24 @@ static bool is_segwit_output(struct wally_tx_output *output, /* Memory leak detection is DEVELOPER-only because we go to great lengths to * record the backtrace when allocations occur: without that, the leak * detection tends to be useless for diagnosing where the leak came from, but - * it has significant overhead. */ + * it has significant overhead. + * + * FIXME: dualopend doesn't always listen to lightningd, so we call this + * at closing time, rather than when it askes. + */ #if DEVELOPER -static void handle_dev_memleak(struct state *state, const u8 *msg) +static void dualopend_dev_memleak(struct state *state) { struct htable *memtable; - bool found_leak; - /* Populate a hash table with all our allocations (except msg, which - * is in use right now). */ - memtable = memleak_find_allocations(tmpctx, msg, msg); + /* Populate a hash table with all our allocations. */ + memtable = memleak_find_allocations(tmpctx, NULL, NULL); /* Now delete state and things it has pointers to. */ memleak_remove_region(memtable, state, tal_bytelen(state)); /* If there's anything left, dump it to logs, and return true. */ - found_leak = dump_memleak(memtable); - wire_sync_write(REQ_FD, - take(towire_dualopend_dev_memleak_reply(NULL, - found_leak))); + dump_memleak(memtable); } #endif /* DEVELOPER */ @@ -1077,10 +1076,6 @@ fetch_psbt_changes(struct state *state, open_err_warn(state, "%s", err); } else if (fromwire_dualopend_psbt_updated(state, msg, &updated_psbt)) { return updated_psbt; -#if DEVELOPER - } else if (fromwire_dualopend_dev_memleak(msg)) { - handle_dev_memleak(state, msg); -#endif /* DEVELOPER */ } else master_badmsg(fromwire_peektype(msg), msg); @@ -3591,11 +3586,6 @@ static u8 *handle_master_in(struct state *state) enum dualopend_wire t = fromwire_peektype(msg); switch (t) { - case WIRE_DUALOPEND_DEV_MEMLEAK: -#if DEVELOPER - handle_dev_memleak(state, msg); -#endif - return NULL; case WIRE_DUALOPEND_OPENER_INIT: opener_start(state, msg); return NULL; @@ -3617,7 +3607,6 @@ static u8 *handle_master_in(struct state *state) /* Handled inline */ case WIRE_DUALOPEND_INIT: case WIRE_DUALOPEND_REINIT: - case WIRE_DUALOPEND_DEV_MEMLEAK_REPLY: case WIRE_DUALOPEND_PSBT_UPDATED: case WIRE_DUALOPEND_GOT_OFFER_REPLY: case WIRE_DUALOPEND_GOT_RBF_OFFER_REPLY: @@ -3966,6 +3955,12 @@ int main(int argc, char *argv[]) per_peer_state_fdpass_send(REQ_FD, state->pps); status_debug("Sent %s with fds", dualopend_wire_name(fromwire_peektype(msg))); + tal_free(msg); + +#if DEVELOPER + /* Now look for memory leaks. */ + dualopend_dev_memleak(state); +#endif /* DEVELOPER */ /* This frees the entire tal tree. */ tal_free(state); diff --git a/openingd/dualopend_wire.csv b/openingd/dualopend_wire.csv index 8dffaef93..35f6274fc 100644 --- a/openingd/dualopend_wire.csv +++ b/openingd/dualopend_wire.csv @@ -223,12 +223,6 @@ msgtype,dualopend_fail_fallen_behind,1028 msgtype,dualopend_shutdown_complete,7025 msgdata,dualopend_shutdown_complete,per_peer_state,per_peer_state, -# master -> dualopend: do you have a memleak? -msgtype,dualopend_dev_memleak,7033 - -msgtype,dualopend_dev_memleak_reply,7133 -msgdata,dualopend_dev_memleak_reply,leak,bool, - # dualopend -> master: this was a dry run, here's some info about this open msgtype,dualopend_dry_run,7026 msgdata,dualopend_dry_run,channel_id,channel_id, diff --git a/openingd/dualopend_wiregen.c b/openingd/dualopend_wiregen.c index c37c75869..89457320f 100644 --- a/openingd/dualopend_wiregen.c +++ b/openingd/dualopend_wiregen.c @@ -44,8 +44,6 @@ const char *dualopend_wire_name(int e) case WIRE_DUALOPEND_GOT_SHUTDOWN: return "WIRE_DUALOPEND_GOT_SHUTDOWN"; case WIRE_DUALOPEND_FAIL_FALLEN_BEHIND: return "WIRE_DUALOPEND_FAIL_FALLEN_BEHIND"; case WIRE_DUALOPEND_SHUTDOWN_COMPLETE: return "WIRE_DUALOPEND_SHUTDOWN_COMPLETE"; - case WIRE_DUALOPEND_DEV_MEMLEAK: return "WIRE_DUALOPEND_DEV_MEMLEAK"; - case WIRE_DUALOPEND_DEV_MEMLEAK_REPLY: return "WIRE_DUALOPEND_DEV_MEMLEAK_REPLY"; case WIRE_DUALOPEND_DRY_RUN: return "WIRE_DUALOPEND_DRY_RUN"; case WIRE_DUALOPEND_VALIDATE_LEASE: return "WIRE_DUALOPEND_VALIDATE_LEASE"; case WIRE_DUALOPEND_VALIDATE_LEASE_REPLY: return "WIRE_DUALOPEND_VALIDATE_LEASE_REPLY"; @@ -82,8 +80,6 @@ bool dualopend_wire_is_defined(u16 type) case WIRE_DUALOPEND_GOT_SHUTDOWN:; case WIRE_DUALOPEND_FAIL_FALLEN_BEHIND:; case WIRE_DUALOPEND_SHUTDOWN_COMPLETE:; - case WIRE_DUALOPEND_DEV_MEMLEAK:; - case WIRE_DUALOPEND_DEV_MEMLEAK_REPLY:; case WIRE_DUALOPEND_DRY_RUN:; case WIRE_DUALOPEND_VALIDATE_LEASE:; case WIRE_DUALOPEND_VALIDATE_LEASE_REPLY:; @@ -956,47 +952,6 @@ bool fromwire_dualopend_shutdown_complete(const tal_t *ctx, const void *p, struc return cursor != NULL; } -/* WIRE: DUALOPEND_DEV_MEMLEAK */ -/* master -> dualopend: do you have a memleak? */ -u8 *towire_dualopend_dev_memleak(const tal_t *ctx) -{ - u8 *p = tal_arr(ctx, u8, 0); - - towire_u16(&p, WIRE_DUALOPEND_DEV_MEMLEAK); - - return memcheck(p, tal_count(p)); -} -bool fromwire_dualopend_dev_memleak(const void *p) -{ - const u8 *cursor = p; - size_t plen = tal_count(p); - - if (fromwire_u16(&cursor, &plen) != WIRE_DUALOPEND_DEV_MEMLEAK) - return false; - return cursor != NULL; -} - -/* WIRE: DUALOPEND_DEV_MEMLEAK_REPLY */ -u8 *towire_dualopend_dev_memleak_reply(const tal_t *ctx, bool leak) -{ - u8 *p = tal_arr(ctx, u8, 0); - - towire_u16(&p, WIRE_DUALOPEND_DEV_MEMLEAK_REPLY); - towire_bool(&p, leak); - - return memcheck(p, tal_count(p)); -} -bool fromwire_dualopend_dev_memleak_reply(const void *p, bool *leak) -{ - const u8 *cursor = p; - size_t plen = tal_count(p); - - if (fromwire_u16(&cursor, &plen) != WIRE_DUALOPEND_DEV_MEMLEAK_REPLY) - return false; - *leak = fromwire_bool(&cursor, &plen); - return cursor != NULL; -} - /* WIRE: DUALOPEND_DRY_RUN */ /* dualopend -> master: this was a dry run */ u8 *towire_dualopend_dry_run(const tal_t *ctx, const struct channel_id *channel_id, struct amount_sat our_funding, struct amount_sat their_funding, const struct lease_rates *lease_rates) @@ -1096,4 +1051,4 @@ bool fromwire_dualopend_validate_lease_reply(const tal_t *ctx, const void *p, wi } return cursor != NULL; } -// SHA256STAMP:323fc0085091d47b8f1f66ee5455fd229fdb4a29fc43711ac81cc5fe9eb9b696 +// SHA256STAMP:bedb1217727e81cbd377567f3db8348b524bd79ccc7030f338c800e84c47b368 diff --git a/openingd/dualopend_wiregen.h b/openingd/dualopend_wiregen.h index 0c40a3565..c8485108b 100644 --- a/openingd/dualopend_wiregen.h +++ b/openingd/dualopend_wiregen.h @@ -70,9 +70,6 @@ enum dualopend_wire { WIRE_DUALOPEND_FAIL_FALLEN_BEHIND = 1028, /* Shutdown is complete */ WIRE_DUALOPEND_SHUTDOWN_COMPLETE = 7025, - /* master -> dualopend: do you have a memleak? */ - WIRE_DUALOPEND_DEV_MEMLEAK = 7033, - WIRE_DUALOPEND_DEV_MEMLEAK_REPLY = 7133, /* dualopend -> master: this was a dry run */ WIRE_DUALOPEND_DRY_RUN = 7026, /* dualopend -> master: validate liqudity offer sig */ @@ -212,15 +209,6 @@ bool fromwire_dualopend_fail_fallen_behind(const void *p); u8 *towire_dualopend_shutdown_complete(const tal_t *ctx, const struct per_peer_state *per_peer_state); bool fromwire_dualopend_shutdown_complete(const tal_t *ctx, const void *p, struct per_peer_state **per_peer_state); -/* WIRE: DUALOPEND_DEV_MEMLEAK */ -/* master -> dualopend: do you have a memleak? */ -u8 *towire_dualopend_dev_memleak(const tal_t *ctx); -bool fromwire_dualopend_dev_memleak(const void *p); - -/* WIRE: DUALOPEND_DEV_MEMLEAK_REPLY */ -u8 *towire_dualopend_dev_memleak_reply(const tal_t *ctx, bool leak); -bool fromwire_dualopend_dev_memleak_reply(const void *p, bool *leak); - /* WIRE: DUALOPEND_DRY_RUN */ /* dualopend -> master: this was a dry run */ u8 *towire_dualopend_dry_run(const tal_t *ctx, const struct channel_id *channel_id, struct amount_sat our_funding, struct amount_sat their_funding, const struct lease_rates *lease_rates); @@ -237,4 +225,4 @@ bool fromwire_dualopend_validate_lease_reply(const tal_t *ctx, const void *p, wi #endif /* LIGHTNING_OPENINGD_DUALOPEND_WIREGEN_H */ -// SHA256STAMP:323fc0085091d47b8f1f66ee5455fd229fdb4a29fc43711ac81cc5fe9eb9b696 +// SHA256STAMP:bedb1217727e81cbd377567f3db8348b524bd79ccc7030f338c800e84c47b368