Skip to content

Commit

Permalink
lightningd/chaintopology: fix use-after-free on shutdown.
Browse files Browse the repository at this point in the history
We were removing the timers, but if a callback to the plugin was
happening now, it could set the timer again on the way out.  The
correct thing to do is make the bitcoind plugin own all the timers, so
when it is freed, all the timers are freed too.

This is much neater than trying to free them explicitly anyway.

```
==77658== Invalid read of size 8
==77658==    at 0x219D67: to_tal_hdr (tal.c:174)
==77658==    by 0x219DFF: to_tal_hdr_or_null (tal.c:186)
==77658==    by 0x21A831: tal_steal_ (tal.c:497)
==77658==    by 0x1721CE: plugin_request_send (plugin.c:1991)
==77658==    by 0x11DCD9: bitcoind_estimate_fees_ (bitcoind.c:248)
==77658==    by 0x120B4B: start_fee_estimate (chaintopology.c:442)
==77658==    by 0x1BF1DE: timer_expired (timeout.c:39)
==77658==    by 0x13EFB5: io_loop_with_timers (io_loop_with_timers.c:32)
==77658==    by 0x17269F: shutdown_plugins (plugin.c:2103)
==77658==    by 0x145253: main (lightningd.c:1151)
==77658==  Address 0x5d019a8 is 24 bytes inside a block of size 304 free'd
==77658==    at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==77658==    by 0x21A527: del_tree (tal.c:421)
==77658==    by 0x21A7F5: tal_free (tal.c:486)
==77658==    by 0x16D55D: plugin_kill (plugin.c:341)
==77658==    by 0x16E350: plugin_conn_finish (plugin.c:720)
==77658==    by 0x20B4A8: destroy_conn (poll.c:244)
==77658==    by 0x20B4CC: destroy_conn_close_fd (poll.c:250)
==77658==    by 0x219F42: notify (tal.c:240)
==77658==    by 0x21A459: del_tree (tal.c:402)
==77658==    by 0x21A7F5: tal_free (tal.c:486)
==77658==    by 0x209AB9: io_close (io.c:450)
==77658==    by 0x20BC1B: io_loop (poll.c:449)
```

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
  • Loading branch information
rustyrussell committed Sep 8, 2021
1 parent 2d60cbe commit f4f0527
Show file tree
Hide file tree
Showing 4 changed files with 2 additions and 19 deletions.
13 changes: 2 additions & 11 deletions lightningd/chaintopology.c
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ static void maybe_completed_init(struct chain_topology *topo)
static void next_topology_timer(struct chain_topology *topo)
{
assert(!topo->extend_timer);
topo->extend_timer = new_reltimer(topo->ld->timers, topo,
topo->extend_timer = new_reltimer(topo->ld->timers, topo->bitcoind,
time_from_sec(topo->poll_seconds),
try_extend_tip, topo);
}
Expand Down Expand Up @@ -584,7 +584,7 @@ AUTODATA(json_command, &parse_feerate_command);
static void next_updatefee_timer(struct chain_topology *topo)
{
assert(!topo->updatefee_timer);
topo->updatefee_timer = new_reltimer(topo->ld->timers, topo,
topo->updatefee_timer = new_reltimer(topo->ld->timers, topo->bitcoind,
time_from_sec(topo->poll_seconds),
start_fee_estimate, topo);
}
Expand Down Expand Up @@ -1191,12 +1191,3 @@ void begin_topology(struct chain_topology *topo)
{
try_extend_tip(topo);
}

void stop_topology(struct chain_topology *topo)
{
/* Remove timers while we're cleaning up plugins. */
tal_free(topo->bitcoind->checkchain_timer);
tal_free(topo->extend_timer);
tal_free(topo->updatefee_timer);
}

2 changes: 0 additions & 2 deletions lightningd/chaintopology.h
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,6 @@ void setup_topology(struct chain_topology *topology,

void begin_topology(struct chain_topology *topo);

void stop_topology(struct chain_topology *topo);

struct txlocator *locate_tx(const void *ctx, const struct chain_topology *topo, const struct bitcoin_txid *txid);

static inline bool topology_synced(const struct chain_topology *topo)
Expand Down
3 changes: 0 additions & 3 deletions lightningd/lightningd.c
Original file line number Diff line number Diff line change
Expand Up @@ -1141,9 +1141,6 @@ int main(int argc, char *argv[])
stop_response = tal_steal(NULL, ld->stop_response);
}

/* Stop topology callbacks. */
stop_topology(ld->topology);

/* We're not going to collect our children. */
remove_sigchild_handler();

Expand Down
3 changes: 0 additions & 3 deletions lightningd/test/run-find_my_abspath.c
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,6 @@ void setup_topology(struct chain_topology *topology UNNEEDED,
/* Generated stub for shutdown_plugins */
void shutdown_plugins(struct lightningd *ld UNNEEDED)
{ fprintf(stderr, "shutdown_plugins called!\n"); abort(); }
/* Generated stub for stop_topology */
void stop_topology(struct chain_topology *topo UNNEEDED)
{ fprintf(stderr, "stop_topology called!\n"); abort(); }
/* Generated stub for timer_expired */
void timer_expired(tal_t *ctx UNNEEDED, struct timer *timer UNNEEDED)
{ fprintf(stderr, "timer_expired called!\n"); abort(); }
Expand Down

0 comments on commit f4f0527

Please sign in to comment.