Skip to content

Commit 2e06d81

Browse files
rustyrussellvincenzopalazzo
authored andcommitted
lightningd: fix false positive in leak detection.
We ask each channeld to report its leaks, and keep a refcount of how many are outstanding. When the channeld replies, or dies, we decrement the count in a destructor. But if the last channeld we're waiting for dies, we can call the destructor in an unexpected place, and thus run leak detection when we were partway through some operation, which gives a false positive. Here's a backtrace showing it: ``` 0x5f93405897e3 send_backtrace common/daemon.c:33 0x5f93405381cf finish_report lightningd/memdump.c:139 0x5f93405382a0 leak_detect_req_done lightningd/memdump.c:172 0x5f9340705c41 notify ccan/ccan/tal/tal.c:243 0x5f9340705ca5 del_tree ccan/ccan/tal/tal.c:437 0x5f9340705ce8 del_tree ccan/ccan/tal/tal.c:447 0x5f93407061f3 tal_free ccan/ccan/tal/tal.c:532 0x5f9340563f5a subd_release_channel lightningd/subd.c:924 0x5f934050fb91 channel_set_owner lightningd/channel.c:31 0x5f9340511b84 channel_err lightningd/channel.c:1081 0x5f93405121a3 channel_fail_transient lightningd/channel.c:1095 0x5f934054e547 peer_channels_cleanup lightningd/peer_control.c:187 0x5f9340550411 peer_connected lightningd/peer_control.c:1689 0x5f9340525101 connectd_msg lightningd/connect_control.c:629 ``` Instead, do the lightningd detection up-front, where it's in a clean environment. Reported-by: Shahana Farooqui Signed-off-by: Rusty Russell <[email protected]>
1 parent 0b4b4c5 commit 2e06d81

File tree

1 file changed

+34
-32
lines changed

1 file changed

+34
-32
lines changed

lightningd/memdump.c

Lines changed: 34 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,6 @@ static void memleak_log(struct logger *log, const char *fmt, ...)
9696

9797
static void finish_report(const struct leak_detect *leaks)
9898
{
99-
struct htable *memtable;
100-
struct command *cmd;
101-
struct lightningd *ld;
10299
struct json_stream *response;
103100

104101
/* If it timed out, we free ourselved and exit! */
@@ -107,34 +104,7 @@ static void finish_report(const struct leak_detect *leaks)
107104
return;
108105
}
109106

110-
/* Convenience variables */
111-
cmd = leaks->cmd;
112-
ld = cmd->ld;
113-
114-
/* Enter everything, except this cmd and its jcon */
115-
memtable = memleak_start(cmd);
116-
117-
/* This command is not a leak! */
118-
memleak_ptr(memtable, cmd);
119-
memleak_ignore_children(memtable, cmd);
120-
121-
/* First delete known false positives. */
122-
memleak_scan_htable(memtable, &ld->topology->txwatches->raw);
123-
memleak_scan_htable(memtable, &ld->topology->txowatches->raw);
124-
memleak_scan_htable(memtable, &ld->topology->outgoing_txs->raw);
125-
memleak_scan_htable(memtable, &ld->htlcs_in->raw);
126-
memleak_scan_htable(memtable, &ld->htlcs_out->raw);
127-
memleak_scan_htable(memtable, &ld->htlc_sets->raw);
128-
memleak_scan_htable(memtable, &ld->peers->raw);
129-
memleak_scan_htable(memtable, &ld->peers_by_dbid->raw);
130-
131-
/* Now delete ld and those which it has pointers to. */
132-
memleak_scan_obj(memtable, ld);
133-
134-
if (dump_memleak(memtable, memleak_log, ld->log))
135-
tal_arr_expand(&leaks->leakers, "lightningd");
136-
137-
response = json_stream_success(cmd);
107+
response = json_stream_success(leaks->cmd);
138108
json_array_start(response, "leaks");
139109
for (size_t num_leakers = 0;
140110
num_leakers < tal_count(leaks->leakers);
@@ -146,7 +116,7 @@ static void finish_report(const struct leak_detect *leaks)
146116
json_array_end(response);
147117

148118
/* Command is now done. */
149-
was_pending(command_success(cmd, response));
119+
was_pending(command_success(leaks->cmd, response));
150120
}
151121

152122
static void leak_detect_timeout(struct leak_detect *leak_detect)
@@ -210,6 +180,34 @@ static void connect_dev_memleak_done(struct subd *connectd,
210180
report_subd_memleak(leaks, connectd);
211181
}
212182

183+
static bool lightningd_check_leaks(struct command *cmd)
184+
{
185+
struct lightningd *ld = cmd->ld;
186+
struct htable *memtable;
187+
188+
/* Enter everything, except this cmd and its jcon */
189+
memtable = memleak_start(cmd);
190+
191+
/* This command is not a leak! */
192+
memleak_ptr(memtable, cmd);
193+
memleak_ignore_children(memtable, cmd);
194+
195+
/* First delete known false positives. */
196+
memleak_scan_htable(memtable, &ld->topology->txwatches->raw);
197+
memleak_scan_htable(memtable, &ld->topology->txowatches->raw);
198+
memleak_scan_htable(memtable, &ld->topology->outgoing_txs->raw);
199+
memleak_scan_htable(memtable, &ld->htlcs_in->raw);
200+
memleak_scan_htable(memtable, &ld->htlcs_out->raw);
201+
memleak_scan_htable(memtable, &ld->htlc_sets->raw);
202+
memleak_scan_htable(memtable, &ld->peers->raw);
203+
memleak_scan_htable(memtable, &ld->peers_by_dbid->raw);
204+
205+
/* Now delete ld and those which it has pointers to. */
206+
memleak_scan_obj(memtable, ld);
207+
208+
return dump_memleak(memtable, memleak_log, ld->log);
209+
}
210+
213211
static struct command_result *json_memleak(struct command *cmd,
214212
const char *buffer,
215213
const jsmntok_t *obj UNNEEDED,
@@ -236,6 +234,10 @@ static struct command_result *json_memleak(struct command *cmd,
236234
leaks->num_outstanding_requests = 0;
237235
leaks->leakers = tal_arr(leaks, const char *, 0);
238236

237+
/* Check for our own leaks. */
238+
if (lightningd_check_leaks(cmd))
239+
tal_arr_expand(&leaks->leakers, "lightningd");
240+
239241
/* hsmd is sync, so do that first. */
240242
msg = hsm_sync_req(tmpctx, cmd->ld, take(towire_hsmd_dev_memleak(NULL)));
241243
if (!fromwire_hsmd_dev_memleak_reply(msg, &found_leak))

0 commit comments

Comments
 (0)