From e11b35cb3aec92d5fc5d77ac6c0937542f9a7caf Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 3 Oct 2023 12:58:55 +1030 Subject: [PATCH] common/memleak: implement callback arg for dump_memleak. This makes it easier to use outside simple subds, and now lightningd can simply dump to log rather than returning JSON. JSON formatting was a lot of work, and we only did it for lightningd, not for subdaemons. Easier to use the logs in all cases. Signed-off-by: Rusty Russell --- channeld/channeld.c | 2 +- closingd/closingd.c | 2 +- common/memleak.c | 38 +++++++++++------- common/memleak.h | 21 ++++++---- common/status.c | 2 +- common/status.h | 2 +- connectd/connectd.c | 2 +- gossipd/gossipd.c | 2 +- hsmd/hsmd.c | 2 +- lightningd/memdump.c | 58 ++++----------------------- onchaind/onchaind.c | 2 +- onchaind/test/run-grind_feerate-bug.c | 11 ++--- onchaind/test/run-grind_feerate.c | 11 ++--- openingd/dualopend.c | 2 +- openingd/openingd.c | 2 +- plugins/libplugin.c | 9 ++--- 16 files changed, 70 insertions(+), 98 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index d552e80cd..cf8eaa9e6 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -5585,7 +5585,7 @@ static void handle_dev_memleak(struct peer *peer, const u8 *msg) /* Now delete peer and things it has pointers to. */ memleak_scan_obj(memtable, peer); - found_leak = dump_memleak(memtable, memleak_status_broken); + found_leak = dump_memleak(memtable, memleak_status_broken, NULL); wire_sync_write(MASTER_FD, take(towire_channeld_dev_memleak_reply(NULL, found_leak))); diff --git a/closingd/closingd.c b/closingd/closingd.c index 10293d2e2..02e734eac 100644 --- a/closingd/closingd.c +++ b/closingd/closingd.c @@ -559,7 +559,7 @@ static void closing_dev_memleak(const tal_t *ctx, memleak_ptr(memtable, scriptpubkey[REMOTE]); memleak_ptr(memtable, funding_wscript); - dump_memleak(memtable, memleak_status_broken); + dump_memleak(memtable, memleak_status_broken, NULL); } /* Figure out what weight we actually expect for this closing tx (using zero fees diff --git a/common/memleak.c b/common/memleak.c index fa6ee54b0..c86b173d6 100644 --- a/common/memleak.c +++ b/common/memleak.c @@ -199,7 +199,7 @@ static bool ptr_match(const void *candidate, void *ptr) return candidate == ptr; } -const void *memleak_get(struct htable *memtable, const uintptr_t **backtrace) +static const void *memleak_get(struct htable *memtable, const uintptr_t **backtrace) { struct htable_iter it; const tal_t *i, *p; @@ -330,51 +330,61 @@ void memleak_init(void) } } +struct print_and_arg { + void PRINTF_FMT(2,3) (*print)(void *arg, const char *fmt, ...); + void *arg; +}; + static int dump_syminfo(void *data, uintptr_t pc UNUSED, const char *filename, int lineno, const char *function) { - void PRINTF_FMT(1,2) (*print)(const char *fmt, ...) = data; + struct print_and_arg *pag = data; /* This can happen in backtraces. */ if (!filename || !function) return 0; - print(" %s:%u (%s)", filename, lineno, function); + pag->print(pag->arg, " %s:%u (%s)", filename, lineno, function); return 0; } static void dump_leak_backtrace(const uintptr_t *bt, - void PRINTF_FMT(1,2) - (*print)(const char *fmt, ...)) + void (*print)(void *arg, const char *fmt, ...), + void *arg) { + struct print_and_arg pag; + if (!bt) return; + pag.print = print; + pag.arg = arg; /* First one serves as counter. */ - print(" backtrace:"); + print(arg, " backtrace:"); for (size_t i = 1; i < bt[0]; i++) { backtrace_pcinfo(backtrace_state, bt[i], dump_syminfo, - NULL, print); + NULL, &pag); } } -bool dump_memleak(struct htable *memtable, - void PRINTF_FMT(1,2) (*print)(const char *fmt, ...)) +bool dump_memleak_(struct htable *memtable, + void PRINTF_FMT(2,3) (*print)(void *arg, const char *fmt, ...), + void *arg) { const tal_t *i; const uintptr_t *backtrace; bool found_leak = false; while ((i = memleak_get(memtable, &backtrace)) != NULL) { - print("MEMLEAK: %p", i); + print(arg, "MEMLEAK: %p", i); if (tal_name(i)) - print(" label=%s", tal_name(i)); + print(arg, " label=%s", tal_name(i)); - dump_leak_backtrace(backtrace, print); - print(" parents:"); + dump_leak_backtrace(backtrace, print, arg); + print(arg, " parents:"); for (tal_t *p = tal_parent(i); p; p = tal_parent(p)) { - print(" %s", tal_name(p)); + print(arg, " %s", tal_name(p)); p = tal_parent(p); } found_leak = true; diff --git a/common/memleak.h b/common/memleak.h index f12c57be0..a15413b23 100644 --- a/common/memleak.h +++ b/common/memleak.h @@ -3,6 +3,7 @@ #include "config.h" #include #include +#include #include struct htable; @@ -131,18 +132,22 @@ void memleak_scan_strmap_(struct htable *memtable, const struct strmap *m); void memleak_ignore_children(struct htable *memtable, const void *p); /** - * memleak_get: get (and remove) a leak from memtable, or NULL + * dump_memleak: use print function to dump memleak details * @memtable: the memtable after all known allocations removed. - * @backtrace: the backtrace to set if there is one. + * @print: the printf-style function to use (takes @arg first) + * @arg: the arg for @print * - * If this returns NULL, it means the @memtable was empty. Otherwise - * it return a pointer to a leak (and removes it from @memtable) + * Returns true if there was a leak. */ -const void *memleak_get(struct htable *memtable, const uintptr_t **backtrace); +#define dump_memleak(memtable, print, arg) \ + dump_memleak_((memtable), \ + typesafe_cb_postargs(void, void *, (print), (arg), const char *, ...), \ + (arg)) + +bool dump_memleak_(struct htable *memtable, + void PRINTF_FMT(2,3) (*print)(void *arg, const char *fmt, ...), + void *arg); extern struct backtrace_state *backtrace_state; -bool dump_memleak(struct htable *memtable, - void PRINTF_FMT(1,2) (*print)(const char *fmt, ...)); - #endif /* LIGHTNING_COMMON_MEMLEAK_H */ diff --git a/common/status.c b/common/status.c index e17c06473..bcf700be2 100644 --- a/common/status.c +++ b/common/status.c @@ -235,7 +235,7 @@ void master_badmsg(u32 type_expected, const u8 *msg) } /* Print BROKEN status: callback for dump_memleak. */ -void memleak_status_broken(const char *fmt, ...) +void memleak_status_broken(void *unused, const char *fmt, ...) { va_list ap; va_start(ap, fmt); diff --git a/common/status.h b/common/status.h index 22458ad78..e4ca3f924 100644 --- a/common/status.h +++ b/common/status.h @@ -71,6 +71,6 @@ void status_send_fatal(const u8 *msg TAKES) NORETURN; void status_send_fd(int fd); /* Print BROKEN status: callback for dump_memleak. */ -void memleak_status_broken(const char *fmt, ...); +void memleak_status_broken(void *unused, const char *fmt, ...); #endif /* LIGHTNING_COMMON_STATUS_H */ diff --git a/connectd/connectd.c b/connectd/connectd.c index e1fe5afa4..1ee614572 100644 --- a/connectd/connectd.c +++ b/connectd/connectd.c @@ -1861,7 +1861,7 @@ static void dev_connect_memleak(struct daemon *daemon, const u8 *msg) memleak_scan_obj(memtable, daemon); memleak_scan_htable(memtable, &daemon->peers->raw); - found_leak = dump_memleak(memtable, memleak_status_broken); + found_leak = dump_memleak(memtable, memleak_status_broken, NULL); daemon_conn_send(daemon->master, take(towire_connectd_dev_memleak_reply(NULL, found_leak))); diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index 62e9bba66..e681a56b1 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -966,7 +966,7 @@ static void dev_gossip_memleak(struct daemon *daemon, const u8 *msg) memleak_scan_obj(memtable, daemon); memleak_scan_htable(memtable, &daemon->peers->raw); - found_leak = dump_memleak(memtable, memleak_status_broken); + found_leak = dump_memleak(memtable, memleak_status_broken, NULL); daemon_conn_send(daemon->master, take(towire_gossipd_dev_memleak_reply(NULL, found_leak))); diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index b2b4be0be..72d74bc47 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -573,7 +573,7 @@ static struct io_plan *handle_memleak(struct io_conn *conn, memleak_ptr(memtable, dev_force_privkey); memleak_ptr(memtable, dev_force_bip32_seed); - found_leak = dump_memleak(memtable, memleak_status_broken); + found_leak = dump_memleak(memtable, memleak_status_broken, NULL); reply = towire_hsmd_dev_memleak_reply(NULL, found_leak); return req_reply(conn, c, take(reply)); } diff --git a/lightningd/memdump.c b/lightningd/memdump.c index fd856e1aa..ad8733611 100644 --- a/lightningd/memdump.c +++ b/lightningd/memdump.c @@ -88,46 +88,17 @@ static const struct json_command dev_memdump_command = { }; AUTODATA(json_command, &dev_memdump_command); -static int json_add_syminfo(void *data, uintptr_t pc UNUSED, - const char *filename, int lineno, - const char *function) +static void memleak_log(struct logger *log, const char *fmt, ...) { - struct json_stream *response = data; - char *str; - - /* This can happen in backtraces. */ - if (!filename || !function) - return 0; - - str = tal_fmt(response, "%s:%u (%s)", filename, lineno, function); - json_add_string(response, NULL, str); - tal_free(str); - return 0; -} - -static void json_add_backtrace(struct json_stream *response, - const uintptr_t *bt) -{ - size_t i; - - if (!bt) - return; - - json_array_start(response, "backtrace"); - /* First one serves as counter. */ - for (i = 1; i < bt[0]; i++) { - backtrace_pcinfo(backtrace_state, - bt[i], json_add_syminfo, - NULL, response); - } - json_array_end(response); + va_list ap; + va_start(ap, fmt); + logv(log, LOG_BROKEN, NULL, true, fmt, ap); + va_end(ap); } static void finish_report(const struct leak_detect *leaks) { struct htable *memtable; - const tal_t *i; - const uintptr_t *backtrace; struct command *cmd; struct lightningd *ld; struct json_stream *response; @@ -162,24 +133,11 @@ static void finish_report(const struct leak_detect *leaks) /* Now delete ld and those which it has pointers to. */ memleak_scan_obj(memtable, ld); + if (dump_memleak(memtable, memleak_log, ld->log)) + tal_arr_expand(&leaks->leakers, "lightningd"); + response = json_stream_success(cmd); json_array_start(response, "leaks"); - while ((i = memleak_get(memtable, &backtrace)) != NULL) { - const tal_t *p; - - json_object_start(response, NULL); - json_add_ptr(response, "value", i); - if (tal_name(i)) - json_add_string(response, "label", tal_name(i)); - - json_add_backtrace(response, backtrace); - json_array_start(response, "parents"); - for (p = tal_parent(i); p; p = tal_parent(p)) - json_add_string(response, NULL, tal_name(p)); - json_array_end(response); - json_object_end(response); - } - for (size_t num_leakers = 0; num_leakers < tal_count(leaks->leakers); num_leakers++) { diff --git a/onchaind/onchaind.c b/onchaind/onchaind.c index da7ae3dbe..a41578810 100644 --- a/onchaind/onchaind.c +++ b/onchaind/onchaind.c @@ -1606,7 +1606,7 @@ static void handle_dev_memleak(struct tracked_output ***outs, const u8 *msg) memleak_remove_globals(memtable, tal_parent(*outs)); memleak_scan_obj(memtable, *outs); - found_leak = dump_memleak(memtable, memleak_status_broken); + found_leak = dump_memleak(memtable, memleak_status_broken, NULL); wire_sync_write(REQ_FD, take(towire_onchaind_dev_memleak_reply(NULL, found_leak))); diff --git a/onchaind/test/run-grind_feerate-bug.c b/onchaind/test/run-grind_feerate-bug.c index 8907b7dbe..227210b01 100644 --- a/onchaind/test/run-grind_feerate-bug.c +++ b/onchaind/test/run-grind_feerate-bug.c @@ -30,10 +30,11 @@ bool derive_keyset(const struct pubkey *per_commitment_point UNNEEDED, bool option_static_remotekey UNNEEDED, struct keyset *keyset UNNEEDED) { fprintf(stderr, "derive_keyset called!\n"); abort(); } -/* Generated stub for dump_memleak */ -bool dump_memleak(struct htable *memtable UNNEEDED, - void (*print)(const char *fmt UNNEEDED, ...)) -{ fprintf(stderr, "dump_memleak called!\n"); abort(); } +/* Generated stub for dump_memleak_ */ +bool dump_memleak_(struct htable *memtable UNNEEDED, + void (*print)(void *arg UNNEEDED, const char *fmt UNNEEDED, ...) UNNEEDED, + void *arg UNNEEDED) +{ fprintf(stderr, "dump_memleak_ called!\n"); abort(); } /* Generated stub for fromwire_basepoints */ void fromwire_basepoints(const u8 **ptr UNNEEDED, size_t *max UNNEEDED, struct basepoints *b UNNEEDED) @@ -101,7 +102,7 @@ void memleak_scan_obj(struct htable *memtable UNNEEDED, const void *obj UNNEEDED struct htable *memleak_start(const tal_t *ctx UNNEEDED) { fprintf(stderr, "memleak_start called!\n"); abort(); } /* Generated stub for memleak_status_broken */ -void memleak_status_broken(const char *fmt UNNEEDED, ...) +void memleak_status_broken(void *unused UNNEEDED, const char *fmt UNNEEDED, ...) { fprintf(stderr, "memleak_status_broken called!\n"); abort(); } /* Generated stub for new_coin_channel_close */ struct chain_coin_mvt *new_coin_channel_close(const tal_t *ctx UNNEEDED, diff --git a/onchaind/test/run-grind_feerate.c b/onchaind/test/run-grind_feerate.c index 5cc527afa..4ac501957 100644 --- a/onchaind/test/run-grind_feerate.c +++ b/onchaind/test/run-grind_feerate.c @@ -29,10 +29,11 @@ bool derive_keyset(const struct pubkey *per_commitment_point UNNEEDED, bool option_static_remotekey UNNEEDED, struct keyset *keyset UNNEEDED) { fprintf(stderr, "derive_keyset called!\n"); abort(); } -/* Generated stub for dump_memleak */ -bool dump_memleak(struct htable *memtable UNNEEDED, - void (*print)(const char *fmt UNNEEDED, ...)) -{ fprintf(stderr, "dump_memleak called!\n"); abort(); } +/* Generated stub for dump_memleak_ */ +bool dump_memleak_(struct htable *memtable UNNEEDED, + void (*print)(void *arg UNNEEDED, const char *fmt UNNEEDED, ...) UNNEEDED, + void *arg UNNEEDED) +{ fprintf(stderr, "dump_memleak_ called!\n"); abort(); } /* Generated stub for fromwire */ const u8 *fromwire(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, void *copy UNNEEDED, size_t n UNNEEDED) { fprintf(stderr, "fromwire called!\n"); abort(); } @@ -148,7 +149,7 @@ void memleak_scan_obj(struct htable *memtable UNNEEDED, const void *obj UNNEEDED struct htable *memleak_start(const tal_t *ctx UNNEEDED) { fprintf(stderr, "memleak_start called!\n"); abort(); } /* Generated stub for memleak_status_broken */ -void memleak_status_broken(const char *fmt UNNEEDED, ...) +void memleak_status_broken(void *unused UNNEEDED, const char *fmt UNNEEDED, ...) { fprintf(stderr, "memleak_status_broken called!\n"); abort(); } /* Generated stub for new_coin_channel_close */ struct chain_coin_mvt *new_coin_channel_close(const tal_t *ctx UNNEEDED, diff --git a/openingd/dualopend.c b/openingd/dualopend.c index 0f3c1a7ef..5a999c45e 100644 --- a/openingd/dualopend.c +++ b/openingd/dualopend.c @@ -976,7 +976,7 @@ static void handle_dev_memleak(struct state *state, const u8 *msg) memleak_scan_obj(memtable, state); /* If there's anything left, dump it to logs, and return true. */ - found_leak = dump_memleak(memtable, memleak_status_broken); + found_leak = dump_memleak(memtable, memleak_status_broken, NULL); wire_sync_write(REQ_FD, take(towire_dualopend_dev_memleak_reply(NULL, found_leak))); diff --git a/openingd/openingd.c b/openingd/openingd.c index 4d31b0497..520276fca 100644 --- a/openingd/openingd.c +++ b/openingd/openingd.c @@ -1419,7 +1419,7 @@ static void handle_dev_memleak(struct state *state, const u8 *msg) memleak_scan_obj(memtable, state); /* If there's anything left, dump it to logs, and return true. */ - found_leak = dump_memleak(memtable, memleak_status_broken); + found_leak = dump_memleak(memtable, memleak_status_broken, NULL); wire_sync_write(REQ_FD, take(towire_openingd_dev_memleak_reply(NULL, found_leak))); diff --git a/plugins/libplugin.c b/plugins/libplugin.c index a3c2d5d51..4ff1508b7 100644 --- a/plugins/libplugin.c +++ b/plugins/libplugin.c @@ -1513,14 +1513,12 @@ void plugin_log(struct plugin *p, enum log_level l, const char *fmt, ...) va_end(ap); } -/* Hack since we have no extra ptr to log_memleak */ -static struct plugin *memleak_plugin; -static void PRINTF_FMT(1,2) log_memleak(const char *fmt, ...) +static void PRINTF_FMT(2,3) log_memleak(struct plugin *plugin, const char *fmt, ...) { va_list ap; va_start(ap, fmt); - plugin_logv(memleak_plugin, LOG_BROKEN, fmt, ap); + plugin_logv(plugin, LOG_BROKEN, fmt, ap); va_end(ap); } @@ -1546,8 +1544,7 @@ static void memleak_check(struct plugin *plugin, struct command *cmd) if (plugin->mark_mem) plugin->mark_mem(plugin, memtable); - memleak_plugin = plugin; - dump_memleak(memtable, log_memleak); + dump_memleak(memtable, log_memleak, plugin); } void plugin_set_memleak_handler(struct plugin *plugin,