memleak hook: find leaks in plugins

ccan/README

@@ -1,3 +1,3 @@
 CCAN imported from http://ccodearchive.net.
 
-CCAN version: init-2506-gec95c3c5
+CCAN version: init-2507-g05ec8351

ccan/ccan/tal/tal.c

@@ -824,36 +824,36 @@ static void dump_node(unsigned int indent, const struct tal_hdr *t)
         const struct prop_hdr *p;
 
 	for (i = 0; i < indent; i++)
-		printf("  ");
-	printf("%p len=%zu", t, t->bytelen);
+		fprintf(stderr, "  ");
+	fprintf(stderr, "%p len=%zu", t, t->bytelen);
         for (p = t->prop; p; p = p->next) {
 		struct children *c;
 		struct name *n;
 		struct notifier *no;
                 if (is_literal(p)) {
-			printf(" \"%s\"", (const char *)p);
+			fprintf(stderr, " \"%s\"", (const char *)p);
 			break;
 		}
 		switch (p->type) {
 		case CHILDREN:
 			c = (struct children *)p;
-			printf(" CHILDREN(%p):parent=%p,children={%p,%p}\n",
+			fprintf(stderr, " CHILDREN(%p):parent=%p,children={%p,%p}\n",
 			       p, c->parent,
 			       c->children.n.prev, c->children.n.next);
 			break;
 		case NAME:
 			n = (struct name *)p;
-			printf(" NAME(%p):%s", p, n->name);
+			fprintf(stderr, " NAME(%p):%s", p, n->name);
 			break;
 		case NOTIFIER:
 			no = (struct notifier *)p;
-			printf(" NOTIFIER(%p):fn=%p", p, no->u.notifyfn);
+			fprintf(stderr, " NOTIFIER(%p):fn=%p", p, no->u.notifyfn);
 			break;
 		default:
-			printf(" **UNKNOWN(%p):%i**", p, p->type);
+			fprintf(stderr, " **UNKNOWN(%p):%i**", p, p->type);
 		}
 	}
-	printf("\n");
+	fprintf(stderr, "\n");
 }
 
 static void tal_dump_(unsigned int level, const struct tal_hdr *t)

ccan/ccan/tal/tal.h

@@ -456,7 +456,7 @@ bool tal_check(const tal_t *ctx, const char *errorstr);
 
 #ifdef CCAN_TAL_DEBUG
 /**
- * tal_dump - dump entire tal tree.
+ * tal_dump - dump entire tal tree to stderr.
  *
  * This is a helper for debugging tal itself, which dumps all the tal internal
  * state.

channeld/channeld.c

@@ -3496,7 +3496,7 @@ static void handle_dev_memleak(struct peer *peer, const u8 *msg)
 	/* Now delete peer and things it has pointers to. */
 	memleak_remove_region(memtable, peer, tal_bytelen(peer));
 
-	found_leak = dump_memleak(memtable);
+	found_leak = dump_memleak(memtable, memleak_status_broken);
 	wire_sync_write(MASTER_FD,
 			 take(towire_channeld_dev_memleak_reply(NULL,
 							       found_leak)));

closingd/closingd.c

@@ -485,7 +485,7 @@ static void closing_dev_memleak(const tal_t *ctx,
 	memleak_remove_pointer(memtable, scriptpubkey[REMOTE]);
 	memleak_remove_pointer(memtable, funding_wscript);
 
-	dump_memleak(memtable);
+	dump_memleak(memtable, memleak_status_broken);
 }
 #endif /* DEVELOPER */
 

common/memleak.c

@@ -160,6 +160,24 @@ void memleak_remove_intmap_(struct htable *memtable, const struct intmap *m)
 		memleak_remove_region(memtable, p, tal_bytelen(p));
 }
 
+static bool handle_strmap(const char *member, void *p, void *memtable_)
+{
+	struct htable *memtable = memtable_;
+
+	/* membername may *not* be a tal ptr, but it can be! */
+	pointer_referenced(memtable, member);
+	memleak_remove_region(memtable, p, tal_bytelen(p));
+
+	/* Keep going */
+	return true;
+}
+
+/* FIXME: If strmap used tal, this wouldn't be necessary! */
+void memleak_remove_strmap_(struct htable *memtable, const struct strmap *m)
+{
+	strmap_iterate_(m, handle_strmap, memtable);
+}
+
 static bool ptr_match(const void *candidate, void *ptr)
 {
 	return candidate == ptr;
@@ -290,6 +308,59 @@ void memleak_init(void)
 	if (backtrace_state)
 		add_backtrace_notifiers(NULL);
 }
+
+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;
+	/* This can happen in backtraces. */
+	if (!filename || !function)
+		return 0;
+
+	print("    %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, ...))
+{
+	if (!bt)
+		return;
+
+	/* First one serves as counter. */
+	print("  backtrace:");
+	for (size_t i = 1; i < bt[0]; i++) {
+		backtrace_pcinfo(backtrace_state,
+				 bt[i], dump_syminfo,
+				 NULL, print);
+	}
+}
+
+bool dump_memleak(struct htable *memtable,
+		  void PRINTF_FMT(1,2) (*print)(const char *fmt, ...))
+{
+	const tal_t *i;
+	const uintptr_t *backtrace;
+	bool found_leak = false;
+
+	while ((i = memleak_get(memtable, &backtrace)) != NULL) {
+		print("MEMLEAK: %p", i);
+		if (tal_name(i))
+			print("  label=%s", tal_name(i));
+
+		dump_leak_backtrace(backtrace, print);
+		print("  parents:");
+		for (tal_t *p = tal_parent(i); p; p = tal_parent(p)) {
+			print("    %s", tal_name(p));
+			p = tal_parent(p);
+		}
+		found_leak = true;
+	}
+
+	return found_leak;
+}
 #else /* !DEVELOPER */
 void *notleak_(const void *ptr, bool plus_children UNNEEDED)
 {

common/memleak.h

@@ -2,6 +2,7 @@
 #define LIGHTNING_COMMON_MEMLEAK_H
 #include "config.h"
 #include <ccan/cast/cast.h>
+#include <ccan/strmap/strmap.h>
 #include <ccan/tal/tal.h>
 #include <ccan/typesafe_cb/typesafe_cb.h>
 #include <inttypes.h>
@@ -109,6 +110,11 @@ void memleak_remove_htable(struct htable *memtable, const struct htable *ht);
 struct intmap;
 void memleak_remove_intmap_(struct htable *memtable, const struct intmap *m);
 
+/* Remove any pointers inside this strmap (which is opaque to memleak). */
+#define memleak_remove_strmap(memtable, strmap) \
+	memleak_remove_strmap_((memtable), tcon_unwrap(strmap))
+void memleak_remove_strmap_(struct htable *memtable, const struct strmap *m);
+
 /**
  * memleak_get: get (and remove) a leak from memtable, or NULL
  * @memtable: the memtable after all known allocations removed.
@@ -121,4 +127,10 @@ const void *memleak_get(struct htable *memtable, const uintptr_t **backtrace);
 
 extern struct backtrace_state *backtrace_state;
 
+#if DEVELOPER
+/* Only defined if DEVELOPER */
+bool dump_memleak(struct htable *memtable,
+		  void PRINTF_FMT(1,2) (*print)(const char *fmt, ...));
+#endif
+
 #endif /* LIGHTNING_COMMON_MEMLEAK_H */

common/status.c

@@ -226,3 +226,14 @@ void master_badmsg(u32 type_expected, const u8 *msg)
 		     "Error parsing %u: %s",
 		     type_expected, tal_hex(tmpctx, msg));
 }
+
+#if DEVELOPER
+/* Print BROKEN status: callback for dump_memleak. */
+void memleak_status_broken(const char *fmt, ...)
+{
+	va_list ap;
+	va_start(ap, fmt);
+	status_vfmt(LOG_BROKEN, NULL, fmt, ap);
+	va_end(ap);
+}
+#endif

common/status.h

@@ -74,4 +74,10 @@ void status_send_fatal(const u8 *msg TAKES) NORETURN;
 
 /* Only for sync status! */
 void status_send_fd(int fd);
+
+#if DEVELOPER
+/* Print BROKEN status: callback for dump_memleak. */
+void memleak_status_broken(const char *fmt, ...);
+#endif
+
 #endif /* LIGHTNING_COMMON_STATUS_H */

common/subdaemon.c

@@ -50,58 +50,3 @@ void subdaemon_setup(int argc, char *argv[])
 
 	daemon_setup(argv[0], status_backtrace_print, status_backtrace_exit);
 }
-
-#if DEVELOPER
- // Indented to avoid header-order check
- #include <backtrace.h>
- #include <common/memleak.h>
-
-static int dump_syminfo(void *data UNUSED, uintptr_t pc UNUSED,
-			const char *filename, int lineno,
-			const char *function)
-{
-	/* This can happen in backtraces. */
-	if (!filename || !function)
-		return 0;
-
-	status_debug("    %s:%u (%s)", filename, lineno, function);
-	return 0;
-}
-
-static void dump_leak_backtrace(const uintptr_t *bt)
-{
-	if (!bt)
-		return;
-
-	/* First one serves as counter. */
-	status_debug("  backtrace:");
-	for (size_t i = 1; i < bt[0]; i++) {
-		backtrace_pcinfo(backtrace_state,
-				 bt[i], dump_syminfo,
-				 NULL, NULL);
-	}
-}
-
-bool dump_memleak(struct htable *memtable)
-{
-	const tal_t *i;
-	const uintptr_t *backtrace;
-	bool found_leak = false;
-
-	while ((i = memleak_get(memtable, &backtrace)) != NULL) {
-		status_broken("MEMLEAK: %p", i);
-		if (tal_name(i))
-			status_broken("  label=%s", tal_name(i));
-
-		dump_leak_backtrace(backtrace);
-		status_broken("  parents:");
-		for (tal_t *p = tal_parent(i); p; p = tal_parent(p)) {
-			status_broken("    %s", tal_name(p));
-			p = tal_parent(p);
-		}
-		found_leak = true;
-	}
-
-	return found_leak;
-}
-#endif /* DEVELOPER */

common/subdaemon.h

@@ -7,7 +7,4 @@ struct htable;
 /* daemon_setup, but for subdaemons */
 void subdaemon_setup(int argc, char *argv[]);
 
-/* Only defined if DEVELOPER */
-bool dump_memleak(struct htable *memtable);
-
 #endif /* LIGHTNING_COMMON_SUBDAEMON_H */

connectd/connectd.c

@@ -1734,7 +1734,7 @@ static struct io_plan *dev_connect_memleak(struct io_conn *conn,
 	/* Now delete daemon and those which it has pointers to. */
 	memleak_remove_region(memtable, daemon, sizeof(daemon));
 
-	found_leak = dump_memleak(memtable);
+	found_leak = dump_memleak(memtable, memleak_status_broken);
 	daemon_conn_send(daemon->master,
 			 take(towire_connectd_dev_memleak_reply(NULL,
 							      found_leak)));

gossipd/gossipd.c

@@ -1260,7 +1260,7 @@ static struct io_plan *dev_gossip_memleak(struct io_conn *conn,
 	/* Now delete daemon and those which it has pointers to. */
 	memleak_remove_region(memtable, daemon, sizeof(*daemon));
 
-	found_leak = dump_memleak(memtable);
+	found_leak = dump_memleak(memtable, memleak_status_broken);
 	daemon_conn_send(daemon->master,
 			 take(towire_gossipd_dev_memleak_reply(NULL,
 							      found_leak)));

hsmd/hsmd.c

@@ -580,7 +580,7 @@ static struct io_plan *handle_memleak(struct io_conn *conn,
 	memleak_remove_pointer(memtable, dev_force_privkey);
 	memleak_remove_pointer(memtable, dev_force_bip32_seed);
 
-	found_leak = dump_memleak(memtable);
+	found_leak = dump_memleak(memtable, memleak_status_broken);
 	reply = towire_hsmd_dev_memleak_reply(NULL, found_leak);
 	return req_reply(conn, c, take(reply));
 }

lightningd/chaintopology.c

@@ -438,6 +438,8 @@ static void update_feerates(struct bitcoind *bitcoind,
 static void start_fee_estimate(struct chain_topology *topo)
 {
 	topo->updatefee_timer = NULL;
+	if (topo->stopping)
+		return;
 	/* Once per new block head, update fee estimates. */
 	bitcoind_estimate_fees(topo->bitcoind, NUM_FEERATES, update_feerates,
 			       topo);
@@ -937,6 +939,8 @@ static void get_new_block(struct bitcoind *bitcoind,
 static void try_extend_tip(struct chain_topology *topo)
 {
 	topo->extend_timer = NULL;
+	if (topo->stopping)
+		return;
 	bitcoind_getrawblockbyheight(topo->bitcoind, topo->tip->height + 1,
 				     get_new_block, topo);
 }
@@ -1066,6 +1070,7 @@ struct chain_topology *new_topology(struct lightningd *ld, struct log *log)
 	topo->feerate_uninitialized = true;
 	topo->root = NULL;
 	topo->sync_waiters = tal(topo, struct list_head);
+	topo->stopping = false;
 	list_head_init(topo->sync_waiters);
 
 	return topo;
@@ -1158,6 +1163,8 @@ check_chain(struct bitcoind *bitcoind, const char *chain,
 static void retry_check_chain(struct chain_topology *topo)
 {
 	topo->bitcoind->checkchain_timer = NULL;
+	if (topo->stopping)
+		return;
 	bitcoind_getchaininfo(topo->bitcoind, false, check_chain, topo);
 }
 
@@ -1194,9 +1201,11 @@ void begin_topology(struct chain_topology *topo)
 
 void stop_topology(struct chain_topology *topo)
 {
+	/* Stop timers from re-arming. */
+	topo->stopping = true;
+
 	/* Remove timers while we're cleaning up plugins. */
 	tal_free(topo->bitcoind->checkchain_timer);
 	tal_free(topo->extend_timer);
 	tal_free(topo->updatefee_timer);
 }
-

lightningd/chaintopology.h

@@ -126,6 +126,9 @@ struct chain_topology {
 	/* The number of headers known to the bitcoin backend at startup. Not
 	 * updated after the initial check. */
 	u32 headercount;
+
+	/* Are we stopped? */
+	bool stopping;
 };
 
 /* Information relevant to locating a TX in a blockchain. */

lightningd/memdump.c

@@ -128,22 +128,6 @@ static void json_add_backtrace(struct json_stream *response,
 	json_array_end(response);
 }
 
-static bool handle_strmap(const char *member, void *p, void *memtable_)
-{
-	struct htable *memtable = memtable_;
-
-	memleak_remove_region(memtable, p, tal_bytelen(p));
-
-	/* Keep going */
-	return true;
-}
-
-/* FIXME: If strmap used tal, this wouldn't be necessary! */
-void memleak_remove_strmap_(struct htable *memtable, const struct strmap *m)
-{
-	strmap_iterate_(m, handle_strmap, memtable);
-}
-
 static void scan_mem(struct command *cmd,
 		     struct json_stream *response,
 		     struct lightningd *ld,