From f37f2b61935297ba03d07a0be9bd1d65744b3b6d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 23 Sep 2020 11:07:04 +0930 Subject: [PATCH] common/memleak: simplify and document API. 1. Rename memleak_enter_allocations to memleak_find_allocations. 2. Unify scanning for pointers into memleak_remove_region / memleak_remove_pointer. 3. Document the functions. Signed-off-by: Rusty Russell --- channeld/channeld.c | 4 +- closingd/closingd.c | 14 ++-- common/memleak.c | 49 ++++++++---- common/memleak.h | 109 +++++++++++++++++++------- connectd/connectd.c | 4 +- gossipd/gossipd.c | 4 +- hsmd/hsmd.c | 15 ++-- lightningd/memdump.c | 6 +- onchaind/onchaind.c | 27 ++----- onchaind/test/run-grind_feerate-bug.c | 21 +++-- onchaind/test/run-grind_feerate.c | 21 +++-- openingd/dualopend.c | 4 +- openingd/openingd.c | 4 +- 13 files changed, 166 insertions(+), 116 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index a48df2b0b..25cfdac98 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -3088,10 +3088,10 @@ static void handle_dev_memleak(struct peer *peer, const u8 *msg) struct htable *memtable; bool found_leak; - memtable = memleak_enter_allocations(tmpctx, msg, msg); + memtable = memleak_find_allocations(tmpctx, msg, msg); /* Now delete peer and things it has pointers to. */ - memleak_remove_referenced(memtable, peer); + memleak_remove_region(memtable, peer, tal_bytelen(peer)); found_leak = dump_memleak(memtable); wire_sync_write(MASTER_FD, diff --git a/closingd/closingd.c b/closingd/closingd.c index f2f19bca2..517481cb2 100644 --- a/closingd/closingd.c +++ b/closingd/closingd.c @@ -581,14 +581,12 @@ static void closing_dev_memleak(const tal_t *ctx, { struct htable *memtable; - memtable = memleak_enter_allocations(tmpctx, - scriptpubkey[LOCAL], - scriptpubkey[REMOTE]); + memtable = memleak_find_allocations(tmpctx, NULL, NULL); - /* Now delete known pointers (these aren't really roots, just - * pointers we know are referenced).*/ - memleak_remove_referenced(memtable, ctx); - memleak_remove_referenced(memtable, funding_wscript); + memleak_remove_pointer(memtable, ctx); + memleak_remove_pointer(memtable, scriptpubkey[LOCAL]); + memleak_remove_pointer(memtable, scriptpubkey[REMOTE]); + memleak_remove_pointer(memtable, funding_wscript); dump_memleak(memtable); } @@ -653,7 +651,7 @@ int main(int argc, char *argv[]) master_badmsg(WIRE_CLOSINGD_INIT, msg); /* stdin == requests, 3 == peer, 4 = gossip, 5 = gossip_store, 6 = hsmd */ - per_peer_state_set_fds(pps, 3, 4, 5); + per_peer_state_set_fds(notleak(pps), 3, 4, 5); snprintf(fee_negotiation_step_str, sizeof(fee_negotiation_step_str), "%" PRIu64 "%s", fee_negotiation_step, diff --git a/common/memleak.c b/common/memleak.c index 03a7d96c3..3a8852bf9 100644 --- a/common/memleak.c +++ b/common/memleak.c @@ -1,3 +1,24 @@ +/* Hello friends! + * + * You found me! This is the inner, deep magic. Right here. + * + * To help development, we have a complete set of routines to scan for + * tal-memory leaks (valgrind will detect non-tal memory leaks at exit, + * but tal hierarchies tends to get freed at exit, so we need something + * more sophisticated). + * + * Memleak detection is only active if DEVELOPER is set. It does several + * things: + * 1. attaches a backtrace list to every allocation, so we can tell + * where it came from. + * 2. when memleak_find_allocations() is called, walks the entire tal + * tree and saves a pointer to all the objects it finds, with + * a few internal exceptions (including everything under 'tmpctx'). + * It then calls registered helpers, which can remove opaque things + * and handles notleak() objects. + * 3. provides a routine to access any remaining pointers in the + * table: these are the leaks. + */ #include #include #include @@ -102,8 +123,8 @@ static void scan_for_pointers(struct htable *memtable, } } -void memleak_scan_region(struct htable *memtable, - const void *ptr, size_t bytelen) +void memleak_remove_region(struct htable *memtable, + const void *ptr, size_t bytelen) { pointer_referenced(memtable, ptr); scan_for_pointers(memtable, ptr, bytelen); @@ -118,15 +139,6 @@ static void remove_with_children(struct htable *memtable, const tal_t *p) remove_with_children(memtable, i); } -void memleak_remove_referenced(struct htable *memtable, const void *root) -{ - /* Now delete the ones which are referenced. */ - memleak_scan_region(memtable, root, tal_bytelen(root)); - - /* Remove memtable itself */ - pointer_referenced(memtable, memtable); -} - /* memleak can't see inside hash tables, so do them manually */ void memleak_remove_htable(struct htable *memtable, const struct htable *ht) { @@ -134,7 +146,7 @@ void memleak_remove_htable(struct htable *memtable, const struct htable *ht) const void *p; for (p = htable_first(ht, &i); p; p = htable_next(ht, &i)) - memleak_scan_region(memtable, p, tal_bytelen(p)); + memleak_remove_region(memtable, p, tal_bytelen(p)); } /* FIXME: If uintmap used tal, this wouldn't be necessary! */ @@ -144,7 +156,7 @@ void memleak_remove_intmap_(struct htable *memtable, const struct intmap *m) intmap_index_t i; for (p = intmap_first_(m, &i); p; p = intmap_after_(m, &i)) - memleak_scan_region(memtable, p, tal_bytelen(p)); + memleak_remove_region(memtable, p, tal_bytelen(p)); } static bool ptr_match(const void *candidate, void *ptr) @@ -157,6 +169,9 @@ const void *memleak_get(struct htable *memtable, const uintptr_t **backtrace) struct htable_iter it; const tal_t *i, *p; + /* Remove memtable itself */ + pointer_referenced(memtable, memtable); + i = htable_first(memtable, &it); if (!i) return NULL; @@ -239,7 +254,7 @@ static void call_memleak_helpers(struct htable *memtable, const tal_t *p) remove_with_children(memtable, p); else pointer_referenced(memtable, p); - memleak_scan_region(memtable, p, tal_bytelen(p)); + memleak_remove_region(memtable, p, tal_bytelen(p)); } else if (name && strends(name, "_notleak")) { pointer_referenced(memtable, i); call_memleak_helpers(memtable, i); @@ -249,9 +264,9 @@ static void call_memleak_helpers(struct htable *memtable, const tal_t *p) } } -struct htable *memleak_enter_allocations(const tal_t *ctx, - const void *exclude1, - const void *exclude2) +struct htable *memleak_find_allocations(const tal_t *ctx, + const void *exclude1, + const void *exclude2) { struct htable *memtable = tal(ctx, struct htable); htable_init(memtable, hash_ptr, NULL); diff --git a/common/memleak.h b/common/memleak.h index 8852b99e3..1afa9725c 100644 --- a/common/memleak.h +++ b/common/memleak.h @@ -8,26 +8,48 @@ struct htable; +/** + * memleak_init: Initialize memleak detection; you call this at the start! + * + * notleak() won't have an effect if called before this (but naming + * tal objects with suffix _notleak works). + */ +void memleak_init(void); + +/** + * notleak: remove a false-positive tal object. + * @p: the tal allocation. + * + * This marks a tal pointer (and anything it refers to) as not being + * leaked. Think hard before using this! + */ +#define notleak(p) ((memleak_typeof(p))notleak_((p), false)) + +/* Mark a pointer and all its tal children as not being leaked. + * You don't want this; it's for simplifying handling of the incoming + * command which asks lightningd to do the dev check. */ +#define notleak_with_children(p) ((memleak_typeof(p))notleak_((p), true)) + #if HAVE_TYPEOF #define memleak_typeof(var) typeof(var) #else #define memleak_typeof(var) void * #endif /* !HAVE_TYPEOF */ -/* Mark a pointer as not being leaked. */ -#define notleak(p) ((memleak_typeof(p))notleak_((p), false)) - -/* Mark a pointer and all its tal children as not being leaked. */ -#define notleak_with_children(p) ((memleak_typeof(p))notleak_((p), true)) - void *notleak_(const void *ptr, bool plus_children); -/* Mark a helper to be called to scan this structure for mem references */ -/* For update-mock: memleak_add_helper_ mock empty */ -void memleak_add_helper_(const tal_t *p, void (*cb)(struct htable *memtable, - const tal_t *)); - #if DEVELOPER +/** + * memleak_add_helper: help memleak look inside this tal object + * @p: the tal object + * @cb: the callback. + * + * memleak looks for tal pointers inside a tal object memory, but some + * structures which use bit-stealing on pointers or use non-tal allocations + * will need this. + * + * The callback usually calls memleak_remove_*. + */ #define memleak_add_helper(p, cb) \ memleak_add_helper_((p), \ typesafe_cb_preargs(void, const tal_t *, \ @@ -38,32 +60,63 @@ void memleak_add_helper_(const tal_t *p, void (*cb)(struct htable *memtable, #define memleak_add_helper(p, cb) #endif -/* Initialize memleak detection */ -void memleak_init(void); - -/* Allocate a htable with all the memory we've allocated. */ -struct htable *memleak_enter_allocations(const tal_t *ctx, - const void *exclude1, - const void *exclude2); - -/* Remove any pointers to memory under root */ -void memleak_remove_referenced(struct htable *memtable, const void *root); +/* For update-mock: memleak_add_helper_ mock empty */ +void memleak_add_helper_(const tal_t *p, void (*cb)(struct htable *memtable, + const tal_t *)); -/* Remove any pointers inside this htable (which is opaque to memleak). */ +/** + * memleak_find_allocations: allocate a htable with all tal objects; + * @ctx: the context to allocate the htable from + * @exclude1: one tal pointer to exclude from adding (if non-NULL) + * @exclude2: second tal pointer to exclude from adding (if non-NULL) + * + * Note that exclude1 and exclude2's tal children are also not added. + */ +struct htable *memleak_find_allocations(const tal_t *ctx, + const void *exclude1, + const void *exclude2); + +/** + * memleak_remove_region - remove this region and anything it references + * @memtable: the memtable create by memleak_find_allocations. + * @p: the pointer to remove. + * @bytelen: the bytes within it to scan for more pointers. + * + * This removes @p from the memtable, then looks for any tal pointers + * inside between @p and @p + @bytelen and calls + * memleak_remove_region() on those if not already removed. + */ +void memleak_remove_region(struct htable *memtable, + const void *p, size_t bytelen); + +/** + * memleak_remove_pointer - remove this pointer + * @memtable: the memtable create by memleak_find_allocations. + * @p: the pointer to remove. + * + * This removes @p from the memtable. + */ +#define memleak_remove_pointer(memtable, p) \ + memleak_remove_region((memtable), (p), 0) + +/* Helper to remove objects inside this htable (which is opaque to memleak). */ void memleak_remove_htable(struct htable *memtable, const struct htable *ht); -/* Remove any pointers inside this uintmap (which is opaque to memleak). */ +/* Helper to remove objects inside this uintmap (which is opaque to memleak). */ #define memleak_remove_uintmap(memtable, umap) \ memleak_remove_intmap_(memtable, uintmap_unwrap_(umap)) struct intmap; void memleak_remove_intmap_(struct htable *memtable, const struct intmap *m); -/* Mark this pointer as being referenced, and search within for more. */ -void memleak_scan_region(struct htable *memtable, - const void *p, size_t bytelen); - -/* Get (and remove) a leak from memtable, or NULL */ +/** + * memleak_get: get (and remove) a leak from memtable, or NULL + * @memtable: the memtable after all known allocations removed. + * @backtrace: the backtrace to set if there is one. + * + * If this returns NULL, it means the @memtable was empty. Otherwise + * it return a pointer to a leak (and removes it from @memtable) + */ const void *memleak_get(struct htable *memtable, const uintptr_t **backtrace); extern struct backtrace_state *backtrace_state; diff --git a/connectd/connectd.c b/connectd/connectd.c index 157983733..986ea2511 100644 --- a/connectd/connectd.c +++ b/connectd/connectd.c @@ -1571,10 +1571,10 @@ static struct io_plan *dev_connect_memleak(struct io_conn *conn, struct htable *memtable; bool found_leak; - memtable = memleak_enter_allocations(tmpctx, msg, msg); + memtable = memleak_find_allocations(tmpctx, msg, msg); /* Now delete daemon and those which it has pointers to. */ - memleak_remove_referenced(memtable, daemon); + memleak_remove_region(memtable, daemon, sizeof(daemon)); found_leak = dump_memleak(memtable); daemon_conn_send(daemon->master, diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index ceeaaba59..1334c290a 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -1346,10 +1346,10 @@ static struct io_plan *dev_gossip_memleak(struct io_conn *conn, struct htable *memtable; bool found_leak; - memtable = memleak_enter_allocations(tmpctx, msg, msg); + memtable = memleak_find_allocations(tmpctx, msg, msg); /* Now delete daemon and those which it has pointers to. */ - memleak_remove_referenced(memtable, daemon); + memleak_remove_region(memtable, daemon, sizeof(*daemon)); found_leak = dump_memleak(memtable); daemon_conn_send(daemon->master, diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 70fd76c53..2cfde5942 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -1799,17 +1799,18 @@ static struct io_plan *handle_memleak(struct io_conn *conn, bool found_leak; u8 *reply; - memtable = memleak_enter_allocations(tmpctx, msg_in, msg_in); + memtable = memleak_find_allocations(tmpctx, msg_in, msg_in); /* Now delete clients and anything they point to. */ - memleak_remove_referenced(memtable, c); - memleak_scan_region(memtable, - dbid_zero_clients, sizeof(dbid_zero_clients)); + memleak_remove_region(memtable, c, tal_bytelen(c)); + memleak_remove_region(memtable, + dbid_zero_clients, sizeof(dbid_zero_clients)); memleak_remove_uintmap(memtable, &clients); - memleak_scan_region(memtable, status_conn, tal_bytelen(status_conn)); + memleak_remove_region(memtable, + status_conn, tal_bytelen(status_conn)); - memleak_scan_region(memtable, dev_force_privkey, 0); - memleak_scan_region(memtable, dev_force_bip32_seed, 0); + memleak_remove_pointer(memtable, dev_force_privkey); + memleak_remove_pointer(memtable, dev_force_bip32_seed); found_leak = dump_memleak(memtable); reply = towire_hsmd_dev_memleak_reply(NULL, found_leak); diff --git a/lightningd/memdump.c b/lightningd/memdump.c index ba5cafbf8..2ddda59cb 100644 --- a/lightningd/memdump.c +++ b/lightningd/memdump.c @@ -132,7 +132,7 @@ static bool handle_strmap(const char *member, void *p, void *memtable_) { struct htable *memtable = memtable_; - memleak_scan_region(memtable, p, tal_bytelen(p)); + memleak_remove_region(memtable, p, tal_bytelen(p)); /* Keep going */ return true; @@ -154,7 +154,7 @@ static void scan_mem(struct command *cmd, const uintptr_t *backtrace; /* Enter everything, except this cmd and its jcon */ - memtable = memleak_enter_allocations(cmd, cmd, cmd->jcon); + memtable = memleak_find_allocations(cmd, cmd, cmd->jcon); /* First delete known false positives. */ memleak_remove_htable(memtable, &ld->topology->txwatches.raw); @@ -164,7 +164,7 @@ static void scan_mem(struct command *cmd, memleak_remove_htable(memtable, &ld->htlc_sets.raw); /* Now delete ld and those which it has pointers to. */ - memleak_remove_referenced(memtable, ld); + memleak_remove_region(memtable, ld, sizeof(*ld)); json_array_start(response, "leaks"); while ((i = memleak_get(memtable, &backtrace)) != NULL) { diff --git a/onchaind/onchaind.c b/onchaind/onchaind.c index f0f055c14..e61203554 100644 --- a/onchaind/onchaind.c +++ b/onchaind/onchaind.c @@ -2087,24 +2087,13 @@ static void handle_preimage(struct tracked_output **outs, #if DEVELOPER static void memleak_remove_globals(struct htable *memtable, const tal_t *topctx) { - /* memleak_scan_region is overkill if these are simple pointers to - * objects which don't contain pointers, but it works. */ if (keyset) - memleak_scan_region(memtable, keyset, sizeof(*keyset)); - - if (remote_per_commitment_point) - memleak_scan_region(memtable, remote_per_commitment_point, - sizeof(*remote_per_commitment_point)); - - if (remote_per_commitment_secret) - memleak_scan_region(memtable, remote_per_commitment_secret, - sizeof(*remote_per_commitment_secret)); - - /* top-level context args */ - memleak_scan_region(memtable, topctx, 0); - - memleak_scan_region(memtable, missing_htlc_msgs, - tal_bytelen(missing_htlc_msgs)); + memleak_remove_region(memtable, keyset, sizeof(*keyset)); + memleak_remove_pointer(memtable, remote_per_commitment_point); + memleak_remove_pointer(memtable, remote_per_commitment_secret); + memleak_remove_pointer(memtable, topctx); + memleak_remove_region(memtable, + missing_htlc_msgs, tal_bytelen(missing_htlc_msgs)); } static bool handle_dev_memleak(struct tracked_output **outs, const u8 *msg) @@ -2115,10 +2104,10 @@ static bool handle_dev_memleak(struct tracked_output **outs, const u8 *msg) if (!fromwire_onchaind_dev_memleak(msg)) return false; - memtable = memleak_enter_allocations(tmpctx, msg, msg); + memtable = memleak_find_allocations(tmpctx, msg, msg); /* Top-level context is parent of outs */ memleak_remove_globals(memtable, tal_parent(outs)); - memleak_remove_referenced(memtable, outs); + memleak_remove_region(memtable, outs, tal_bytelen(outs)); found_leak = dump_memleak(memtable); wire_sync_write(REQ_FD, diff --git a/onchaind/test/run-grind_feerate-bug.c b/onchaind/test/run-grind_feerate-bug.c index 9713db507..9fd8f3b8c 100644 --- a/onchaind/test/run-grind_feerate-bug.c +++ b/onchaind/test/run-grind_feerate-bug.c @@ -112,18 +112,15 @@ struct bitcoin_tx *htlc_success_tx(const tal_t *ctx UNNEEDED, /* Generated stub for master_badmsg */ void master_badmsg(u32 type_expected UNNEEDED, const u8 *msg) { fprintf(stderr, "master_badmsg called!\n"); abort(); } -/* Generated stub for memleak_enter_allocations */ -struct htable *memleak_enter_allocations(const tal_t *ctx UNNEEDED, - const void *exclude1 UNNEEDED, - const void *exclude2 UNNEEDED) -{ fprintf(stderr, "memleak_enter_allocations called!\n"); abort(); } -/* Generated stub for memleak_remove_referenced */ -void memleak_remove_referenced(struct htable *memtable UNNEEDED, const void *root UNNEEDED) -{ fprintf(stderr, "memleak_remove_referenced called!\n"); abort(); } -/* Generated stub for memleak_scan_region */ -void memleak_scan_region(struct htable *memtable UNNEEDED, - const void *p UNNEEDED, size_t bytelen UNNEEDED) -{ fprintf(stderr, "memleak_scan_region called!\n"); abort(); } +/* Generated stub for memleak_find_allocations */ +struct htable *memleak_find_allocations(const tal_t *ctx UNNEEDED, + const void *exclude1 UNNEEDED, + const void *exclude2 UNNEEDED) +{ fprintf(stderr, "memleak_find_allocations called!\n"); abort(); } +/* Generated stub for memleak_remove_region */ +void memleak_remove_region(struct htable *memtable UNNEEDED, + const void *p UNNEEDED, size_t bytelen UNNEEDED) +{ fprintf(stderr, "memleak_remove_region called!\n"); abort(); } /* Generated stub for new_coin_chain_fees */ struct chain_coin_mvt *new_coin_chain_fees(const tal_t *ctx UNNEEDED, const char *account_name UNNEEDED, diff --git a/onchaind/test/run-grind_feerate.c b/onchaind/test/run-grind_feerate.c index 1f9b3a13e..c1168f4db 100644 --- a/onchaind/test/run-grind_feerate.c +++ b/onchaind/test/run-grind_feerate.c @@ -126,18 +126,15 @@ struct bitcoin_tx *htlc_timeout_tx(const tal_t *ctx UNNEEDED, /* Generated stub for master_badmsg */ void master_badmsg(u32 type_expected UNNEEDED, const u8 *msg) { fprintf(stderr, "master_badmsg called!\n"); abort(); } -/* Generated stub for memleak_enter_allocations */ -struct htable *memleak_enter_allocations(const tal_t *ctx UNNEEDED, - const void *exclude1 UNNEEDED, - const void *exclude2 UNNEEDED) -{ fprintf(stderr, "memleak_enter_allocations called!\n"); abort(); } -/* Generated stub for memleak_remove_referenced */ -void memleak_remove_referenced(struct htable *memtable UNNEEDED, const void *root UNNEEDED) -{ fprintf(stderr, "memleak_remove_referenced called!\n"); abort(); } -/* Generated stub for memleak_scan_region */ -void memleak_scan_region(struct htable *memtable UNNEEDED, - const void *p UNNEEDED, size_t bytelen UNNEEDED) -{ fprintf(stderr, "memleak_scan_region called!\n"); abort(); } +/* Generated stub for memleak_find_allocations */ +struct htable *memleak_find_allocations(const tal_t *ctx UNNEEDED, + const void *exclude1 UNNEEDED, + const void *exclude2 UNNEEDED) +{ fprintf(stderr, "memleak_find_allocations called!\n"); abort(); } +/* Generated stub for memleak_remove_region */ +void memleak_remove_region(struct htable *memtable UNNEEDED, + const void *p UNNEEDED, size_t bytelen UNNEEDED) +{ fprintf(stderr, "memleak_remove_region called!\n"); abort(); } /* Generated stub for new_coin_chain_fees */ struct chain_coin_mvt *new_coin_chain_fees(const tal_t *ctx UNNEEDED, const char *account_name UNNEEDED, diff --git a/openingd/dualopend.c b/openingd/dualopend.c index fac2456d9..7d265cf60 100644 --- a/openingd/dualopend.c +++ b/openingd/dualopend.c @@ -1223,10 +1223,10 @@ static void handle_dev_memleak(struct state *state, const u8 *msg) /* Populate a hash table with all our allocations (except msg, which * is in use right now). */ - memtable = memleak_enter_allocations(tmpctx, msg, msg); + memtable = memleak_find_allocations(tmpctx, msg, msg); /* Now delete state and things it has pointers to. */ - memleak_remove_referenced(memtable, state); + 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); diff --git a/openingd/openingd.c b/openingd/openingd.c index db83ab0ce..75e1c6956 100644 --- a/openingd/openingd.c +++ b/openingd/openingd.c @@ -1262,10 +1262,10 @@ static void handle_dev_memleak(struct state *state, const u8 *msg) /* Populate a hash table with all our allocations (except msg, which * is in use right now). */ - memtable = memleak_enter_allocations(tmpctx, msg, msg); + memtable = memleak_find_allocations(tmpctx, msg, msg); /* Now delete state and things it has pointers to. */ - memleak_remove_referenced(memtable, state); + memleak_remove_region(memtable, state, sizeof(*state)); /* If there's anything left, dump it to logs, and return true. */ found_leak = dump_memleak(memtable);